gh-96863: Fix subprocess.Popen.wait to raise ProcessLookupError for external waits #137804
Conversation
Writtic
requested review from
gpshead,
1st1,
asvetlov,
kumaraditya303 and
willingc
as code owners
|
All commit authors signed the Contributor License Agreement. |
| proc.wait() | ||
| try: | ||
| proc.wait() | ||
| except ProcessLookupError: |
There was a problem hiding this comment.
Would you like to use https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises if it is always happened?
There was a problem hiding this comment.
Thanks! I’ll use assertRaises here.
There was a problem hiding this comment.
It seems it’s not always raised, so I’ll revert to using a try–except block on cb8d874.
| sts = 0 | ||
| return (pid, sts) | ||
|
|
||
| raise ProcessLookupError(f'Process {self.pid} is already waited on externally') |
There was a problem hiding this comment.
@gpshead I think that we should specify status code rather than raise exception because it will change current behavior. WDYT?
There was a problem hiding this comment.
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.
|
BTW, thanks for the contribution! not all PRs make sense, getting eyeballs on issues that perhaps haven't had sufficient discussion and discussing what is actually needed is the larger benefit in such situations. :) (ie: don't feel bad that i'm closing your PR, it was helpful regardless - see the discussion on the issue, i think other PR opportunities exist for this one) |
Patch for the Issue: #96863