Deterministically close GreenFileDescriptorIO backing FileObjectPosix if the...

Deterministically close GreenFileDescriptorIO backing FileObjectPosix if the (libuv) event loop watchers produce an error.

Especially on Python 3.7, the C-level garbage collection could kick in and close the underlying fileno at an inopportune time, killing an unrelated file, even though there were no Python-level references alive (apparently).

src/gevent/_fileobjectposix.py

@@ -33,19 +33,37 @@ class GreenFileDescriptorIO(RawIOBase):
 
     def __init__(self, fileno, mode='r', closefd=True):
         RawIOBase.__init__(self) # Python 2: pylint:disable=no-member,non-parent-init-called
+
         self._closefd = closefd
         self._fileno = fileno
         make_nonblocking(fileno)
         readable = 'r' in mode
         writable = 'w' in mode
-        self.hub = get_hub()
 
+        self.hub = get_hub()
         io_watcher = self.hub.loop.io
+        try:
             if readable:
                 self._read_event = io_watcher(fileno, 1)
 
             if writable:
                 self._write_event = io_watcher(fileno, 2)
+        except:
+            # If anything goes wrong, it's important to go ahead and
+            # close these watchers *now*, especially under libuv, so
+            # that they don't get eventually reclaimed by the garbage
+            # collector at some random time, thanks to the C level
+            # slot (even though we don't seem to have any actual references
+            # at the Python level). Previously, if we didn't close now,
+            # that random close in the future would cause issues if we had duplicated
+            # the fileno (if a wrapping with statement had closed an open fileobject,
+            # for example)
+
+            # test__fileobject can show a failure if this doesn't happen
+            # TRAVIS=true GEVENT_LOOP=libuv python -m gevent.tests.test__fileobject \
+            #    TestFileObjectPosix.test_seek TestFileObjectThread.test_bufsize_0
+            self.close()
+            raise
 
     def readable(self):
         return self._read_event is not None
@@ -70,6 +88,17 @@ class GreenFileDescriptorIO(RawIOBase):
     def closed(self):
         return self._closed
 
+    def __destroy_events(self):
+        read_event = self._read_event
+        write_event = self._write_event
+        hub = self.hub
+        self.hub = self._read_event = self._write_event = None
+
+        if read_event is not None:
+            hub.cancel_wait(read_event, cancel_wait_ex, True)
+        if write_event is not None:
+            hub.cancel_wait(write_event, cancel_wait_ex, True)
+
     def close(self):
         if self._closed:
             return
@@ -77,15 +106,7 @@ class GreenFileDescriptorIO(RawIOBase):
         # TODO: Can we use 'read_event is not None and write_event is
         # not None' to mean _closed?
         self._closed = True
-        read_event = self._read_event
-        write_event = self._write_event
-        self._read_event = self._write_event = None
-
-        if read_event is not None:
-            self.hub.cancel_wait(read_event, cancel_wait_ex, True)
-        if write_event is not None:
-            self.hub.cancel_wait(write_event, cancel_wait_ex, True)
-
+        self.__destroy_events()
         fileno = self._fileno
         if self._closefd:
             self._fileno = None
@@ -295,10 +316,6 @@ class FileObjectPosix(FileObjectBase):
             # that code was never tested and was explicitly marked as "not used"
             raise ValueError('mode can only be [rb, rU, wb], not %r' % (orig_mode,))
 
-        self._fobj = fobj
-
-        # This attribute is documented as available for non-blocking reads.
-        self.fileio = GreenFileDescriptorIO(fileno, mode, closefd=close)
 
         self._orig_bufsize = bufsize
         if bufsize < 0 or bufsize == 1:
@@ -317,7 +334,14 @@ class FileObjectPosix(FileObjectBase):
                 # attribute.
                 IOFamily = FlushingBufferedWriter
 
-        super(FileObjectPosix, self).__init__(IOFamily(self.fileio, bufsize), close)
+
+        self._fobj = fobj
+        # This attribute is documented as available for non-blocking reads.
+        self.fileio = GreenFileDescriptorIO(fileno, mode, closefd=close)
+
+        buffered_fobj = IOFamily(self.fileio, bufsize)
+
+        super(FileObjectPosix, self).__init__(buffered_fobj, close)
 
     def _do_close(self, fobj, closefd):
         try:

