Fix Popen.communicate() to raise exceptions from reading the streams.

And a general clean up of how the streams are read, ensuring we just have one greenlet. This avoids the 'reentrant call' errors that could prevent closing the streams on time.

Fixes #1510

CHANGES.rst

@@ -39,7 +39,21 @@ Library and Dependency Updates
   with debugging. The event libraries allocate small amounts of memory
   at startup. The allocation functions have to take the GIL, but
   because of the limited amount of actual allocation that gets done
-  this is not expected to be a concern.
+  this is not expected to be a bottleneck.
+
+Other
+-----
+
+- Make `gevent.subprocess.Popen.communicate` raise exceptions raised
+  by reading from the process, like the standard library. In
+  particular, under Python 3, if the process output is being decoded
+  as text, this can now raise ``UnicodeDecodeError``. Reported in
+  :issue:`1510` by Ofer Koren.
+
+- Make `gevent.subprocess.Popen.communicate` be more careful about
+  closing files. Previously if a timeout error happened, a second call
+  to ``communicate`` might not close the pipe.
+
 
 1.5a3 (2020-01-01)
 ==================

src/gevent/_fileobjectcommon.py

@@ -371,7 +371,13 @@ class FileObjectBase(object):
         return getattr(self._io, name)
 
     def __repr__(self):
-        return '<%s _fobj=%r%s>' % (self.__class__.__name__, self.io, self._extra_repr())
+        return '<%s at 0x%x %s_fobj=%r%s>' % (
+            self.__class__.__name__,
+            id(self),
+            'closed' if self.closed else '',
+            self.io,
+            self._extra_repr()
+        )
 
     def _extra_repr(self):
         return ''

src/gevent/testing/__init__.py

@@ -85,6 +85,7 @@ from .skipping import skipOnPurePython
 from .skipping import skipWithCExtensions
 from .skipping import skipOnLibuvOnTravisOnCPython27
 from .skipping import skipOnPy37
+from .skipping import skipOnPy3
 from .skipping import skipWithoutResource
 from .skipping import skipWithoutExternalNetwork
 from .skipping import skipOnPy2

src/gevent/testing/openfiles.py

@@ -22,16 +22,52 @@ from __future__ import absolute_import, print_function, division
 import os
 import unittest
 import re
+import gc
+import functools
 
 from . import sysinfo
 
-# Linux/OS X/BSD platforms can implement this by calling out to lsof
+# Linux/OS X/BSD platforms /can/ implement this by calling out to lsof.
+# However, if psutil is available (it is cross-platform) use that.
+# It is *much* faster than shelling out to lsof each time
+# (Running 14 tests takes 3.964s with lsof and 0.046 with psutil)
+# However, it still doesn't completely solve the issue on Windows: fds are reported
+# as -1 there, so we can't fully check those.
+
+def _collects(func):
+    # We've seen OSError: No such file or directory /proc/PID/fd/NUM.
+    # This occurs in the loop that checks open files. It first does
+    # listdir() and then tries readlink() on each file. But the file
+    # went away. This must be because of async GC in PyPy running
+    # destructors at arbitrary times. This became an issue in PyPy 7.2
+    # but could theoretically be an issue with any objects caught in a
+    # cycle. This is one reason we GC before we begin. (The other is
+    # to clean up outstanding objects that will close files in
+    # __del__.)
+    #
+    # Note that this can hide errors, though, by causing greenlets to get
+    # collected and drop references and thus close files. We should be deterministic
+    # and careful about closing things.
+    @functools.wraps(func)
+    def f():
+        gc.collect()
+        gc.collect()
+        enabled = gc.isenabled()
+        gc.disable()
+
+        try:
+            return func()
+        finally:
+            if enabled:
+                gc.enable()
+    return f
 
 
 if sysinfo.WIN:
     def _run_lsof():
         raise unittest.SkipTest("lsof not expected on Windows")
 else:
+    @_collects
     def _run_lsof():
         import tempfile
         pid = os.getpid()
@@ -70,6 +106,7 @@ def default_get_open_files(pipes=False):
     results['data'] = data
     return results
 
+@_collects
 def default_get_number_open_files():
     if os.path.exists('/proc/'):
         # Linux only
@@ -91,12 +128,8 @@ except ImportError:
     get_open_files = default_get_open_files
     get_number_open_files = default_get_number_open_files
 else:
