Repository navigation
gh-153962: Re-raise unexpected OSError in subprocess Popen._internal_poll - #153964
fedonman wants to merge 4 commits into
Conversation
Sorry, something went wrong.
…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. |
Sorry, something went wrong.
|
Note: Please don't do these periodic Update branch merges if they're not fixing conflicts. It's impossible to stay perfectly up to date, and it's a drain on CI resources to re-test the same code repeatedly with no other action. If necessary, a maintainer can do a branch update just before merge for a final check. |
Sorry, something went wrong.
|
Noted! |
Sorry, something went wrong.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
See also @vstinner's objection on the issue.
Sorry, something went wrong.
| # 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.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Closing after @vstinner suggestion. |
Sorry, something went wrong.
On POSIX, Popen.poll() caught every OSError from os.waitpid() but only did something useful with two of them: ECHILD, and the case where the call comes from __del__. Any other error, like EINVAL, was caught and dropped, which left returncode as None and made poll() look like the child was still running.
This adds an else: raise so those unexpected errors reach the caller instead of vanishing.
The new raise only runs when the call isn't from __del__ (that case is handled first, since it passes a deadstate value), so __del__ still never raises. The ECHILD behavior is unchanged.
I added a test that patches subprocess._del_safe.waitpid to raise EINVAL and checks that poll() re-raises it. It fails without the change and passes with it, and the rest of test_subprocess still passes. The patch goes through subprocess._del_safe because that's the copy of os.waitpid this code actually calls.
Found as item 8 in devdanzin's standard library audit: https://gist.github.com/devdanzin/3198710e3c0128fda5e0a7b4e0768e5f
Fixes #153962.