Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bpo-40550: Fix time-of-check/time-of-action issue in subprocess.Popen.send_signal. #20010

Merged
merged 2 commits into from
Nov 21, 2020
Merged

bpo-40550: Fix time-of-check/time-of-action issue in subprocess.Popen.send_signal. #20010

merged 2 commits into from
Nov 21, 2020

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented May 8, 2020 •
edited by bedevere-bot
Loading

All the implementation details have been explained the bug, please check it out.

https://bugs.python.org/issue40550

@FFY00 FFY00 requested a review from gpshead as a code owner May 8, 2020 21:20
ZackerySpytz
ZackerySpytz reviewed May 14, 2020
View reviewed changes
Copy link
Contributor

@ZackerySpytz ZackerySpytz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if this PR will be accepted, but I left some comments.

Also, this PR should probably have a unit test.

@FFY00
Copy link
Member Author

FFY00 commented May 14, 2020

This fixes a race condition, there's no way to reliably have a unit test.

The process needs to exit between the return code check and the action itself.

@remilapeyre
Copy link
Contributor

I think you could write a unit test by synchronising the threads so the race condition occurs.

You can mock os.kill() so that it waits for a threading.Event before continuing, then wait in another thread for the process to die. Once it's dead, you can release the first thread by setting the event and the kill() should raise a ProcessLookupError.

remilapeyre
remilapeyre approved these changes May 14, 2020
View reviewed changes
Copy link
Contributor

@remilapeyre remilapeyre left a comment

Choose a reason for hiding this comment

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

Hi @FFY00, I checked the test and it simulates the race condition successfully 👍

Try not to force-push during reviews thought as it makes it harder to see what has been updated since the last time one viewed that changes.

@FFY00
Copy link
Member Author

FFY00 commented May 14, 2020 •
edited by bedevere-bot
Loading

You can click on force-pushed (in @FFY00 FFY00 force-pushed the FFY00:[bpo-40550](https://bugs.python.org/issue40550) branch from 649b00f to 49e7004 5 hours ago), it should land you in this link: https://github.com/python/cpython/compare/649b00f3308818d88e237083600c1955162fdadd..49e7004bc963966bfb2f264d6022ef4477f21bcc. You can see the changes perfectly. It shows you the changes just as same as if I were to push a different commit, but without dirtying up the history. This might not be a problem in simple PRs like this but it makes a difference when you have more than 1 commit. And makes a huge difference when you are changing a lot of code, since you won't be able to look at one clean commit, you will need to look at the original commit and the fix. For these reasons, I believe force pushing to be good practice.

@FFY00
Copy link
Member Author

FFY00 commented May 14, 2020

Sorry, after writing all that I messed up the git history 😕

You can still view the correct diff here: https://github.com/python/cpython/compare/49e7004bc963966bfb2f264d6022ef4477f21bcc..3d7dfbc1c19280dd19693d8d8e0574590c0c1bee

@FFY00
Copy link
Member Author

FFY00 commented Jun 19, 2020

@gpshead do you have a minute to review this?

gpshead
gpshead reviewed Nov 21, 2020
View reviewed changes
@gpshead
Remove a redundant check of errno.
1605031 
The ProcessLookupError already means errno == ESRCH.
@gpshead gpshead changed the title bpo-40550: fix time-of-check/time-of-action issue in multiprocessing bpo-40550: Fix time-of-check/time-of-action issue in subprocess.Popen.send_signal. Nov 21, 2020
@gpshead gpshead self-assigned this Nov 21, 2020
@gpshead gpshead merged commit 01a202a into python:master Nov 21, 2020
@miss-islington
Copy link
Contributor

Thanks @FFY00 for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 21, 2020
@FFY00 @miss-islington
bpo-40550: Fix time-of-check/time-of-action issue in subprocess.Popen…
3f1fbdc 
….send_signal. (pythonGH-20010)

send_signal() now swallows the exception if the process it thought was still alive winds up not to exist anymore (always a plausible race condition despite the checks).

Co-authored-by: Gregory P. Smith <[email protected]>
(cherry picked from commit 01a202a)

Co-authored-by: Filipe Laíns <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Nov 21, 2020
@bedevere-bot
Copy link

GH-23439 is a backport of this pull request to the 3.9 branch.

@miss-islington
Copy link
Contributor

Sorry, @FFY00 and @gpshead, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 01a202ab6b0ded546e47073db6498262086c52e9 3.8

@gpshead gpshead added type-bug An unexpected behavior, bug, or error and removed needs backport to 3.8 labels Nov 21, 2020
miss-islington added a commit that referenced this pull request Nov 21, 2020
@miss-islington @FFY00
bpo-40550: Fix time-of-check/time-of-action issue in subprocess.Popen…
713b4bb 
….send_signal. (GH-20010)

send_signal() now swallows the exception if the process it thought was still alive winds up not to exist anymore (always a plausible race condition despite the checks).

Co-authored-by: Gregory P. Smith <[email protected]>
(cherry picked from commit 01a202a)

Co-authored-by: Filipe Laíns <[email protected]>
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@FFY00 @gpshead @adorilson
bpo-40550: Fix time-of-check/time-of-action issue in subprocess.Popen…
92c4faa 
….send_signal. (pythonGH-20010)

send_signal() now swallows the exception if the process it thought was still alive winds up not to exist anymore (always a plausible race condition despite the checks).

Co-authored-by: Gregory P. Smith <[email protected]>
@gschaffner gschaffner mentioned this pull request Nov 20, 2024
Open
gschaffner added a commit to gschaffner/cpython that referenced this pull request Nov 20, 2024
@gschaffner
Fix child process watchers racing to free PIDs managed by Popen
65cce5c 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZackerySpytz ZackerySpytz ZackerySpytz left review comments

@gpshead gpshead gpshead approved these changes

@remilapeyre remilapeyre remilapeyre approved these changes

Assignees

@gpshead gpshead

Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@FFY00 @remilapeyre @miss-islington @bedevere-bot @gpshead @ZackerySpytz @the-knights-who-say-ni