Skip to content

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

Merged
gpshead merged 2 commits intopython:masterfrom
FFY00:bpo-40550
Nov 21, 2020
Merged

bpo-40550: Fix time-of-check/time-of-action issue in subprocess.Popen.send_signal.#20010
gpshead merged 2 commits intopython:masterfrom
FFY00:bpo-40550

Conversation

@FFY00
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

remilapeyre commented May 14, 2020

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
Copy Markdown

@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
Copy Markdown
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
bpo-40550: fix time-of-check/time-of-action issue in multiprocessing
3d7dfbc 
Signed-off-by: Filipe Laíns <lains@archlinux.org>
@FFY00
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

miss-islington commented Nov 21, 2020

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 <greg@krypto.org>
(cherry picked from commit 01a202a)

Co-authored-by: Filipe Laíns <lains@archlinux.org>
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Nov 21, 2020

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

@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Nov 21, 2020

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 <greg@krypto.org>
(cherry picked from commit 01a202a)

Co-authored-by: Filipe Laíns <lains@archlinux.org>
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 <greg@krypto.org>
@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

@gpshead gpshead gpshead approved these changes

+2 more reviewers

@ZackerySpytz ZackerySpytz ZackerySpytz left review comments

@remilapeyre remilapeyre remilapeyre approved these changes

Reviewers whose approvals may not affect merge requirements

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