Skip to content

bpo-39995: Fix race condition in ProcessPoolExecutor._ThreadWakeup #19751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

aeros
Copy link
Contributor

@aeros aeros commented Apr 28, 2020

Addresses a race condition in ProcessPoolExecutor._ThreadWakeup by using a threading.Event to ensure either end of its pipe is not closed while it is actively sending or receiving bytes.

Fixes an intermittent failure in test_concurrent_futures.ProcessPoolSpawnProcessPoolExecutorTest.test_killed_child, and aims to fix some of the others reported in the bpo issue.

https://bugs.python.org/issue39995

@aeros aeros added the type-bug An unexpected behavior, bug, or error label Apr 28, 2020
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would favour a non-locking solution (hence my suggestion on the bpo issue), but this PR is faulty anyway. You could very well have the following execution sequence:

Thread A                                Thread B
--------                                ---------

<Enters wakeup()>

if not self._closed:  # Success

                                        <Enters close()>
                                      
                                        self._closed = True
                                        self._not_running.wait()  # Success
                                        self._writer.close()
                                      
    self._not_running.clear()
    self._writer.send_bytes(b"")  # Oops

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be poked with soft cushions!

@aeros
Copy link
Contributor Author

aeros commented Apr 28, 2020

@pitrou Ah, I had somehow not considered that scenario and got a bit carried away with the surface-level success of it appearing to resolve test_killed_child failure locally. In retrospect, I can see now that this PR would simply make the race condition slightly less likely to occur, but does not properly fix it. Thanks for the feedback; I'll close the PR.

@vstinner
Copy link
Member

this PR is faulty anyway

I wrote PR #19758 which is similar to this one, but doesn't have this race condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes skip news type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants