| msg135635 - (view) |
Author: Antoine Pitrou (pitrou) *  |
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 ) |
|
|
| msg135641 - (view) |
Author: Brian Curtin (brian.curtin) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2011-05-10 09:11 |
| Well, no, looks ok for me then :) |
|
|
| msg135715 - (view) |
Author: Charles-François Natali (neologix) * |
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) * |
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) * |
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) * |
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 (vstinner) * |
Date: 2011-07-05 20:36 |
| See also issue #12494: "subprocess: check_output() doesn't close pipes on error". |
|
|