Merge pull request #1602 from gevent/issue1587

Avoid closing the same libuv watcher object twice

.travis.yml

@@ -174,7 +174,7 @@ jobs:
         # First, the build dependencies (see setup.cfg)
         # so that we don't have to use build isolation and can better use the cache;
         # Note that we can't use -U for cffi and greenlet on PyPy.
-        - &build-gevent-deps pip install -U setuptools wheel twine && pip install -U 'cffi;platform_python_implementation=="CPython"' cython 'greenlet;platform_python_implementation=="CPython"'
+        - &build-gevent-deps pip install -U setuptools wheel twine && pip install -U 'faulthandler; python_version == "2.7" and platform_python_implementation == "CPython"' 'cffi;platform_python_implementation=="CPython"' cython 'greenlet;platform_python_implementation=="CPython"'
         # Next, build the wheel *in place*. This helps ccache, and also lets us cache the configure
         # output (pip install uses a random temporary directory, making this difficult)
         - python setup.py bdist_wheel

dev-requirements.txt

@@ -9,6 +9,9 @@ pylint>=1.8.0 ; python_version < "3.4"
 pylint >= 2.5.0 ; python_version >= "3.4"  and platform_python_implementation == "CPython"
 astroid >= 2.4.0 ; python_version >= "3.4" and platform_python_implementation == "CPython"
 
+# backport of faulthandler
+faulthandler ; python_version == "2.7"  and platform_python_implementation == "CPython"
+
 # For generating CHANGES.rst
 towncrier
 # For making releases

docs/changes/1587.bugfix

+Avoid closing the same Python libuv watcher IO object twice. Under
+some circumstances (only seen on Windows), that could lead to program
+crashes.

src/gevent/_ffi/loop.py

@@ -92,7 +92,7 @@ class AbstractCallbacks(object):
           :func:`_python_handle_error` to deal with it. The Python watcher
           object will have the exception tuple saved in ``_exc_info``.
         - 1
-          Everything went according to plan. You should check to see if the libev
+          Everything went according to plan. You should check to see if the native
           watcher is still active, and call :func:`python_stop` if it is not. This will
           clean up the memory. Finding the watcher still active at the event loop level,
           but not having stopped itself at the gevent level is a buggy scenario and
@@ -182,8 +182,9 @@ class AbstractCallbacks(object):
                     and the_watcher in the_watcher.loop._keepaliveset
                     and the_watcher._watcher is orig_ffi_watcher):
                 # It didn't stop itself, *and* it didn't stop itself, reset
-                # its watcher, and start itself again. libuv's io watchers MAY
-                # do that.
+                # its watcher, and start itself again. libuv's io watchers
+                # multiplex and may do this.
+
                 # The normal, expected scenario when we find the watcher still
                 # in the keepaliveset is that it is still active at the event loop
                 # level, so we don't expect that python_stop gets called.

src/gevent/libuv/loop.py

@@ -483,19 +483,19 @@ class loop(AbstractLoop):
             val = _callbacks.python_callback(handle, arg)
             if val == -1: # Failure.
                 _callbacks.python_handle_error(handle, arg)
-            elif val == 1: # Success
+            elif val == 1: # Success, and we may need to close the Python watcher.
                 if not libuv.uv_is_active(watcher_ptr):
-                    # The callback closed the watcher in C. Good.
-                    # It's supposed to also reset the pointer to NULL at
+                    # The callback closed the native watcher resources. Good.
+                    # It's *supposed* to also reset the .data handle to NULL at
                     # that same time. If it resets it to something else, we're
                     # re-using the same watcher object, and that's not correct either.
-                    # Prevoiusly we checked for that case, but we shouldn't need to.
+                    # On Windows in particular, if the .data handle is changed because
+                    # the IO multiplexer is being restarted, trying to dereference the
+                    # *old* handle can crash with an FFI error.
                     handle_after_callback = watcher_ptr.data
                     try:
-                        if handle_after_callback:
+                        if handle_after_callback and handle_after_callback == handle:
                             _callbacks.python_stop(handle_after_callback)
-                        if handle_after_callback != handle:
-                            _callbacks.python_stop(handle)
                     finally:
                         watcher_ptr.data = ffi.NULL
         return True

src/gevent/libuv/watcher.py

@@ -18,7 +18,23 @@ from gevent._ffi import _dbg
 
 # A set of uv_handle_t* CFFI objects. Kept around
 # to keep the memory alive until libuv is done with them.
-_closing_watchers = set()
+class _ClosingWatchers(dict):
+    __slots__ = ()
+
+    def remove(self, obj):
+        try:
+            del self[obj]
+        except KeyError: # pragma: no cover
+            # This has been seen to happen if the module is executed twice
+            # and so the callback doesn't match the storage seen by watcher objects.
+            print(
+                'gevent error: Unable to remove closing watcher from keepaliveset. '
+                'Has the module state been corrupted or executed more than once?',
+                file=sys.stderr
+            )
+
+_closing_watchers = _ClosingWatchers()
+
 
 # In debug mode, it would be nice to be able to clear the memory of
 # the watcher (its size determined by
@@ -136,7 +152,7 @@ class watcher(_base.watcher):
             # and trying to close it results in libuv terminating the process.
             # Sigh. Same thing if it's already in the process of being
             # closed.
-            _closing_watchers.add(ffi_watcher)
+            _closing_watchers[ffi_handle_watcher] = ffi_watcher
             libuv.uv_close(ffi_watcher, libuv._uv_close_callback)
 
     def _watcher_ffi_set_init_ref(self, ref):

src/gevent/testing/__init__.py

@@ -28,10 +28,22 @@ import unittest
 # It's important to do this ASAP, because if we're monkey patched,
 # then importing things like the standard library test.support can
 # need to construct the hub (to check for IPv6 support using a socket).
+# We can't do it in the testrunner, as the testrunner spaws new, unrelated
+# processes.
 from .hub import QuietHub
 import gevent.hub
 gevent.hub.set_default_hub_class(QuietHub)
 
+try:
+    import faulthandler
+except ImportError:
+    # The backport isn't installed.
+    pass
+else:
+    # Enable faulthandler for stack traces. We have to do this here
+    # for the same reasons as above.
+    faulthandler.enable()
+
 from .sysinfo import VERBOSE
 from .sysinfo import WIN
 from .sysinfo import LINUX

src/gevent/tests/test__execmodules.py

@@ -40,4 +40,7 @@ class Test(unittest.TestCase):
 
 
 if __name__ == '__main__':
+    # This should not be combined with other tests in the same process
+    # because it messes with global shared state.
+    # pragma: testrunner-no-combine
     main()