classification
Title: subprocess.Popen.__exit__ doesn't wait for process end
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.2, Python 3.3
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: brian.curtin, gregory.p.smith, haypo, neologix, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2011-05-09 19:24 by pitrou, last changed 2011-07-05 20:36 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess_zombie.diff neologix, 2011-05-11 16:44 review
Messages (17)
msg135635 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-05-09 19:24
I find it a bit strange that Popen.__exit__ closes all standard file descriptors leading to the child process, but doesn't wait for process end afterwards.
(context management support was added in issue10554)
msg135641 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-05-09 20:41
Seems like it would be enough to add a wait() at the end?

diff -r 9e473917cbfb Lib/subprocess.py
--- a/Lib/subprocess.py Mon May 09 21:17:02 2011 +0200
+++ b/Lib/subprocess.py Mon May 09 15:30:02 2011 -0500
@@ -796,6 +796,7 @@
             self.stderr.close()
         if self.stdin:
             self.stdin.close()
+        self.wait()

     def __del__(self, _maxsize=sys.maxsize, _active=_active):
         if not self._child_created:
msg135642 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-05-09 20:42
> Seems like it would be enough to add a wait() at the end?

Probably, perhaps with a try/finally?

I'm not entirely sure this is a good idea, by the way. May there be some
drawbacks?
msg135648 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-05-09 21:02
Actually, I don't think the wait() is a good idea. If you want to block and infinitely wait on the process to close, you should do so explicitly.

It's probably better that we try to use terminate() or kill() and raise if that fails.
msg135649 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-05-09 21:05
> Actually, I don't think the wait() is a good idea. If you want to
> block and infinitely wait on the process to close, you should do so
> explicitly.

Ok.

> It's probably better that we try to use terminate() or kill() and raise if that fails.

Uh, I think it's worse. Using a context manager shouldn't do something
potentially destructive like killing a process.
msg135652 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-05-09 21:11
Hm, yeah, not sure what I was thinking there.

I'm thinking there's not a lot we can do here, but also not a lot that we should do here. We don't want to wait, and we don't want to close, so maybe we should just document that the usage should be limited only to expecting the open handles to be closed?
msg135658 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-05-09 22:00
> I'm thinking there's not a lot we can do here, but also not a lot that
> we should do here. We don't want to wait, and we don't want to close,
> so maybe we should just document that the usage should be limited only
> to expecting the open handles to be closed?

Sounds good indeed :)
msg135675 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2011-05-10 02:28
Looks like we already mention that.

"""
Popen objects are supported as context managers via the with statement, closing any open file descriptors on exit.
"""

Antoine, do you think this should be more strongly worded?
msg135690 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-05-10 09:11
Well, no, looks ok for me then :)
msg135715 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-10 16:08
There's just one thing I'm concerned with.
People using context managers tend to expect the __exit__ method to
perform cleanup actions and release corresponding resources if
necessary, for example closing the underlying file or socket.
So my guess is that most people won't explicitly wait for the process
termination. You can already find such examples inside
test_subprocess.py, Lib/ctypes/util.py (to parse ldconfig's output),
and even in subprocess' documentation:

"""
with Popen(["ifconfig"], stdout=PIPE) as proc:
    log.write(proc.stdout.read())
"""

The problem is that until a child process is wait()ed on or its parent
process terminates (so that the child gets re-parented to init), the
child remains a zombie/defunct process. So if you have long-running
process spawning many subprocesses, you could end up having many
zombie processes, potentially filling up your process table at some
point. And this really sucks.
So I think it could be worth mentioning this somewhere in the
documentation (I mean, if we can find find such leaks inside CPython
code base, then it's definitely something that should be documented).
msg135765 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2011-05-11 08:57
I didn't initially like the idea of __exit__ blocking on another
process... but the zombie issue is real does make me think we should
reconsider this and have it wait().

It is a backwards incompatible change if anyone has started using the
Popen context manager to launch a long running subprocess that they
did not want to wait for.  That should be exceedingly rare.

I say change the behavior to wait() in 3.3, 3.2.1 and 2.7.2.  Keep the
note in the documentation and turn it into something that stands out
better like a note or a warning suggesting that people always call
wait() after the Popen context manager exits if they need to be
compatible with 2.7.1 or earlier.
msg135783 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-11 16:44
I'm re-opening this issue, since Gregory agrees to change the current behaviour.
Patch attached (along with test and documentation update).
msg135814 - (view) Author: Roundup Robot (python-dev) Date: 2011-05-12 04:42
New changeset 7a3f3ad83676 by Gregory P. Smith in branch 'default':
- Issue #12044: Fixed subprocess.Popen when used as a context manager to
http://hg.python.org/cpython/rev/7a3f3ad83676
msg135815 - (view) Author: Roundup Robot (python-dev) Date: 2011-05-12 05:20
New changeset b00a64a5cb93 by Gregory P. Smith in branch '3.2':
merge - 7a3f3ad83676  Fixes Issue #12044.
http://hg.python.org/cpython/rev/b00a64a5cb93
msg135816 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2011-05-12 05:23
did my commits in the reverse order (default before 3.2), oops.  this is fixed.  this wasn't ever in 2.7 so no need for the documentation note.  i'm not worried about adding a note about 3.2.0 vs 3.2.1 beyond the mention in Misc/NEWS as this was new in 3.2.0 and fixing this behavior is a pretty clear bug fix.
msg135841 - (view) Author: Roundup Robot (python-dev) Date: 2011-05-12 14:48
New changeset 361f87c8f36a by Łukasz Langa in branch 'default':
Cleaned up a backward merge after fixes issue #12044.
http://hg.python.org/cpython/rev/361f87c8f36a
msg139904 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-07-05 20:36
See also issue #12494: "subprocess: check_output() doesn't close pipes on error".
History
Date User Action Args
2011-07-05 20:36:58hayposetnosy: + haypo
messages: + msg139904
2011-05-12 14:48:27python-devsetmessages: + msg135841
2011-05-12 05:23:31gregory.p.smithsetstatus: open -> closed
resolution: accepted
messages: + msg135816
2011-05-12 05:20:58python-devsetmessages: + msg135815
2011-05-12 04:42:23python-devsetnosy: + python-dev
messages: + msg135814
2011-05-11 20:12:57gregory.p.smithsetassignee: gregory.p.smith
2011-05-11 16:44:29neologixsetstatus: closed -> open
files: + subprocess_zombie.diff
messages: + msg135783

components: + Library (Lib)
keywords: + patch
resolution: rejected -> (no value)
2011-05-11 08:57:51gregory.p.smithsetmessages: + msg135765
2011-05-10 16:08:26neologixsetmessages: + msg135715
2011-05-10 09:11:45pitrousetstatus: pending -> closed
resolution: rejected
messages: + msg135690
2011-05-10 02:28:20brian.curtinsetstatus: open -> pending

messages: + msg135675
2011-05-09 22:00:40pitrousetmessages: + msg135658
2011-05-09 21:11:27brian.curtinsetmessages: + msg135652
2011-05-09 21:05:14pitrousetmessages: + msg135649
2011-05-09 21:02:25brian.curtinsetmessages: + msg135648
2011-05-09 20:42:15pitrousetmessages: + msg135642
2011-05-09 20:41:02brian.curtinsetmessages: + msg135641
2011-05-09 19:24:14pitroucreate