Context management support for subprocess.Popen

[merwok]

BPO	10554
Nosy	@merwok, @briancurtin
Files	subprocess.diff subprocess2.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/briancurtin'
closed_at = <Date 2010-12-03.02:50:33.187>
created_at = <Date 2010-11-27.20:47:38.378>
labels = ['easy', 'type-feature', 'library']
title = 'Context management support for subprocess.Popen'
updated_at = <Date 2010-12-03.02:50:33.186>
user = 'https://github.com/merwok'

bugs.python.org fields:

activity = <Date 2010-12-03.02:50:33.186>
actor = 'eric.araujo'
assignee = 'brian.curtin'
closed = True
closed_date = <Date 2010-12-03.02:50:33.187>
closer = 'eric.araujo'
components = ['Library (Lib)']
creation = <Date 2010-11-27.20:47:38.378>
creator = 'eric.araujo'
dependencies = []
files = ['19916', '19917']
hgrepos = []
issue_num = 10554
keywords = ['patch', 'easy']
message_count = 8.0
messages = ['122554', '123155', '123156', '123159', '123160', '123161', '123164', '123165']
nosy_count = 2.0
nosy_names = ['eric.araujo', 'brian.curtin']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue10554'
versions = ['Python 3.2']

[merwok]

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

[briancurtin]

Here's a patch which implements the context manager and adds a few tests and a small doc change.

Tested on Mac and Windows.

[merwok]

Thanks! Patch looks good, tests pass and doc builds.

I have one question and two micro-nitpicks on http://codereview.appspot.com/3441041/

[briancurtin]

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.

[merwok]

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

[merwok]

:keyword:`with`` → :keyword:`with`

[briancurtin]

Committed in r86951. Thanks for the reviews!

[merwok]

“closing any open file descriptors on exit” is exactly the perfect wording I was looking for. Thanks for the patch!