classification
Title: subprocess: check_output() doesn't close pipes on error
Type: Stage:
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, martin.panter, python-dev, rosslagerwall
Priority: normal Keywords: patch

Created on 2011-07-04 22:13 by haypo, last changed 2016-04-04 20:51 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess_check_output.patch haypo, 2011-07-04 22:13 review
subprocess_check_output-2.patch haypo, 2011-07-05 11:02 review
Messages (9)
msg139812 - (view) Author: STINNER Victor (haypo) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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) (Python committer) 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 (haypo) * (Python committer) 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) (Python committer) 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 (haypo) * (Python committer) 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) * (Python committer) Date: 2016-04-04 20:51
As I understand it, this change only made it to 3.3+
History
Date User Action Args
2016-04-04 20:51:44martin.pantersetnosy: + martin.panter

messages: + msg262868
versions: - Python 2.7, Python 3.2
2011-09-01 21:57:18hayposetstatus: open -> closed
resolution: fixed
messages: + msg143360
2011-09-01 21:54:29python-devsetnosy: + python-dev
messages: + msg143359
2011-09-01 08:49:03rosslagerwallsetmessages: + msg143313
2011-09-01 08:21:18hayposetmessages: + msg143312
2011-09-01 05:05:51rosslagerwallsetmessages: + msg143297
2011-07-11 12:06:46rosslagerwallsetnosy: + rosslagerwall
2011-07-05 20:36:40hayposetmessages: + msg139903
2011-07-05 11:02:45hayposetfiles: + subprocess_check_output-2.patch

messages: + msg139848
2011-07-04 22:13:45haypocreate