| msg139812 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2011-07-04 22:13 |
| subprocess.check_output() doesn't close explicitly pipes if an error occurs. See for example issue #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: - call() (will fix check_call) - getstatusoutput() (will fix getoutput): see patch attached to the issue #10197 to replace os.popen() by subprocess.Popen |
|
|
| msg139848 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2011-07-05 11:02 |
| 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. |
|
|
| msg139903 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2011-07-05 20:36 |
| See also issue #12044 which changed the context manager to call the wait() method. |
|
|
| msg143297 - (view) |
Author: Ross Lagerwall (rosslagerwall) |
Date: 2011-09-01 05:05 |
| 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. |
|
|
| msg143312 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2011-09-01 08:21 |
| > The second patch looks good. Tests? Ok, I will try to write a new patch with tests. > But, it *probably* shouldn't be applied to 2.7& 3.2 given that it is a behaviour change. I consider that this issue is a bug, so it should be fixed in 2.7 and 3.2. I agree that *killing* the process is a behaviour change, but we can just close pipes on error for 2.7 and 3.2. |
|
|
| msg143313 - (view) |
Author: Ross Lagerwall (rosslagerwall) |
Date: 2011-09-01 08:49 |
| > I consider that this issue is a bug, so it should be fixed in 2.7 and > 3.2. I agree that *killing* the process is a behaviour change, but we > can just close pipes on error for 2.7 and 3.2. Yeah, that seems right. |
|
|
| msg143359 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2011-09-01 21:54 |
| New changeset 86b7f14485c9 by Victor Stinner in branch 'default': Issue #12494: Close pipes and kill process on error in subprocess functions http://hg.python.org/cpython/rev/86b7f14485c9 |
|
|
| msg143360 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2011-09-01 21:57 |
| > The second patch looks good. Tests? 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 consider that this issue is a bug, so it should be fixed in 2.7 > and 3.2. 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. |
|
|
| msg262868 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2016-04-04 20:51 |
| As I understand it, this change only made it to 3.3+ |
|
|