homepage

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)

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:

> 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?

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.

> 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.

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?

> 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 :)

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?

Well, no, looks ok for me then :)

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).

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.

I'm re-opening this issue, since Gregory agrees to change the current behaviour.
Patch attached (along with test and documentation update).

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 

New changeset b00a64a5cb93 by Gregory P. Smith in branch '3.2':
merge - 7a3f3ad83676 Fixes Issue #12044.
http://hg.python.org/cpython/rev/b00a64a5cb93 

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.

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 

See also issue #12494: "subprocess: check_output() doesn't close pipes on error".