classification
Title: add pass_fds paramter to subprocess.Popen()
Type: enhancement Stage: test needed
Components: Library (Lib) Versions: Python 3.2
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: amaury.forgeotdarc, ferringb, gregory.p.smith, milko.krachounov, miss-islington, pitrou, serhiy.storchaka, zhigang
Priority: normal Keywords: patch

Created on 2009-07-24 07:56 by zhigang, last changed 2019-09-20 17:01 by miss-islington. This issue is now closed.

Files
File name Uploaded Description Edit
python-subprocess-add-pass-fds.patch zhigang, 2009-07-24 07:56 python-subprocess-add-pass-fds.patch
fd_status.py milko.krachounov, 2010-12-11 14:04 Open fd tester
test_pass_fds.py milko.krachounov, 2010-12-11 14:05
subprocess-pass_fd_fix_example.patch milko.krachounov, 2010-12-11 14:05
Pull Requests
URL Status Linked Edit
PR 16283 merged orivej, 2019-09-19 10:14
Messages (11)
msg90871 - (view) Author: Zhigang Wang (zhigang) Date: 2009-07-24 07:56
The current subprocess.Popen() has a boolean close_fds parameter, which
cannot satisfy all the requirements. Eg. want to pass specific fd to
child process, but close others.

This patch adds a extra parameter pass_fds to subprocess.Popen's
__init__(). This parameter only effect when close_fds=True. When
close_fds=True, all fds in pass_fds will not closed before exec.
msg90872 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-07-24 08:08
A couple of unit tests would be great, as well as a paragraph for the
documentation.
msg90874 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-07-24 08:28
The reason os.closerange() is used is that it can be slow to call
os.close() on lots of fds (I suppose this depends on the OS). See
http://code.python.org/hg/trunk/rev/43caff85ec85

Therefore, the patch should be smart enough to continue using
os.closerange() on ranges of contiguous FDs to be closed.
msg123337 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2010-12-04 11:22
I've committed this feature just in time for 3.2beta1 (so it can't be said i'm adding a feature after the beta ;).  r87026

It still needs tests and documentation.  It doesn't break any existing tests.

I'll take care of that after some sleep.
msg123790 - (view) Author: Milko Krachounov (milko.krachounov) Date: 2010-12-11 14:04
The patch doesn't seem to work.

I added this before closerange in _close_all_but_a_sorted_few_fds:

print("Closing", start_fd, "up to", fd, "exclusive")

And used the attached script to run as a subprocess to check for open fds (taken from my tests patch for issue 7213).

Here's the result:
Python 3.2b1 (py3k:87158M, Dec 11 2010, 02:55:28) 
[GCC 4.4.5] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> import os
>>> import subprocess
>>> subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=False).wait()
0,1,2
0
>>> os.pipe()
(3, 4)
>>> os.pipe()
(5, 6)
>>> subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=False).wait()
0,1,2,3,4,5,6
0
>>> subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=True).wait()
0,1,2
0
>>> subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=True, pass_fds=(6,)).wait()
0,1,2,6
0
>>> subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=True, pass_fds=(3,)).wait()
0,1,2
0
>>> subprocess._posixsubprocess = None
>>> subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=True, pass_fds=(6,)).wait()
Closing 3 up to 6 exclusive
Closing 7 up to 8 exclusive
0,1,2,6
0
>>> subprocess.Popen([sys.executable, 'fd_status.py'], close_fds=True, pass_fds=(3,)).wait()
Closing 3 up to 8 exclusive
0,1,2
0

I also attach a possible test for pass_fds, and an example fix for Python-only implementation. The test requires either my tests patch for issue 7213, or the attached fd_status.py to be put in subprocessdata subdir of Lib/test. The fixed Python implementation passes my test and works fine in the console, I haven't tried the C one. (I don't have a patch for the fix, since it would conflict with the patches for issue 7213.)
msg123938 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2010-12-14 13:44
Bug fix, unittest and documentation added in r87229.  Thanks!
msg123958 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-12-14 16:00
It seems there are (intermittent?) test failures:

======================================================================
FAIL: test_pass_fds (test.test_subprocess.POSIXProcessTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home2/buildbot/slave/3.x.loewis-sun/build/Lib/test/test_subprocess.py", line 1067, in test_pass_fds
    "fd to be closed passed")
AssertionError: {5} is not False : fd to be closed passed

======================================================================
FAIL: test_pass_fds (test.test_subprocess.ProcessTestCasePOSIXPurePython)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home2/buildbot/slave/3.x.loewis-sun/build/Lib/test/test_subprocess.py", line 1067, in test_pass_fds
    "fd to be closed passed")
AssertionError: {5} is not False : fd to be closed passed

(from http://www.python.org/dev/buildbot/all/builders/sparc%20solaris10%20gcc%203.x/builds/2337/steps/test/logs/stdio )
msg151423 - (view) Author: Ferringb (ferringb) * Date: 2012-01-17 02:17
Just noticed this patch... aside from liking the intention, the api for this is going to grow tiresome quick since it expects the FDs to already be in place; is there any reasons a mapping wasn't used here, specifically of (src_fd|src_fileobj) -> target_fd ?

If that was fed in, post fork the client can shuffle around the fd's into appropriate positions- something the parent may not be able to do (threaded environment for example, or async in some respect).

I've had similar functionality in my own process code for a while, and have found it to be generally pretty easy to deal with- for subprocess it has the added benefit that the existing stdin/stdout/stderr bits could be pretty easily folded directly into it.

So... any reason this route wasn't considered, or just wasn't thought about?
msg151424 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-01-17 02:20
Wasn't thought about.  I have seen something similar to that done in another c++ subprocess implementation since.  If you have suggestions for a more useful API, feel free to propose them in a new issue.
msg172876 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-14 12:06
_posixsubprocess.fork_exec docstring was not updated.
msg352872 - (view) Author: miss-islington (miss-islington) Date: 2019-09-20 17:01
New changeset 77abf23c67c1a465a8899666c69f6bcd6930e003 by Miss Islington (bot) (Orivej Desh) in branch 'master':
bpo-6559: Update _posixsubprocess.fork_exec doc (GH-16283)
https://github.com/python/cpython/commit/77abf23c67c1a465a8899666c69f6bcd6930e003
History
Date User Action Args
2019-09-20 17:01:12miss-islingtonsetnosy: + miss-islington
messages: + msg352872
2019-09-19 10:14:40orivejsetpull_requests: + pull_request15868
2012-10-14 12:06:14serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg172876
2012-01-17 02:20:52gregory.p.smithsetmessages: + msg151424
2012-01-17 02:17:07ferringbsetnosy: + ferringb
messages: + msg151423
2010-12-14 16:00:19pitrousetmessages: + msg123958
2010-12-14 13:44:48gregory.p.smithsetstatus: open -> closed
resolution: accepted
messages: + msg123938
2010-12-14 02:49:18r.david.murraysettype: enhancement
title: [PATCH]add pass_fds paramter to subprocess.Popen() -> add pass_fds paramter to subprocess.Popen()
2010-12-11 14:05:33milko.krachounovsetfiles: + subprocess-pass_fd_fix_example.patch
2010-12-11 14:05:13milko.krachounovsetfiles: + test_pass_fds.py
2010-12-11 14:04:53milko.krachounovsetfiles: + fd_status.py
nosy: + milko.krachounov
messages: + msg123790

2010-12-04 11:22:23gregory.p.smithsetassignee: gregory.p.smith

messages: + msg123337
nosy: + gregory.p.smith
2009-07-24 08:28:18pitrousetnosy: + pitrou
messages: + msg90874
2009-07-24 08:08:10amaury.forgeotdarcsetnosy: + amaury.forgeotdarc

messages: + msg90872
stage: test needed
2009-07-24 07:56:35zhigangcreate