bpo-39104: Fix hanging ProcessPoolExecutor on shutdown nowait with pickling failure #17670

tomMoral · 2019-12-20T06:54:20Z

As reported initially by @rad-pat in #6084, the following script causes a deadlock.

from concurrent.futures import ProcessPoolExecutor


class ObjectWithPickleError():
    """Triggers a RuntimeError when sending job to the workers"""

    def __reduce__(self):
        raise RuntimeError()


if __name__ == "__main__":
    e = ProcessPoolExecutor()
    f = e.submit(id, ObjectWithPickleError())
    e.shutdown(wait=False)
    f.result()  # Deadlock on get

This is caused by the fact that the main process is closing communication channels that might be necessary to the queue_management_thread later. To avoid this, this PR let the queue_management_thread manage all the closing.

https://bugs.python.org/issue39104

Automerge-Triggered-By: @pitrou

rad-pat

Just a comment. I had also started trying to make a solution, but perhaps I didn't grasp the entire problem. Please see rad-pat@188c222, including a more basic test without the pickle error.

rad-pat · 2019-12-20T09:07:14Z

Lib/concurrent/futures/process.py

@@ -87,7 +87,12 @@ def close(self):
        self._reader.close()

    def wakeup(self):
-        self._writer.send_bytes(b"")
+        try:


Maybe a _closed flag might be better than catching the exception? You could also then check the flag in the clear method?

Yes you are right, this looks better.

…tdown(wait=False) When a ProcessPoolExecutor was created, a job added and then shutdown with wait=False, an OSError: handle is closed error was raised by the ThreadWakeup class. The threadwakeup should not be closed on shutdown if wait is False, but in the shutdown_worker method.

tomMoral · 2020-01-17T16:11:53Z

Sorry for the delay @rad-pat !

I cherry picked your solution to add more tests and add the closed flag.

rad-pat · 2020-01-17T16:43:38Z

Thanks, it looks good to me @tomMoral, just need some core dev interest now and hopefully we can get this resolved

rad-pat · 2020-02-11T10:34:59Z

@pitrou I believe you have an interest in reviewing PRs for multiprocessing, is there any chance of a review of this one? Thanks.

pitrou

Thank you very much for the PR. Some comments below.

Lib/concurrent/futures/process.py

pitrou · 2020-02-13T17:44:25Z

Lib/concurrent/futures/process.py

@@ -339,6 +346,8 @@ def shutdown_worker():

        # Release the queue's resources as soon as possible.


Not a requirement for this PR, but it seems to me that the _queue_management_worker function has grown quite long and complicated. Do you think it could be turned into an object with a bunch of short and readable helper methods?

Yes this would be a very much easier to read indeed. I will try to make a new PR with this change when I get a chance.

Looking forward to your PR then @tomMoral :-)

Lib/test/test_concurrent_futures.py

bedevere-bot · 2020-02-13T17:48:25Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

rad-pat · 2020-02-14T12:15:29Z

@tomMoral , I've hopefully made all the requested changes here https://github.com/rad-pat/cpython/tree/FIX_hanging_interpreter_executor_shutdown_nowait, but I can't make a PR to your repo. Would you please be able to pull in the changes?

tomMoral · 2020-02-14T14:03:46Z

@tomMoral , I've hopefully made all the requested changes here rad-pat/cpython@FIX_hanging_interpreter_executor_shutdown_nowait , but I can't make a PR to your repo. Would you please be able to pull in the changes?

Thanks @rad-pat , I updated the PR with your change and made a couple of adjustments!

rad-pat · 2020-02-15T14:24:13Z

Great stuff, thanks @tomMoral , I'm hoping/pushing for this fix because it's giving me a production issue right now. So, comment per bot's request ... I have made the requested changes; please review again

Lib/test/test_concurrent_futures.py

Fix typos

miss-islington · 2020-02-16T18:03:29Z

@tomMoral: Status check is done, and it's a failure ❌ .

miss-islington · 2020-02-16T18:09:25Z

@tomMoral: Status check is done, and it's a success ✅ .

@rad-pat

