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

gh-90872: Fix subprocess.Popen.wait() for negative timeout #116989

Merged
merged 2 commits into from
Mar 19, 2024
Merged

gh-90872: Fix subprocess.Popen.wait() for negative timeout #116989

merged 2 commits into from
Mar 19, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Mar 19, 2024 •
edited by bedevere-app bot
Loading

On Windows, subprocess.Popen.wait() no longer calls WaitForSingleObject() with a negative timeout: pass 0 ms if the timeout is negative.

@vstinner
pythongh-90872: Fix subprocess.Popen.wait() for negative timeout
e29c2aa 
On Windows, subprocess.Popen.wait() no longer calls
WaitForSingleObject() with a negative timeout: pass 0 ms if the
timeout is negative.
@vstinner vstinner requested a review from gpshead as a code owner March 19, 2024 10:12
@bedevere-app bedevere-app bot mentioned this pull request Mar 19, 2024
Closed
@vstinner
Copy link
Member Author

cc @serhiy-storchaka

@vstinner vstinner added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Mar 19, 2024
@serhiy-storchaka
Copy link
Member

How difficult is to write test?

@vstinner
Copy link
Member Author

@serhiy-storchaka:

How difficult is to write test?

I added a test. Please review the updated change.

@serhiy-storchaka
Copy link
Member

How difficult is to write test without mocking?

@vstinner
Copy link
Member Author

How difficult is to write test without mocking?

I would prefer to not have to reimplement "wait until a process completes" in test_subprocess.

What is the problem with mocking?

@zooba
Copy link
Member

zooba commented Mar 19, 2024

I prefer the mocking in this case, as it's testing a change to our logic.

Fixing the DWORD converter would be worth a deeper test, but we know its behaviour right now is wrong, and so we just want to make sure we don't pass it a risky value.

@vstinner
Copy link
Member Author

Fixing the DWORD converter would be worth a deeper test, but we know its behaviour right now is wrong, and so we just want to make sure we don't pass it a risky value.

I have it on my TODO list for 3 years: see https://discuss.python.org/t/conversion-between-python-and-c-integers/44117 discussion.

Changing the default behavior is non trivial. It's a low-priority for me. So I wrote this "trivial" change to fix at least subprocess.

Anyway, if DWORD converter is changed, I suppose that in the future, it will raise an exception (maybe after a deprecation period), so it's better to not pass negative values :-)

@serhiy-storchaka
Copy link
Member

The change itself in this PR is simple, and it can go without tests (if it is difficult to write proper tests). My concern is that it may be not enough, and there may be other places that need a fix for this case or similar cases. If it is not too difficult, I would prefer a test without mocking.

@zooba
Copy link
Member

zooba commented Mar 19, 2024

The change itself in this PR is simple, and it can go without tests (if it is difficult to write proper tests)

This change does have a test, it's just the originally observed (bad) behaviour does not.

@vstinner vstinner merged commit 27cf3ed into python:main Mar 19, 2024
35 checks passed
@vstinner vstinner deleted the subprocess_wait branch March 19, 2024 13:42
@miss-islington-app
Copy link

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 27cf3ed00cfe942f4277c273a3dda8ee2ba61fc8 3.12

@miss-islington-app
Copy link

Sorry, @vstinner, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 27cf3ed00cfe942f4277c273a3dda8ee2ba61fc8 3.11

@bedevere-app
Copy link

bedevere-app bot commented Mar 19, 2024

GH-117002 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Mar 19, 2024
@bedevere-app
Copy link

bedevere-app bot commented Mar 19, 2024

GH-117003 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Mar 19, 2024
@vstinner
Copy link
Member Author

Merged, thanks for your reviews!

vstinner added a commit to vstinner/cpython that referenced this pull request Mar 19, 2024
@vstinner
pythongh-90872: Fix subprocess.Popen.wait() for negative timeout (pyt…
6c47965 
…hon#116989)

On Windows, subprocess.Popen.wait() no longer calls
WaitForSingleObject() with a negative timeout: pass 0 ms if the
timeout is negative.

(cherry picked from commit 27cf3ed)
vstinner added a commit that referenced this pull request Mar 19, 2024
@vstinner
[3.12] gh-90872: Fix subprocess.Popen.wait() for negative timeout (#1…
35e8f04 
…16989) (#117002)

gh-90872: Fix subprocess.Popen.wait() for negative timeout (#116989)

On Windows, subprocess.Popen.wait() no longer calls
WaitForSingleObject() with a negative timeout: pass 0 ms if the
timeout is negative.

(cherry picked from commit 27cf3ed)
vstinner added a commit that referenced this pull request Mar 19, 2024
@vstinner
[3.11] gh-90872: Fix subprocess.Popen.wait() for negative timeout (#1…
c7b01e3 
…16989) (#117003)

gh-90872: Fix subprocess.Popen.wait() for negative timeout (#116989)

On Windows, subprocess.Popen.wait() no longer calls
WaitForSingleObject() with a negative timeout: pass 0 ms if the
timeout is negative.

(cherry picked from commit 27cf3ed)
vstinner added a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
@vstinner
pythongh-90872: Fix subprocess.Popen.wait() for negative timeout (pyt…
c4a6d3c 
…hon#116989)

On Windows, subprocess.Popen.wait() no longer calls
WaitForSingleObject() with a negative timeout: pass 0 ms if the
timeout is negative.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
@vstinner @adorilson
pythongh-90872: Fix subprocess.Popen.wait() for negative timeout (pyt…
a1648be 
…hon#116989)

On Windows, subprocess.Popen.wait() no longer calls
WaitForSingleObject() with a negative timeout: pass 0 ms if the
timeout is negative.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@vstinner @diegorusso
pythongh-90872: Fix subprocess.Popen.wait() for negative timeout (pyt…
860d719 
…hon#116989)

On Windows, subprocess.Popen.wait() no longer calls
WaitForSingleObject() with a negative timeout: pass 0 ms if the
timeout is negative.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zooba zooba zooba approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner

Assignees

@vstinner vstinner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vstinner @serhiy-storchaka @zooba