src/gevent/testing/flaky.py

@@ -23,8 +23,6 @@ import sys
 import functools
 import unittest
 
-from gevent.util import dump_stacks
-
 from . import sysinfo
 from . import six
 
@@ -69,7 +67,6 @@ reraiseFlakyTestTimeoutLibuv = reraiseFlakyTestRaceCondition
 if sysinfo.RUNNING_ON_CI or (sysinfo.PYPY and sysinfo.WIN):
     # pylint: disable=function-redefined
     def reraiseFlakyTestRaceCondition():
-        if sysinfo.PYPY and sysinfo.WIN:
         # Getting stack traces is incredibly expensive
         # in pypy on win, at least in test virtual machines.
         # It can take minutes. The traceback consistently looks like
@@ -78,9 +75,13 @@ if sysinfo.RUNNING_ON_CI or (sysinfo.PYPY and sysinfo.WIN):
         # dump_stacks -> traceback.format_stack
         #    -> traceback.extract_stack -> linecache.checkcache
         #    -> os.stat -> _structseq.structseq_new
+
+        # Moreover, without overriding __repr__ or __str__,
+        # the msg doesn't get printed like we would want (its basically
+        # unreadable, all printed on one line). So skip that.
+
+        #msg = '\n'.join(dump_stacks())
         msg = str(sys.exc_info()[1])
-        else:
-            msg = '\n'.join(dump_stacks())
         six.reraise(FlakyTestRaceCondition,
                     FlakyTestRaceCondition(msg),
                     sys.exc_info()[2])

src/gevent/tests/test__fileobject.py

@@ -102,7 +102,7 @@ class TestFileObjectBlock(greentest.TestCase):
 
         with open(path, 'rb') as f_raw:
             try:
-                f = self._makeOne(f_raw, 'rb')
+                f = self._makeOne(f_raw, 'rb', close=False)
             except ValueError:
                 # libuv on Travis can raise EPERM
                 # from FileObjectPosix. I can't produce it on mac os locally,
@@ -111,12 +111,17 @@ class TestFileObjectBlock(greentest.TestCase):
                 # That shouldn't have any effect on io watchers, though, which were
                 # already being explicitly closed.
                 reraiseFlakyTestRaceConditionLibuv()
+
             if PY3 or hasattr(f, 'seekable'):
                 # On Python 3, all objects should have seekable.
                 # On Python 2, only our custom objects do.
                 self.assertTrue(f.seekable())
             f.seek(15)
             self.assertEqual(15, f.tell())
+
+            # Note that a duplicate close() of the underlying
+            # file descriptor can look like an OSError from this line
+            # as we exit the with block
             fileobj_data = f.read(1024)
 
         self.assertEqual(native_data, s)
@@ -183,23 +188,6 @@ class TestFileObjectThread(ConcurrentFileObjectMixin,
     def _getTargetClass(self):
         return fileobject.FileObjectThread
 
-    def tearDown(self):
-        # Make sure outstanding tasks have completed.
-        # On Travis with Python 3.7 and libuv, test_bufsize_0
-        # fails in os.fdopen(), claiming the file descriptor is bad.
-        # This would happen if we closed (garbage collected?) a FD,
-        # opened a pipe and got the same int FD, and then some background
-        # task closed that same int FD again. FileObjectThread.close()
-        # goes through threadpool.apply(), which is supposed to be synchronous;
-        # make sure it is.
-        gevent.get_hub().threadpool.join()
-
-        # And collect any outstanding garbage, in case some resource is "leaking"
-        # (We have no indication that it is, but we're flailing wildly here to try
-        # to understand what could be happening)
-        gc.collect()
-        super(TestFileObjectThread, self).tearDown()
-
     # FileObjectThread uses os.fdopen() when passed a file-descriptor,
     # which returns an object with a destructor that can't be
     # bypassed, so we can't even create one that way