gh-127049: Fix asyncio.subprocess.Process race condition killing an unrelated process on Unix
#127051
+59
−69
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…_signal in asyncio (python#121126)" This reverts the non-test changes of commit bd473aa.
Note that we call Popen.poll/wait in the event loop thread to avoid Popen's thread-unsafety. Without this workaround, when testing _ThreadedChildWatcher with the reproducer shared in the linked issue on my machine: * Case 1 of the known race condition ignored in pythonGH-20010 is met (and an unsafe kill call is issued) about 1 in 10 times. * The race condition pythonGH-127050 is met (causing _process_exited's assert returncode is not None to raise) about 1 in 30 times.
|
All commit authors signed the Contributor License Agreement. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposal that fixes GH-127049. The idea is to revert GH-121126 and re-fix GH-87744 in a way that doesn't introduce GH-127049. The proposed approach here is to change child watchers to behave closer to their analog on Windows (
_WindowsSubprocessTransport), i.e. letsubprocesshandle the PID lifetime management instead of havingsubprocessdo the allocation andasynciodo the deallocation, i.e. follow the guidance of #86724 (comment).In the
_ThreadedChildWatchercase this borrows thewaitid+WNOWAITidea from Trio. (See also #82811 (comment).) Note that in the_ThreadedChildWatchercase, while it would ideally be safe to just callPopen.waitin the thread instead of involvingwaitid+WNOWAIT,Popencurrently has some thread-unsafeties, so runningPopen.waitorPopen.pollin the thread in practice resulted in unsafekillcalls and brokentransport._process_exited(returncode=None)calls. See the longer commit message.I have marked this as a draft because it does not yet have regression tests. Both cases (
_PidfdChildWatcherand_ThreadedChildWatcher) can have (nearly-)deterministic (but consistent regardless) regression tests, but I am not sure how folks would want them implemented, as the reproducers I put together for the report involve monkeypatchingos.waitpidandos.kill(which is nontrivial in part becausesubprocessholds strong references toos.waitpid, and which will miss anyos.waitidcalls made withoutWNOWAITunless we patch that too).Anyway, if this PR is pursued, I'd appreciate some help with adding tests. This is also my first PR proposed to CPython so any feedback/pointers are appreciated. :)