Merge pull request #1176 from gevent/issue1172

Use /dev/fd|/proc/self/fd to get open FDs to close in Popen

CHANGES.rst

@@ -10,6 +10,18 @@
 - On Windows, CFFI is now a dependency so that the libuv backend
   really can be used by default.
 
+- Fix a bug detecting whether we can use the memory monitoring
+  features when psutil is not installed.
+
+- `gevent.subprocess.Popen` uses ``/proc/self/fd`` (on Linux) or
+  ``/dev/fd`` (on BSD, including macOS) to find the file descriptors
+  to close when ``close_fds`` is true. This matches an optimization
+  added to Python 3 (and backports it to Python 2.7), making process
+  spawning up to 9 times faster. Also, on Python 3, since Python 3.3
+  is no longer supported, we can also optimize the case where
+  ``close_fds`` is false (not the default), making process spawning up
+  to 38 times faster. Initially reported in :issue:`1172` by Ofer Koren.
+
 
 1.3b1 (2018-04-13)
 ==================

Makefile

@@ -179,7 +179,7 @@ develop:
 # disables the cache.
 # We need wheel>=0.26 on Python 3.5. See previous revisions.
 	time ${PYTHON} -m pip install -U -r ci-requirements.txt
-	GEVENTSETUP_EV_VERIFY=3 time ${PYTHON} -m pip install -U -e .
+	GEVENTSETUP_EV_VERIFY=3 time ${PYTHON} -m pip install -U -e .[test,dnspython,events]
 	${PYTHON} -m pip freeze
 	ccache -s
 	@${PYTHON} scripts/travis.py fold_end install

appveyor.yml

@@ -142,7 +142,7 @@ build_script:
   # into a variable, using python to glob.
   - "%PYEXE% -c \"import glob; print(glob.glob('dist/*whl')[0])\" > whl.txt"
   - set /p PYWHL=<whl.txt
-  - "%PYEXE% -m pip install %PYWHL%"
+  - "%PYEXE% -m pip install %PYWHL%[test,events,dnspython]"
 
 test_script:
   # Run the project tests

ci-requirements.txt

@@ -12,23 +12,8 @@ greenlet>=0.4.13 ; platform_python_implementation == "CPython"
 pylint>=1.8.0
 # pyyaml is included here and doesn't install on travis with 3.7a3
 prospector[with_pyroma] ; python_version < '3.7'
-# We don't run coverage on Windows, and pypy can't build it there
-# anyway (coveralls -> cryptopgraphy -> openssl)
-coverage>=4.0 ; sys_platform != 'win32'
-coveralls>=1.0 ; sys_platform != 'win32'
 # See version requirements in setup.py
 cffi >= 1.11.5 ; platform_python_implementation == "CPython"
-futures
-dnspython
-idna
-# Makes tests faster
-# Fails to build on PyPy on Windows.
-psutil ; platform_python_implementation == "CPython" or sys_platform != "win32"
+
 # benchmarks use this
 perf
-# examples, called from tests, use this
-requests
-
-# Events
-zope.event
-zope.interface

dev-requirements.txt

@@ -4,5 +4,4 @@
 restview
 
 -r ci-requirements.txt
--r rtd-requirements.txt
--e .
+-e .[test,events,dnspython,doc]

setup.py

@@ -327,6 +327,27 @@ def run_setup(ext_modules, run_make):
                 'zope.event',
                 'zope.interface',
             ],
+            'doc': [
+                'repoze.sphinx.autointerface',
+            ],
+            'test': [
+                'zope.interface',
+                'zope.event',
+
+                # Makes tests faster
+                # Fails to build on PyPy on Windows.
+                'psutil ; platform_python_implementation == "CPython" or sys_platform != "win32"',
+                # examples, called from tests, use this
+                'requests',
+
+                # We don't run coverage on Windows, and pypy can't build it there
+                # anyway (coveralls -> cryptopgraphy -> openssl)
+                'coverage>=4.0 ; sys_platform != "win32"',
+                'coveralls>=1.0 ; sys_platform != "win32"',
+
+                'futures ; python_version == "2.7"',
+                'mock ; python_version == "2.7"',
+            ]
         },
         # It's always safe to pass the CFFI keyword, even if
         # cffi is not installed: it's just ignored in that case.

src/gevent/subprocess.py

@@ -379,6 +379,12 @@ def __str__(self):
                     (self.cmd, self.timeout))
 
 
