Skip to content

gh-96863: Fix subprocess.Popen.wait to raise ProcessLookupError for external waits #137804

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

Closed
wants to merge 6 commits into from
Closed

gh-96863: Fix subprocess.Popen.wait to raise ProcessLookupError for external waits #137804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ea5a376
Fix subprocess.Popen.wait to raise ProcessLookupError for external waits
Writtic Aug 15, 2025
d8af14f
Ensure ProcessLookupError is raised when waiting for killed process
Writtic Aug 15, 2025
cb8d874
Handle ProcessLookupError with a try–except block, since it’s not alw…
Writtic Aug 15, 2025
a542c71
Handle ProcessLookupError in wait calls for subprocesses in test_eintr
Writtic Aug 15, 2025
f77e2ea
Ensure process is killed in test_wait_after_external_wait
Writtic Aug 15, 2025
327f904
Use f-string for exception message in test_wait_after_external_wait
Writtic Aug 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions Lib/subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2020,11 +2020,17 @@ def _try_wait(self, wait_flags):
try:
(pid, sts) = os.waitpid(self.pid, wait_flags)
except ChildProcessError:
# This happens if SIGCLD is set to be ignored or waiting
# This happens if SIGCHLD is set to be ignored or waiting
# for child processes has otherwise been disabled for our
# process. This child is dead, we can't get the status.
pid = self.pid
sts = 0
# process. This child is dead, we can't get the status.
if signal.getsignal(signal.SIGCHLD) == signal.SIG_IGN:
# SIGCHLD was ignored; for compatibility return a zero
# status for this child.
pid = self.pid
sts = 0
return (pid, sts)

raise ProcessLookupError(f'Process {self.pid} is already waited on externally')
Copy link
Member

@corona10 corona10 Aug 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@gpshead I think that we should specify status code rather than raise exception because it will change current behavior. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Callers do not expect wait() to raise an exception when the process has died by default because our behavior has been not to raise. Often callers don't even care about the exit code. As seen in the tests that had to be updated due to this proposed change. I'm sure people have written code that calls wait multiple times as well, expecting later calls to be no-ops. We don't want to break that.

I'll reply further on the issue. I do not think we want this feature.

return (pid, sts)


Expand Down
12 changes: 10 additions & 2 deletions Lib/test/_test_eintr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,11 @@ def _test_wait_multiple(self, wait_func):
wait_func()
# Call the Popen method to avoid a ResourceWarning
for proc in processes:
proc.wait()
try:
proc.wait()
except ProcessLookupError:
# Process already exited, which is expected but not always raised
pass

def test_wait(self):
self._test_wait_multiple(os.wait)
Expand All @@ -114,7 +118,11 @@ def _test_wait_single(self, wait_func):
proc = self.new_sleep_process()
wait_func(proc.pid)
# Call the Popen method to avoid a ResourceWarning
proc.wait()
try:
proc.wait()
except ProcessLookupError:
# Process already exited, which is expected but not always raised
pass

def test_waitpid(self):
self._test_wait_single(lambda pid: os.waitpid(pid, 0))
Expand Down
7 changes: 6 additions & 1 deletion Lib/test/test_asyncio/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,12 @@ async def kill_running():

# kill the process (but asyncio is not notified immediately)
proc.kill()
proc.wait()
try:
proc.wait()
except ProcessLookupError:
Copy link
Member

Choose a reason for hiding this comment

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

Would you like to use https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises if it is always happened?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I’ll use assertRaises here.

Copy link
Author

Choose a reason for hiding this comment

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

It seems it’s not always raised, so I’ll revert to using a try–except block on cb8d874.

# Process already exited, which is expected but not always
# raised
pass

proc.kill = mock.Mock()
proc_returncode = proc.poll()
Expand Down
20 changes: 20 additions & 0 deletions Lib/test/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3361,6 +3361,26 @@ def test_stopped(self):

self.assertEqual(returncode, -3)

def test_wait_after_external_wait(self):
"""Test behavior when process is already waited on externally; issue96863."""
# Create a process with non-zero exit code
proc = subprocess.Popen([sys.executable, '-c', 'import sys; sys.exit(1)'])

# Wait for the process externally using os.wait()
pid, status = os.wait()
self.assertEqual(pid, proc.pid)
self.assertEqual(os.WEXITSTATUS(status), 1)

# Now calling proc.wait() should raise ProcessLookupError
# instead of returning 0 (the old buggy behavior)
with self.assertRaises(ProcessLookupError) as cm:
proc.wait()

# Verify the exception contains proper information
self.assertIn(f'{proc.pid} is already waited on externally',
str(cm.exception))
proc.kill()

def test_send_signal_race(self):
# bpo-38630: send_signal() must poll the process exit status to reduce
# the risk of sending the signal to the wrong process.
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2025-08-15-14-06-33.gh-issue-96863.rUk3aJ.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix :meth:`subprocess.Popen.wait` returning zero always when process already
waited on externally. Now raises :exc:`ProcessLookupError` instead of losing
exit status. Preserves ``SIGCHLD=SIG_IGN`` compatibility.
Loading