…ckling failure (GH-17670) As reported initially by @rad-pat in #6084, the following script causes a deadlock. ``` from concurrent.futures import ProcessPoolExecutor class ObjectWithPickleError(): """Triggers a RuntimeError when sending job to the workers""" def __reduce__(self): raise RuntimeError() if __name__ == "__main__": e = ProcessPoolExecutor() f = e.submit(id, ObjectWithPickleError()) e.shutdown(wait=False) f.result() # Deadlock on get ``` This is caused by the fact that the main process is closing communication channels that might be necessary to the `queue_management_thread` later. To avoid this, this PR let the `queue_management_thread` manage all the closing. https://bugs.python.org/issue39104 Automerge-Triggered-By: @pitrou

tomMoral added 2 commits December 20, 2019 07:39

FIX hang on executor shutdown no wait with failure

44f70fd

TST non regression test for python#6084

38d8c98

the-knights-who-say-ni added the CLA signed label Dec 20, 2019

bedevere-bot added the awaiting review label Dec 20, 2019

tomMoral mentioned this pull request Dec 20, 2019

bpo-33056 FIX leaking fd in concurrent.futures.ProcessPoolExecutor #6084

Merged

rad-pat reviewed Dec 20, 2019

View reviewed changes

csabella requested a review from pitrou February 11, 2020 11:54

pitrou changed the title ~~bpo-39104 Fix hanging ProcessPoolExcutor on shutdown nowait with pickling failure~~ bpo-39104: Fix hanging ProcessPoolExcutor on shutdown nowait with pickling failure Feb 13, 2020

pitrou requested changes Feb 13, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Feb 13, 2020

CLN improve test and remove unnecessary code

de5ef88

pitrou reviewed Feb 16, 2020

View reviewed changes

Lib/test/test_concurrent_futures.py Outdated Show resolved Hide resolved

pitrou reviewed Feb 16, 2020

View reviewed changes

Lib/test/test_concurrent_futures.py Outdated Show resolved Hide resolved

pitrou reviewed Feb 16, 2020

View reviewed changes

Lib/test/test_concurrent_futures.py Outdated Show resolved Hide resolved

pitrou reviewed Feb 16, 2020

View reviewed changes

Lib/test/test_concurrent_futures.py Outdated Show resolved Hide resolved

Apply suggestions from code review

8d6b285

Fix typos

pitrou approved these changes Feb 16, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Feb 16, 2020

pitrou changed the title ~~bpo-39104: Fix hanging ProcessPoolExcutor on shutdown nowait with pickling failure~~ bpo-39104: Fix hanging ProcessPoolExecutor on shutdown nowait with pickling failure Feb 16, 2020

Add NEWS

9d47cf9

pitrou added the 🤖 automerge label Feb 16, 2020

miss-islington merged commit a5cbab5 into python:master Feb 16, 2020

bedevere-bot removed the awaiting merge label Feb 16, 2020

sthagen mentioned this pull request Feb 16, 2020

bpo-39104: Fix hanging ProcessPoolExecutor on shutdown nowait with pi… sthagen/python-cpython#252

Merged

tomMoral mentioned this pull request Feb 18, 2020

bpo-39678 RFC refactor queue manager thread #18551

Merged

This was referenced Apr 29, 2020

MTN backport some changes from cpython joblib/loky#245

Closed

loky fails with OSError and FileNotFoundError when executor.shutdown(wait=False) joblib/loky#241

Closed

		@@ -339,6 +346,8 @@ def shutdown_worker():

		# Release the queue's resources as soon as possible.

Uh oh!

bpo-39104: Fix hanging ProcessPoolExecutor on shutdown nowait with pickling failure #17670

bpo-39104: Fix hanging ProcessPoolExecutor on shutdown nowait with pickling failure #17670

Uh oh!

Conversation

tomMoral commented Dec 20, 2019 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rad-pat left a comment

Choose a reason for hiding this comment

Uh oh!

rad-pat Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

tomMoral Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

tomMoral commented Jan 17, 2020

Uh oh!

rad-pat commented Jan 17, 2020

Uh oh!

rad-pat commented Feb 11, 2020

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

tomMoral Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

pitrou Feb 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Feb 13, 2020

Uh oh!

rad-pat commented Feb 14, 2020

Uh oh!

tomMoral commented Feb 14, 2020

Uh oh!

rad-pat commented Feb 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miss-islington commented Feb 16, 2020

Uh oh!

miss-islington commented Feb 16, 2020

Uh oh!

Uh oh!

tomMoral commented Dec 20, 2019 •

edited by miss-islington

Loading