+if hasattr(os, 'set_inheritable'):
+    _set_inheritable = os.set_inheritable
+else:
+    _set_inheritable = lambda i, v: True
+
+
 class Popen(object):
     """
     The underlying process creation and management in this module is
@@ -1145,42 +1151,75 @@ def pipe_cloexec(self):
             self._set_cloexec_flag(w)
             return r, w
 
-        def _close_fds(self, keep):
-            # `keep` is a set of fds, so we
-            # use os.closerange from 3 to min(keep)
-            # and then from max(keep + 1) to MAXFD and
-            # loop through filling in the gaps.
-            # Under new python versions, we need to explicitly set
-            # passed fds to be inheritable or they will go away on exec
-            if hasattr(os, 'set_inheritable'):
-                set_inheritable = os.set_inheritable
+        _POSSIBLE_FD_DIRS = (
+            '/proc/self/fd', # Linux
+            '/dev/fd', # BSD, including macOS
+        )
+
+        @classmethod
+        def _close_fds(cls, keep, errpipe_write):
+            # From the C code:
+            # errpipe_write is part of keep. It must be closed at
+            # exec(), but kept open in the child process until exec() is
+            # called.
+            for path in cls._POSSIBLE_FD_DIRS:
+                if os.path.isdir(path):
+                    return cls._close_fds_from_path(path, keep, errpipe_write)
+            return cls._close_fds_brute_force(keep, errpipe_write)
+
+        @classmethod
+        def _close_fds_from_path(cls, path, keep, errpipe_write):
+            # path names a directory whose only entries have
+            # names that are ascii strings of integers in base10,
+            # corresponding to the fds the current process has open
+            try:
+                fds = [int(fname) for fname in os.listdir(path)]
+            except (ValueError, OSError):
+                cls._close_fds_brute_force(keep, errpipe_write)
             else:
-                set_inheritable = lambda i, v: True
-            if hasattr(os, 'closerange'):
-                keep = sorted(keep)
-                min_keep = min(keep)
-                max_keep = max(keep)
-                os.closerange(3, min_keep)
-                os.closerange(max_keep + 1, MAXFD)
-                for i in xrange(min_keep, max_keep):
-                    if i in keep:
-                        set_inheritable(i, True)
+                for i in keep:
+                    if i == errpipe_write:
                         continue
+                    _set_inheritable(i, True)
 
-                    try:
-                        os.close(i)
-                    except:
-                        pass
-            else:
-                for i in xrange(3, MAXFD):
-                    if i in keep:
-                        set_inheritable(i, True)
+                for fd in fds:
+                    if fd in keep or fd < 3:
                         continue
                     try:
-                        os.close(i)
+                        os.close(fd)
                     except:
                         pass
 
+        @classmethod
+        def _close_fds_brute_force(cls, keep, errpipe_write):
+            # `keep` is a set of fds, so we
+            # use os.closerange from 3 to min(keep)
+            # and then from max(keep + 1) to MAXFD and
+            # loop through filling in the gaps.
+
+            # Under new python versions, we need to explicitly set
+            # passed fds to be inheritable or they will go away on exec
+
+            # XXX: Bug: We implicitly rely on errpipe_write being the largest open
+            # FD so that we don't change its cloexec flag.
+
+            assert hasattr(os, 'closerange') # Added in 2.7
+            keep = sorted(keep)
+            min_keep = min(keep)
+            max_keep = max(keep)
+            os.closerange(3, min_keep)
+            os.closerange(max_keep + 1, MAXFD)
+
+            for i in xrange(min_keep, max_keep):
+                if i in keep:
+                    _set_inheritable(i, True)
+                    continue
+
+                try:
+                    os.close(i)
+                except:
+                    pass
+
         def _execute_child(self, args, executable, preexec_fn, close_fds,
                            pass_fds, cwd, env, universal_newlines,
                            startupinfo, creationflags, shell,
@@ -1235,6 +1274,13 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
                         raise
                     if self.pid == 0:
                         # Child
+
+                        # XXX: Technically we're doing a lot of stuff here that
+                        # may not be safe to do before a exec(), depending on the OS.
+                        # CPython 3 goes to great lengths to precompute a lot
+                        # of this info before the fork and pass it all to C functions that
+                        # try hard not to call things like malloc(). (Of course,
+                        # CPython 2 pretty much did what we're doing.)
                         try:
                             # Close parent's pipe ends
                             if p2cwrite != -1:
@@ -1254,16 +1300,16 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
                                 errwrite = os.dup(errwrite)
 
                             # Dup fds for child
-                            def _dup2(a, b):
+                            def _dup2(existing, desired):
                                 # dup2() removes the CLOEXEC flag but
                                 # we must do it ourselves if dup2()
                                 # would be a no-op (issue #10806).
-                                if a == b:
-                                    self._set_cloexec_flag(a, False)
-                                elif a != -1:
-                                    os.dup2(a, b)
+                                if existing == desired:
+                                    self._set_cloexec_flag(existing, False)
+                                elif existing != -1:
+                                    os.dup2(existing, desired)
                                 try:
-                                    self._remove_nonblock_flag(b)
+                                    self._remove_nonblock_flag(desired)
                                 except OSError:
                                     # Ignore EBADF, it may not actually be
                                     # open yet.
@@ -1296,21 +1342,7 @@ def _dup2(a, b):
                             if close_fds:
                                 fds_to_keep = set(pass_fds)
                                 fds_to_keep.add(errpipe_write)
-                                self._close_fds(fds_to_keep)
-                            elif hasattr(os, 'get_inheritable'):
-                                # close_fds was false, and we're on
-                                # Python 3.4 or newer, so "all file
-                                # descriptors except standard streams
-                                # are closed, and inheritable handles
-                                # are only inherited if the close_fds
-                                # parameter is False."
-                                for i in xrange(3, MAXFD):
-                                    try:
-                                        if i == errpipe_write or os.get_inheritable(i):
-                                            continue
-                                        os.close(i)
-                                    except:
-                                        pass
+                                self._close_fds(fds_to_keep, errpipe_write)
 
                             if restore_signals:
                                 # restore the documented signals back to sig_dfl;

src/greentest/greentest/__init__.py

@@ -119,3 +119,10 @@
 
 from greentest.flaky import reraiseFlakyTestTimeout
 from greentest.flaky import reraiseFlakyTestRaceCondition
+
+try:
+    from unittest import mock
+except ImportError: # Python 2
+    import mock
+
+mock = mock