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 @@ if 'TimeoutExpired' not in globals():
                     (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,41 +1151,74 @@ class Popen(object):
             self._set_cloexec_flag(w)
             return r, w
 
-        def _close_fds(self, keep):
+        _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:
+                for i in keep:
+                    if i == errpipe_write:
+                        continue
+                    _set_inheritable(i, True)
+
+                for fd in fds:
+                    if fd in keep or fd < 3:
+                        continue
+                    try:
+                        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
-            if hasattr(os, 'set_inheritable'):
-                set_inheritable = os.set_inheritable
-            else:
-                set_inheritable = lambda i, v: True
-            if hasattr(os, 'closerange'):
+
+            # 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)
+                    _set_inheritable(i, True)
                     continue
 
                 try:
                     os.close(i)
                 except:
                     pass
-            else:
-                for i in xrange(3, MAXFD):
-                    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,
@@ -1235,6 +1274,13 @@ class Popen(object):
                         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 @@ class Popen(object):
                                 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 @@ class Popen(object):
                             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 @@ BaseTestCase = unittest.TestCase
 
 from greentest.flaky import reraiseFlakyTestTimeout
 from greentest.flaky import reraiseFlakyTestRaceCondition
+
+try:
+    from unittest import mock
+except ImportError: # Python 2
+    import mock
+
+mock = mock

src/greentest/test__subprocess.py

 import sys
 import os
 import errno
+import unittest
+
+import time
+import gc
+import tempfile
+
 import greentest
 import gevent
+from greentest import mock
 from gevent import subprocess
 
 if not hasattr(subprocess, 'mswindows'):
     # PyPy3, native python subprocess
     subprocess.mswindows = False
-import time
-import gc
-import tempfile
 
 
 PYPY = hasattr(sys, 'pypy_version_info')
@@ -236,8 +240,8 @@ class Test(greentest.TestCase):
         r = p.stdout.readline()
         self.assertEqual(r, b'foobar\n')
 
-    if sys.platform != 'win32':
-
+    @greentest.ignores_leakcheck
+    @greentest.skipOnWindows("Not sure why?")
     def test_subprocess_in_native_thread(self):
         # gevent.subprocess doesn't work from a background
         # native thread. See #688
@@ -261,7 +265,6 @@ class Test(greentest.TestCase):
         self.assertTrue(isinstance(ex[0], TypeError), ex)
         self.assertEqual(ex[0].args[0], 'child watchers are only available on the default loop')
 
-        test_subprocess_in_native_thread.ignore_leakcheck = True
 
     @greentest.skipOnLibuvOnPyPyOnWin("hangs")
     def __test_no_output(self, kwargs, kind):
@@ -295,6 +298,91 @@ class Test(greentest.TestCase):
         # https://github.com/gevent/gevent/pull/939
         self.__test_no_output({}, bytes)
 
+@greentest.skipOnWindows("Testing POSIX fd closing")
+class TestFDs(unittest.TestCase):
+
+    @mock.patch('os.closerange')
+    @mock.patch('gevent.subprocess._set_inheritable')
+    @mock.patch('os.close')
+    def test_close_fds_brute_force(self, close, set_inheritable, closerange):
+        keep = (
+            4, 5,
+            # Leave a hole
+            # 6,
+            7,
+        )
+        subprocess.Popen._close_fds_brute_force(keep, None)
+
+        closerange.assert_has_calls([
+            mock.call(3, 4),
+            mock.call(8, subprocess.MAXFD),
+        ])
+
+        set_inheritable.assert_has_calls([
+            mock.call(4, True),
+            mock.call(5, True),
+        ])
+
+        close.assert_called_once_with(6)
+
+    @mock.patch('gevent.subprocess.Popen._close_fds_brute_force')
+    @mock.patch('os.listdir')
+    def test_close_fds_from_path_bad_values(self, listdir, brute_force):
+        listdir.return_value = 'Not an Integer'
+
+        subprocess.Popen._close_fds_from_path('path', [], 42)
+        brute_force.assert_called_once_with([], 42)
+
+    @mock.patch('os.listdir')
+    @mock.patch('os.closerange')
+    @mock.patch('gevent.subprocess._set_inheritable')
+    @mock.patch('os.close')
+    def test_close_fds_from_path(self, close, set_inheritable, closerange, listdir):
+        keep = (
+            4, 5,
+            # Leave a hole
+            # 6,
+            7,
+        )
+        listdir.return_value = ['1', '6', '37']
+
+        subprocess.Popen._close_fds_from_path('path', keep, 5)
+
+        self.assertEqual([], closerange.mock_calls)
+
+        set_inheritable.assert_has_calls([
+            mock.call(4, True),
+            mock.call(7, True),
+        ])
+
+        close.assert_has_calls([
+            mock.call(6),
+            mock.call(37),
+        ])
+
+    @mock.patch('gevent.subprocess.Popen._close_fds_brute_force')
+    @mock.patch('os.path.isdir')
+    def test_close_fds_no_dir(self, isdir, brute_force):
+        isdir.return_value = False
+
+        subprocess.Popen._close_fds([], 42)
+        brute_force.assert_called_once_with([], 42)
+        isdir.assert_has_calls([
+            mock.call('/proc/self/fd'),
+            mock.call('/dev/fd'),
+        ])
+
+    @mock.patch('gevent.subprocess.Popen._close_fds_from_path')
+    @mock.patch('gevent.subprocess.Popen._close_fds_brute_force')
+    @mock.patch('os.path.isdir')
+    def test_close_fds_with_dir(self, isdir, brute_force, from_path):
+        isdir.return_value = True
+
+        subprocess.Popen._close_fds([7], 42)
+
+        self.assertEqual([], brute_force.mock_calls)
+        from_path.assert_called_once_with('/proc/self/fd', [7], 42)
+
 class RunFuncTestCase(greentest.TestCase):
     # Based on code from python 3.6