-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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.
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
subprocess: check_output() doesn't close pipes on error #56703
Comments
subprocess.check_output() doesn't close explicitly pipes if an error occurs. See for example issue bpo-12493 for an example of an error on .communicate(). Attached patch uses a context manager to ensure that all pipes are always closed and that the status is read to avoid zombies. Other subprocess functions should be fixed:
|
subprocess_check_output-2.patch is a more complete patch: "fix" (?) call(), check_output() and getstatusoutput(). These functions kill the process if an exception occurs to not hang on wait() in Popen.__exit__(). Because of the kill, I don't know if the fix should be applied to 2.7 and 3.2. In case of an exception, is it better to keep the subprocess alive, or to kill it? If we keep it alive, the caller of the function cannot interact with the process, and we don't know exactly when it will finish. By "exception", I mean unexpected exceptions: check_output() handles explicitly the TimeoutExpired exception. |
See also issue bpo-12044 which changed the context manager to call the wait() method. |
The second patch looks good. Tests? I think it would be better to kill the process than to let it carry on. But, it *probably* shouldn't be applied to 2.7 & 3.2 given that it is a behaviour change. |
Ok, I will try to write a new patch with tests.
I consider that this issue is a bug, so it should be fixed in 2.7 and |
Yeah, that seems right. |
New changeset 86b7f14485c9 by Victor Stinner in branch 'default': |
I don't see how to test that the pipes are closed, because the Popen object (and its stdout and stderr attributes) are not visible outside the patched functions.
I tried to write a patch, but I chose to keep the code unchanged. I prefer to keep this little tricky bug, instead of introducing another more important regression. Reopen the issue if you have a script to test the fix, or if you really want a backport. |
As I understand it, this change only made it to 3.3+ |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: