Skip to content

Commit 8edd99d

Browse files
gpsheadgpshead
committed
Issue #6559: fix the subprocess.Popen pass_fds implementation. Add a unittest.
Issue #7213: Change the close_fds default on Windows to better match the new default on POSIX. True when possible (False if stdin/stdout/stderr are supplied). Update the documentation to reflect all of the above.
1 parent 39f34aa commit 8edd99d

5 files changed

Lines changed: 79 additions & 27 deletions

File tree

‎Doc/library/subprocess.rst‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ Using the subprocess Module
2828
This module defines one class called :class:`Popen`:
2929

3030

31-
.. class:: Popen(args, bufsize=0, executable=None, stdin=None, stdout=None, stderr=None, preexec_fn=None, close_fds=_PLATFORM_DEFAULT, shell=False, cwd=None, env=None, universal_newlines=False, startupinfo=None, creationflags=0, restore_signals=True, start_new_session=False)
31+
.. class:: Popen(args, bufsize=0, executable=None, stdin=None, stdout=None, stderr=None, preexec_fn=None, close_fds=True, shell=False, cwd=None, env=None, universal_newlines=False, startupinfo=None, creationflags=0, restore_signals=True, start_new_session=False, pass_fds=())
3232

3333
Arguments are:
3434

@@ -153,14 +153,22 @@ This module defines one class called :class:`Popen`:
153153

154154
If *close_fds* is true, all file descriptors except :const:`0`, :const:`1` and
155155
:const:`2` will be closed before the child process is executed. (Unix only).
156-
The default varies by platform: :const:`False` on Windows and :const:`True`
157-
on POSIX and other platforms.
156+
The default varies by platform: Always true on Unix. On Windows it is
157+
true when *stdin*/*stdout*/*stderr* are :const:`None`, false otherwise.
158158
On Windows, if *close_fds* is true then no handles will be inherited by the
159159
child process. Note that on Windows, you cannot set *close_fds* to true and
160160
also redirect the standard handles by setting *stdin*, *stdout* or *stderr*.
161161

162-
.. versionchanged:: 3.2
163-
The default was changed to True on non Windows platforms.
162+
.. versionchanged:: 3.2
163+
The default for *close_fds* was changed from :const:`False` to
164+
what is described above.
165+
166+
*pass_fds* is an optional sequence of file descriptors to keep open
167+
between the parent and child. Providing any *pass_fds* forces
168+
*close_fds* to be :const:`True`. (Unix only)
169+
170+
.. versionadded:: 3.2
171+
The *pass_fds* parameter was added.
164172

165173
If *cwd* is not ``None``, the child's current directory will be changed to *cwd*
166174
before it is executed. Note that this directory is not considered when

‎Lib/subprocess.py‎

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,10 @@
2727
2828
class Popen(args, bufsize=0, executable=None,
2929
stdin=None, stdout=None, stderr=None,
30-
preexec_fn=None, close_fds=_PLATFORM_DEFAULT, shell=False,
30+
preexec_fn=None, close_fds=True, shell=False,
3131
cwd=None, env=None, universal_newlines=False,
3232
startupinfo=None, creationflags=0,
33-
restore_signals=True, start_new_session=False):
33+
restore_signals=True, start_new_session=False, pass_fds=()):
3434
3535
3636
Arguments are:
@@ -81,8 +81,11 @@ class Popen(args, bufsize=0, executable=None,
8181
8282
If close_fds is true, all file descriptors except 0, 1 and 2 will be
8383
closed before the child process is executed. The default for close_fds
84-
varies by platform: False on Windows and True on all other platforms
85-
such as POSIX.
84+
varies by platform: Always true on POSIX. True when stdin/stdout/stderr
85+
are None on Windows, false otherwise.
86+
87+
pass_fds is an optional sequence of file descriptors to keep open between the
88+
parent and child. Providing any pass_fds implicitly sets close_fds to true.
8689
8790
if shell is true, the specified command will be executed through the
8891
shell.
@@ -621,17 +624,14 @@ def getoutput(cmd):
621624
return getstatusoutput(cmd)[1]
622625

623626

624-
if mswindows:
625-
_PLATFORM_DEFAULT = False
626-
else:
627-
_PLATFORM_DEFAULT = True
627+
_PLATFORM_DEFAULT_CLOSE_FDS = object()
628628

629629

630630
class Popen(object):
631631
def __init__(self, args, bufsize=0, executable=None,
632632
stdin=None, stdout=None, stderr=None,
633-
preexec_fn=None, close_fds=_PLATFORM_DEFAULT, shell=False,
634-
cwd=None, env=None, universal_newlines=False,
633+
preexec_fn=None, close_fds=_PLATFORM_DEFAULT_CLOSE_FDS,
634+
shell=False, cwd=None, env=None, universal_newlines=False,
635635
startupinfo=None, creationflags=0,
636636
restore_signals=True, start_new_session=False,
637637
pass_fds=()):
@@ -648,22 +648,31 @@ def __init__(self, args, bufsize=0, executable=None,
648648
if preexec_fn is not None:
649649
raise ValueError("preexec_fn is not supported on Windows "
650650
"platforms")
651-
if close_fds and (stdin is not None or stdout is not None or
652-
stderr is not None):
653-
raise ValueError("close_fds is not supported on Windows "
654-
"platforms if you redirect stdin/stdout/stderr")
651+
any_stdio_set = (stdin is not None or stdout is not None or
652+
stderr is not None)
653+
if close_fds is _PLATFORM_DEFAULT_CLOSE_FDS:
654+
if any_stdio_set:
655+
close_fds = False
656+
else:
657+
close_fds = True
658+
elif close_fds and any_stdio_set:
659+
raise ValueError(
660+
"close_fds is not supported on Windows platforms"
661+
" if you redirect stdin/stdout/stderr")
655662
else:
656663
# POSIX
664+
if close_fds is _PLATFORM_DEFAULT_CLOSE_FDS:
665+
close_fds = True
666+
if pass_fds and not close_fds:
667+
warnings.warn("pass_fds overriding close_fds.", RuntimeWarning)
668+
close_fds = True
657669
if startupinfo is not None:
658670
raise ValueError("startupinfo is only supported on Windows "
659671
"platforms")
660672
if creationflags != 0:
661673
raise ValueError("creationflags is only supported on Windows "
662674
"platforms")
663675

664-
if pass_fds and not close_fds:
665-
raise ValueError("pass_fds requires close_fds=True.")
666-
667676
self.stdin = None
668677
self.stdout = None
669678
self.stderr = None
@@ -876,7 +885,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
876885
unused_restore_signals, unused_start_new_session):
877886
"""Execute program (MS Windows version)"""
878887

879-
assert not pass_fds, "pass_fds not yet supported on Windows"
888+
assert not pass_fds, "pass_fds not supported on Windows."
880889

881890
if not isinstance(args, str):
882891
args = list2cmdline(args)
@@ -1091,7 +1100,7 @@ def _close_all_but_a_sorted_few_fds(self, fds_to_keep):
10911100
# precondition: fds_to_keep must be sorted and unique
10921101
start_fd = 3
10931102
for fd in fds_to_keep:
1094-
if fd > start_fd:
1103+
if fd >= start_fd:
10951104
os.closerange(start_fd, fd)
10961105
start_fd = fd + 1
10971106
if start_fd <= MAXFD:

‎Lib/test/test_subprocess.py‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,6 +1042,37 @@ def test_close_fds(self):
10421042
"Some fds were left open")
10431043
self.assertIn(1, remaining_fds, "Subprocess failed")
10441044

1045+
def test_pass_fds(self):
1046+
fd_status = support.findfile("fd_status.py", subdir="subprocessdata")
1047+
1048+
open_fds = set()
1049+
1050+
for x in range(5):
1051+
fds = os.pipe()
1052+
self.addCleanup(os.close, fds[0])
1053+
self.addCleanup(os.close, fds[1])
1054+
open_fds.update(fds)
1055+
1056+
for fd in open_fds:
1057+
p = subprocess.Popen([sys.executable, fd_status],
1058+
stdout=subprocess.PIPE, close_fds=True,
1059+
pass_fds=(fd, ))
1060+
output, ignored = p.communicate()
1061+
1062+
remaining_fds = set(map(int, output.split(b',')))
1063+
to_be_closed = open_fds - {fd}
1064+
1065+
self.assertIn(fd, remaining_fds, "fd to be passed not passed")
1066+
self.assertFalse(remaining_fds & to_be_closed,
1067+
"fd to be closed passed")
1068+
1069+
# pass_fds overrides close_fds with a warning.
1070+
with self.assertWarns(RuntimeWarning) as context:
1071+
self.assertFalse(subprocess.call(
1072+
[sys.executable, "-c", "import sys; sys.exit(0)"],
1073+
close_fds=False, pass_fds=(fd, )))
1074+
self.assertIn('overriding close_fds', str(context.warning))
1075+
10451076

10461077
@unittest.skipUnless(mswindows, "Windows specific tests")
10471078
class Win32ProcessTestCase(BaseTestCase):

‎Misc/NEWS‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,12 @@ Library
2323
- Issue #10107: Warn about unsaved files in IDLE on OSX.
2424

2525
- Issue #7213: subprocess.Popen's default for close_fds has been changed.
26-
It is now platform specific, keeping its default of False on Windows and
27-
changing the default to True on POSIX and other platforms.
26+
It is now True in most cases other than on Windows when input, output or
27+
error handles are provided.
28+
29+
- Issue #6559: subprocess.Popen has a new pass_fds parameter (actually
30+
added in 3.2beta1) to allow specifying a specific list of file descriptors
31+
to keep open in the child process.
2832

2933

3034
What's New in Python 3.2 Beta 1?

‎Modules/_posixsubprocess.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ static void child_exec(char *const exec_array[],
107107
errno = 0; /* We don't want to report an OSError. */
108108
goto error;
109109
}
110-
if (keep_fd <= start_fd)
110+
if (keep_fd < start_fd)
111111
continue;
112112
for (fd_num = start_fd; fd_num < keep_fd; ++fd_num) {
113113
close(fd_num);

0 commit comments

Comments
 (0)