-    # If psutil is available (it is cross-platform) use that.
-    # It is *much* faster than shelling out to lsof each time
-    # (Running 14 tests takes 3.964s with lsof and 0.046 with psutil)
-    # However, it still doesn't completely solve the issue on Windows: fds are reported
-    # as -1 there, so we can't fully check those.
 
+    @_collects
     def get_open_files():
         """
         Return a list of popenfile and pconn objects.
@@ -108,37 +141,26 @@ else:
            (socket.listen(1)). Unlike the lsof implementation, this will only
            return sockets in a state like that.
         """
-        # We've seen OSError: No such file or directory
-        # /proc/PID/fd/NUM. This occurs in the loop that checks open
-        # files. It first does listdir() and then tries readlink() on
-        # each file. But the file went away. This must be because of
-        # async GC in PyPy running destructors at arbitrary times.
-        # This became an issue in PyPy 7.2 but could theoretically be
-        # an issue with any objects caught in a cycle. Try to clean
-        # that up before we begin.
-        import gc
-        gc.collect()
-        gc.collect()
+
         results = dict()
-        gc.disable()
-        try:
-            for _ in range(3):
-                try:
-                    process = psutil.Process()
-                    results['data'] = process.open_files() + process.connections('all')
-                    break
-                except OSError:
-                    pass
-            else:
-                # No break executed
-                raise unittest.SkipTest("Unable to read open files")
-        finally:
-            gc.enable()
+
+        for _ in range(3):
+            try:
+                process = psutil.Process()
+                results['data'] = process.open_files() + process.connections('all')
+                break
+            except OSError:
+                pass
+        else:
+            # No break executed
+            raise unittest.SkipTest("Unable to read open files")
+
         for x in results['data']:
             results[x.fd] = x
         results['data'] += ['From psutil', process]
         return results
 
+    @_collects
     def get_number_open_files():
         process = psutil.Process()
         try:
@@ -146,3 +168,28 @@ else:
         except AttributeError:
             # num_fds is unix only. Is num_handles close enough on Windows?
             return 0
+
+
+
+class DoesNotLeakFilesMixin(object): # pragma: no cover
+    """
+    A test case mixin that helps find a method that's leaking an
+    open file.
+
+    Only mix this in when needed to debug, it slows tests down.
+    """
+    def setUp(self):
+        self.__open_files_count = get_number_open_files()
+        super(DoesNotLeakFilesMixin, self).setUp()
+
+    def tearDown(self):
+        super(DoesNotLeakFilesMixin, self).tearDown()
+        after = get_number_open_files()
+        if after > self.__open_files_count:
+            raise AssertionError(
+                "Too many open files. Before: %s < After: %s.\n%s" % (
+                    self.__open_files_count,
+                    after,
+                    get_open_files()
+                )
+            )

src/gevent/testing/skipping.py

@@ -43,6 +43,7 @@ skipOnPyPy3 = _do_not_skip
 skipOnPyPyOnWindows = _do_not_skip
 
 skipOnPy2 = unittest.skip if sysinfo.PY2 else _do_not_skip
+skipOnPy3 = unittest.skip if sysinfo.PY3 else _do_not_skip
 skipOnPy37 = unittest.skip if sysinfo.PY37 else _do_not_skip
 
 skipOnPurePython = unittest.skip if sysinfo.PURE_PYTHON else _do_not_skip

src/gevent/testing/testcase.py

@@ -248,6 +248,11 @@ class TestCase(TestCaseMetaClass("NewBase",
     __timeout__ = params.LOCAL_TIMEOUT if not sysinfo.RUNNING_ON_CI else params.CI_TIMEOUT
 
     switch_expected = 'default'
+    #: Set this to true to cause errors that get reported to the hub to
+    #: always get propagated to the main greenlet. This can be done at the
+    #: class or method level.
+    #: .. caution:: This can hide errors and make it look like exceptions
+    #:    are propagated even if they're not.
     error_fatal = True
     uses_handle_error = True
     close_on_teardown = ()
@@ -257,7 +262,7 @@ class TestCase(TestCaseMetaClass("NewBase",
         # pylint:disable=arguments-differ
         if self.switch_expected == 'default':
             self.switch_expected = get_switch_expected(self.fullname)
-        return BaseTestCase.run(self, *args, **kwargs)
+        return super(TestCase, self).run(*args, **kwargs)
 
     def setUp(self):
         super(TestCase, self).setUp()