Incorrect exit status of an already waited process in subprocess.Popen

[csernazs]

Bug report

If a process run by subprocess.Popen and its exit status is obtained by os.wait() or os.waitpid(), then calling the wait() method on the Popen object always returns zero.

Example:

import os
import subprocess

proc = subprocess.Popen(["false"]) # could be any process with non-zero exit status
pid, status = os.wait()
print("status from os.wait()", status)  # prints non-zero exit status correctly
print("status from proc.wait()", proc.wait()) # prints zero exit status incorrectly

The root cause of the problem is that the child has been already wait()ed, so the Popen object has no chance to obtain the exit code as the process no longer exists. It calls waitpid() here but it ignores ECHILD and defaults the status to zero.

Your environment

• CPython versions tested on: Python 3.10.4 (main, Jun 29 2022, 12:14:53) [GCC 11.2.0] on linux
• Operating system and architecture: Linux x86-64

Linked PRs

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

[0x1306e6d]

Hello, let me try to fix this issue :)

[Writtic]

@corona10 Let me try to fix it.

[corona10]

Please go a head!

[gpshead]

(taking discussion from the PR over here) https://github.com/python/cpython/pull/137804/files#r2280997788 -

We cannot raise an exception from an API where existing code is not expecting to have to handle one.

wait() has always been a best effort function because it is impossible to prevent something else from doing the wait and thus consuming the exit code. If anything perhaps we should just document that and suggest people use os.waitpid if it is important for them to be robust to rogue other waiters in their process or on their system consuming exit codes ("if something outside of the subprocess.Popen instance already waited on the child process, the exit code is lost and zero will be returned - use os.waitpid if you need to detect this unusual situation").

We could also add a related note to the os.wait() docs that suggests people not use that blind "any child" API in code uses subprocess or multiprocessing or concurrent.futures.ProcessPoolExecutor as it will inhibit their ability to get the exit status of their own child processes.

---

If we were going to add this issue's feature all, due to the behavior change from the common need case, it should be behind a feature flag, either yet another Popen keyword only parameter and instance property to set, and/or a keyword only flag to wait() itself. That way users could opt-in to getting an exception if the child has already been waited on, at which point we should just let that be the ChildProcessError itself when the flag is set. (the reason I suggest "instance property to be set" is that this design pattern would allow people to write version agnostic subprocess using code that does not need to check the version and if the feature exists or not - that pattern doesn't make for a great API otherwise)

I doubt anyone would ever use that flag? Users can already just call os.waitpid themselves if they want the behavior.

If we were designing a subprocess API from the start, I agree, this exception would probably have been a good idea. We've got decades of code using the subprocess API at this point and have not heard significant calls for a change here.

[csernazs]

Thanks for the summary!

I agree that solving this problem is nearly impossible properly (no API breakings, no additional flags). This is why I didn't propose a PR or solution idea as I wanted to see what others think - I agree with @gpshead. If subprocess was a totally new API (about to be released) then raising an exception maybe a good idea.

I think the "fix" would be a documentation upgrade as you suggested. This is also the same category as if someone calls os.read on an fd which is managed by someone else, then bad things will happen.

I had also an idea about keeping a global state of the wait()ed or waitpid()ed processes' status (so this can be updated by os.wait and os.waitpid and subprocess could check that), but I think that would add more issues (memory "leak", synchronization, pid reaches maxmium then the same pid can be seen later on, etc). Since subprocess module have an accounting of the current process objects here, os.wait or os.waitpid could iterate over the list and update the process object related to the same pid. I also find this solution ugly and perhaps it would cause more issues in the long term, just wanted to mention in case you have different opinion (so you think it is doable).

Thanks,
Zsolt