classification
Title: Context management support for subprocess.Popen
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brian.curtin Nosy List: brian.curtin, eric.araujo
Priority: normal Keywords: easy, patch

Created on 2010-11-27 20:47 by eric.araujo, last changed 2010-12-03 02:50 by eric.araujo. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess.diff brian.curtin, 2010-12-03 01:43
subprocess2.diff brian.curtin, 2010-12-03 02:12
Messages (8)
msg122554 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-27 20:47
I propose that __enter__ and __exit__ be added to subprocess.Popen.  __enter__ returns self, __exit__ closes open file descriptors.

__exit__ could also do the same checks that __del__ does (and which I don’t entirely understand.

See also os.popen (os._wrap_close).
msg123155 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-03 01:43
Here's a patch which implements the context manager and adds a few tests and a small doc change.

Tested on Mac and Windows.
msg123156 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-12-03 01:57
Thanks!  Patch looks good, tests pass and doc builds.

I have one question and two micro-nitpicks on http://codereview.appspot.com/3441041/
msg123159 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-03 02:12
I updated the doc to be much more simple. I got used to sys.executable based tests :) New patch attached.

As for __del__, I think it should do it's thing, and the exit will do it's own. Context managers are traditionally used on file-based things, or things that can be opened or closed. Creating a Popen object will open one or more pipes, so we should just close those opened pipes.
msg123160 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-12-03 02:21
New doc looks good. Suggested changes: make with a link, explaing why it’s a context manager.

-   Popen objects are supported as context managers via the ``with`` statement.
+   Popen objects are supported as context managers via the :keyword:`with`` statement, so that file descriptors are closed cleanly.

I agree with your view on __del__ vs. __exit__.
msg123161 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-12-03 02:23
:keyword:`with`` → :keyword:`with`
msg123164 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-03 02:47
Committed in r86951. Thanks for the reviews!
msg123165 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-12-03 02:50
“closing any open file descriptors on exit” is exactly the perfect wording I was looking for.  Thanks for the patch!
History
Date User Action Args
2010-12-03 02:50:33eric.araujosetstatus: open -> closed

messages: + msg123165
2010-12-03 02:47:17brian.curtinsetassignee: brian.curtin
resolution: fixed
messages: + msg123164
stage: needs patch -> resolved
2010-12-03 02:24:46belopolskysettitle: Context managerment support for subprocess.Popen -> Context management support for subprocess.Popen
2010-12-03 02:23:07eric.araujosetmessages: + msg123161
2010-12-03 02:21:15eric.araujosetmessages: + msg123160
2010-12-03 02:12:13brian.curtinsetfiles: + subprocess2.diff

messages: + msg123159
2010-12-03 01:57:58eric.araujosetmessages: + msg123156
2010-12-03 01:43:19brian.curtinsetfiles: + subprocess.diff

nosy: + brian.curtin
messages: + msg123155

keywords: + patch
2010-11-27 20:47:38eric.araujocreate