gh-153962: Re-raise unexpected OSError in subprocess Popen._internal_poll - #153964
gh-153962: Re-raise unexpected OSError in subprocess Popen._internal_poll#153964fedonman wants to merge 4 commits into
OSError in subprocess Popen._internal_poll#153964Conversation
…ernal_poll On POSIX, Popen._internal_poll() caught every OSError from os.waitpid() and, unless it was ECHILD or the call came from __del__ (a non-None _deadstate), silently discarded it, leaving returncode as None and masking a real failure. Add an else branch that re-raises such unexpected errors. The re-raise is only reachable when _deadstate is None; __del__ and _cleanup() always pass a non-None _deadstate, so __del__ still never raises. ECHILD handling is unchanged.
|
@zware part of EuroPython sprint. |
|
Note: Please don't do these periodic |
|
Noted! |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
See also @vstinner's objection on the issue.
| # An unexpected error (e.g. EINVAL) must not be | ||
| # silently swallowed, leaving returncode as None: | ||
| # surface it to the caller. This branch is never | ||
| # reached from __del__, which always passes a | ||
| # non-None _deadstate. | ||
| raise |
There was a problem hiding this comment.
It harms readability if the comment is 5 times large than the actual code, especially if the code is presumably unreachable. I suggest either to leave a minimal one-line comment or to remove it.
|
I suggest closing the PR and its issue. I'm not sure that it's worth it to fix a theoretical issue. The PR issue says:
So only error fault injection can trigger the issue. |
On POSIX,
Popen.poll()caught everyOSErrorfromos.waitpid()but only did something useful with two of them:ECHILD, and the case where the call comes from__del__. Any other error, likeEINVAL, was caught and dropped, which leftreturncodeasNoneand madepoll()look like the child was still running.This adds an
else: raiseso those unexpected errors reach the caller instead of vanishing.The new
raiseonly runs when the call isn't from__del__(that case is handled first, since it passes a deadstate value), so__del__still never raises. TheECHILDbehavior is unchanged.I added a test that patches
subprocess._del_safe.waitpidto raiseEINVALand checks thatpoll()re-raises it. It fails without the change and passes with it, and the rest oftest_subprocessstill passes. The patch goes throughsubprocess._del_safebecause that's the copy ofos.waitpidthis code actually calls.Found as item 8 in devdanzin's standard library audit: https://gist.github.com/devdanzin/3198710e3c0128fda5e0a7b4e0768e5f
Fixes #153962.
Popen._internal_pollsilently swallows unexpectedOSErrorfrom waitpid() #153962