[Python-checkins] bpo-35537: subprocess can use posix_spawn with pipes (GH-11575)

[Victor Stinner]

https://github.com/python/cpython/commit/f6243ac1e4828299fe5a8e943d7bd41cab1f34cd
commit: f6243ac1e4828299fe5a8e943d7bd41cab1f34cd
branch: master
author: Victor Stinner <vstinner at redhat.com>
committer: GitHub <noreply at github.com>
date: 2019-01-23T19:00:39+01:00
summary:

bpo-35537: subprocess can use posix_spawn with pipes (GH-11575)

* subprocess.Popen can now also use os.posix_spawn() with pipes,
  but only if pipe file descriptors are greater than 2.
* Fix Popen._posix_spawn(): set '_child_created' attribute to True.
* Add Popen._close_pipe_fds() helper function to factorize the code.

files:
M Doc/whatsnew/3.8.rst
M Lib/subprocess.py

diff --git a/Doc/whatsnew/3.8.rst b/Doc/whatsnew/3.8.rst
index 8b38cce35368..ce46afd48158 100644
--- a/Doc/whatsnew/3.8.rst
+++ b/Doc/whatsnew/3.8.rst
@@ -280,8 +280,8 @@ Optimizations
   and Linux (using glibc 2.24 or newer) if all these conditions are met:
 
   * *close_fds* is false;
-  * *preexec_fn*, *pass_fds*, *cwd*, *stdin*, *stdout*, *stderr* and
-    *start_new_session* parameters are not set;
+  * *preexec_fn*, *pass_fds*, *cwd* and *start_new_session* parameters
+    are not set;
   * the *executable* path contains a directory.
 
 * :func:`shutil.copyfile`, :func:`shutil.copy`, :func:`shutil.copy2`,
diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index b94575b8401e..2300c7352e0d 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -1066,6 +1066,34 @@ def wait(self, timeout=None):
                 pass
             raise  # resume the KeyboardInterrupt
 
+    def _close_pipe_fds(self,
+                        p2cread, p2cwrite,
+                        c2pread, c2pwrite,
+                        errread, errwrite):
+        # self._devnull is not always defined.
+        devnull_fd = getattr(self, '_devnull', None)
+
+        if _mswindows:
+            if p2cread != -1:
+                p2cread.Close()
+            if c2pwrite != -1:
+                c2pwrite.Close()
+            if errwrite != -1:
+                errwrite.Close()
+        else:
+            if p2cread != -1 and p2cwrite != -1 and p2cread != devnull_fd:
+                os.close(p2cread)
+            if c2pwrite != -1 and c2pread != -1 and c2pwrite != devnull_fd:
+                os.close(c2pwrite)
+            if errwrite != -1 and errread != -1 and errwrite != devnull_fd:
+                os.close(errwrite)
+
+        if devnull_fd is not None:
+            os.close(devnull_fd)
+
+        # Prevent a double close of these handles/fds from __init__ on error.
+        self._closed_child_pipe_fds = True
+
 
     if _mswindows:
         #
@@ -1244,17 +1272,9 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
                 # output pipe are maintained in this process or else the
                 # pipe will not close when the child process exits and the
                 # ReadFile will hang.
-                if p2cread != -1:
-                    p2cread.Close()
-                if c2pwrite != -1:
-                    c2pwrite.Close()
-                if errwrite != -1:
-                    errwrite.Close()
-                if hasattr(self, '_devnull'):
-                    os.close(self._devnull)
-                # Prevent a double close of these handles/fds from __init__
-                # on error.
-                self._closed_child_pipe_fds = True
+                self._close_pipe_fds(p2cread, p2cwrite,
+                                     c2pread, c2pwrite,
+                                     errread, errwrite)
 
             # Retain the process handle, but close the thread handle
             self._child_created = True
@@ -1441,7 +1461,10 @@ def _get_handles(self, stdin, stdout, stderr):
                     errread, errwrite)
 
 
-        def _posix_spawn(self, args, executable, env, restore_signals):
+        def _posix_spawn(self, args, executable, env, restore_signals,
+                         p2cread, p2cwrite,
+                         c2pread, c2pwrite,
+                         errread, errwrite):
             """Execute program using os.posix_spawn()."""
             if env is None:
                 env = os.environ
@@ -1456,7 +1479,26 @@ def _posix_spawn(self, args, executable, env, restore_signals):
                         sigset.append(signum)
                 kwargs['setsigdef'] = sigset
 
+            file_actions = []
+            for fd in (p2cwrite, c2pread, errread):
+                if fd != -1:
+                    file_actions.append((os.POSIX_SPAWN_CLOSE, fd))
+            for fd, fd2 in (
+                (p2cread, 0),
+                (c2pwrite, 1),
+                (errwrite, 2),
+            ):
+                if fd != -1:
+                    file_actions.append((os.POSIX_SPAWN_DUP2, fd, fd2))
+            if file_actions:
+                kwargs['file_actions'] = file_actions
+
             self.pid = os.posix_spawn(executable, args, env, **kwargs)
+            self._child_created = True
+
+            self._close_pipe_fds(p2cread, p2cwrite,
+                                 c2pread, c2pwrite,
+                                 errread, errwrite)
 
         def _execute_child(self, args, executable, preexec_fn, close_fds,
                            pass_fds, cwd, env,
@@ -1489,11 +1531,14 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
                     and not close_fds
                     and not pass_fds
                     and cwd is None
-                    and p2cread == p2cwrite == -1
-                    and c2pread == c2pwrite == -1
-                    and errread == errwrite == -1
+                    and (p2cread == -1 or p2cread > 2)
+                    and (c2pwrite == -1 or c2pwrite > 2)
+                    and (errwrite == -1 or errwrite > 2)
                     and not start_new_session):
-                self._posix_spawn(args, executable, env, restore_signals)
+                self._posix_spawn(args, executable, env, restore_signals,
+                                  p2cread, p2cwrite,
+                                  c2pread, c2pwrite,
+                                  errread, errwrite)
                 return
 
             orig_executable = executable
@@ -1548,18 +1593,9 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
                     # be sure the FD is closed no matter what
                     os.close(errpipe_write)
 
-                # self._devnull is not always defined.
-                devnull_fd = getattr(self, '_devnull', None)
-                if p2cread != -1 and p2cwrite != -1 and p2cread != devnull_fd:
-                    os.close(p2cread)
-                if c2pwrite != -1 and c2pread != -1 and c2pwrite != devnull_fd:
-                    os.close(c2pwrite)
-                if errwrite != -1 and errread != -1 and errwrite != devnull_fd:
-                    os.close(errwrite)
-                if devnull_fd is not None:
-                    os.close(devnull_fd)
-                # Prevent a double close of these fds from __init__ on error.
-                self._closed_child_pipe_fds = True
+                self._close_pipe_fds(p2cread, p2cwrite,
+                                     c2pread, c2pwrite,
+                                     errread, errwrite)
 
                 # Wait for exec to fail or succeed; possibly raising an
                 # exception (limited in size)