Merge pull request #1100 from gevent/issue1098

Don't let destroying the default C loop from two different Python loo…

CHANGES.rst

@@ -58,6 +58,13 @@
     order, or be a dotted name; it may also be assigned to an
     object in Python code at ``gevent.config.loop``).
 
+- Fix an interpreter crash that could happen if two or more ``loop``
+  objects referenced the default event loop and one of them was
+  destroyed and then the other one destroyed or (in the libev C
+  extension implementation only) deallocated (garbage collected). See
+  :issue:`1098`.
+
+
 1.3a1 (2018-01-27)
 ==================
 

src/gevent/_ffi/loop.py

@@ -312,6 +312,13 @@ class AbstractLoop(object):
 
     starting_timer_may_update_loop_time = False
 
+    # Subclasses should set this in __init__ to reflect
+    # whether they were the default loop.
+    _default = None
+
+    # A class variable.
+    _default_loop_destroyed = False
+
     def __init__(self, ffi, lib, watchers, flags=None, default=None):
         self._ffi = ffi
         self._lib = lib
@@ -470,12 +477,32 @@ class AbstractLoop(object):
         raise NotImplementedError()
 
     def destroy(self):
+        """
+        Clean up resources used by this loop.
+
+        Note that, unlike the libev C loop implementation, this object
+        *does not* have a finalizer (``__del__``). If you create loops
+        (especially loops that are not the default) you *should* call
+        this method when you are done with the loop.
+        """
         if self._ptr:
-            self._stop_aux_watchers()
+            try:
+                if self._default:
+                    if not self._can_destroy_default_loop():
+                        return False
+                    type(self)._default_loop_destroyed = True
+
+                self._stop_aux_watchers()
+            finally:
+                # not ffi.NULL, we don't want something that can be
+                # passed to C and crash later. This will create nice friendly
+                # TypeError from CFFI.
+                self._ptr = None
 
-            # not ffi.NULL, we don't want something that can be
-            # passed to C and crash later.
-            self._ptr = None
+            return True
+
+    def _can_destroy_default_loop(self):
+        return not type(self)._default_loop_destroyed
 
     @property
     def ptr(self):
@@ -566,7 +593,7 @@ class AbstractLoop(object):
 
     @property
     def default(self):
-        pass
+        return self._default if self._ptr else False
 
     @property
     def iteration(self):

src/gevent/libev/corecext.pyx

@@ -389,9 +389,15 @@ cdef public class loop [object PyGeventLoopObject, type PyGeventLoop_Type]:
 
     ## data members
     cdef bint starting_timer_may_update_loop_time
+    # We must capture the 'default' state at initialiaztion
+    # time. Destroying the default loop in libev sets
+    # the libev internal pointer to 0, and ev_is_default_loop will
+    # no longer work.
+    cdef bint _default
 
     def __cinit__(self, object flags=None, object default=None, libev.intptr_t ptr=0):
         self.starting_timer_may_update_loop_time = 0
+        self._default = 0
         libev.ev_prepare_init(&self._prepare,
                               <void*>gevent_run_callbacks)
         libev.ev_timer_init(&self._periodic_signal_checker,
@@ -405,6 +411,7 @@ cdef public class loop [object PyGeventLoopObject, type PyGeventLoop_Type]:
         cdef object old_handler = None
         if ptr:
             self._ptr = <libev.ev_loop*>ptr
+            self._default = libev.ev_is_default_loop(self._ptr)
         else:
             c_flags = _flags_to_int(flags)
             _check_flags(c_flags)
@@ -415,6 +422,7 @@ cdef public class loop [object PyGeventLoopObject, type PyGeventLoop_Type]:
                 if _default_loop_destroyed:
                     default = False
             if default:
+                self._default = 1
                 self._ptr = libev.gevent_ev_default_loop(c_flags)
                 if not self._ptr:
                     raise SystemError("ev_default_loop(%s) failed" % (c_flags, ))
@@ -469,31 +477,48 @@ cdef public class loop [object PyGeventLoopObject, type PyGeventLoop_Type]:
         finally:
             self.starting_timer_may_update_loop_time = False
 
-    def _stop_watchers(self):
+    cdef _stop_watchers(self, libev.ev_loop* ptr):
+        if not ptr:
+            return
+
         if libev.ev_is_active(&self._prepare):
-            libev.ev_ref(self._ptr)
-            libev.ev_prepare_stop(self._ptr, &self._prepare)
+            libev.ev_ref(ptr)
+            libev.ev_prepare_stop(ptr, &self._prepare)
         if libev.ev_is_active(&self._periodic_signal_checker):
-            libev.ev_ref(self._ptr)
-            libev.ev_timer_stop(self._ptr, &self._periodic_signal_checker)
+            libev.ev_ref(ptr)
+            libev.ev_timer_stop(ptr, &self._periodic_signal_checker)
 
     def destroy(self):
         global _default_loop_destroyed
-        if self._ptr:
-            self._stop_watchers()
+
+        cdef libev.ev_loop* ptr = self._ptr
+        self._ptr = NULL
+
+        if ptr:
+            if self._default and _default_loop_destroyed:
+                # Whoops! Program error. They destroyed the default loop,
+                # using a different loop object. Our _ptr is still
+                # valid, but the libev loop is gone. Doing anything
+                # else with it will likely cause a crash.
+                return
+            self._stop_watchers(ptr)
             if __SYSERR_CALLBACK == self._handle_syserr:
                 set_syserr_cb(None)
-            if libev.ev_is_default_loop(self._ptr):
+            if self._default:
                 _default_loop_destroyed = True
-            libev.ev_loop_destroy(self._ptr)
-            self._ptr = NULL
+            libev.ev_loop_destroy(ptr)
 
     def __dealloc__(self):
-        if self._ptr:
-            self._stop_watchers()
-            if not libev.ev_is_default_loop(self._ptr):
-                libev.ev_loop_destroy(self._ptr)
-            self._ptr = NULL
+        cdef libev.ev_loop* ptr = self._ptr
+        self._ptr = NULL
+        if ptr != NULL:
+            if self._default and _default_loop_destroyed:
+                # See destroy(). This is a bug in the caller.
+                return
+
+            self._stop_watchers(ptr)
+            if not self._default:
+                libev.ev_loop_destroy(ptr)
 
     @property
     def ptr(self):
@@ -578,8 +603,9 @@ cdef public class loop [object PyGeventLoopObject, type PyGeventLoop_Type]:
 
     @property
     def default(self):
-        _check_loop(self)
-        return True if libev.ev_is_default_loop(self._ptr) else False
+        # If we're destroyed, we are not the default loop anymore,
+        # as far as Python is concerned.
+        return self._default if self._ptr else False
 
     @property
     def iteration(self):
@@ -662,7 +688,7 @@ cdef public class loop [object PyGeventLoopObject, type PyGeventLoop_Type]:
         if not self._ptr:
             return 'destroyed'
         cdef object msg = self.backend
-        if self.default:
+        if self._default:
             msg += ' default'
         msg += ' pending=%s' % self.pendingcnt
         msg += self._format_details()

src/gevent/libev/corecffi.py

@@ -194,9 +194,6 @@ def embeddable_backends():
 def time():
     return libev.ev_time()
 
-_default_loop_destroyed = False
-
-
 from gevent._ffi.loop import AbstractLoop
 
 
@@ -216,6 +213,8 @@ class loop(AbstractLoop):
 
     def __init__(self, flags=None, default=None):
         AbstractLoop.__init__(self, ffi, libev, _watchers, flags, default)
+        self._default = True if libev.ev_is_default_loop(self._ptr) else False
+
 
     def _init_loop(self, flags, default):
         c_flags = _flags_to_int(flags)
@@ -224,7 +223,7 @@ class loop(AbstractLoop):
         c_flags |= libev.EVFLAG_FORKCHECK
         if default is None:
             default = True
-            if _default_loop_destroyed:
+            if loop._default_loop_destroyed:
                 default = False
 
         if default:
@@ -272,17 +271,16 @@ class loop(AbstractLoop):
         self.ref() # we should go through the loop now
 
     def destroy(self):
-        global _default_loop_destroyed
         if self._ptr:
             ptr = self._ptr
 
-            super(loop, self).destroy()
+            should_destroy_loop = super(loop, self).destroy()
 
             if globals()["__SYSERR_CALLBACK"] == self._handle_syserr:
                 set_syserr_cb(None)
-            if libev.ev_is_default_loop(ptr):
-                _default_loop_destroyed = True
-            libev.ev_loop_destroy(ptr)
+
+            if should_destroy_loop:
+                libev.ev_loop_destroy(ptr)
 
     @property
     def MAXPRI(self):
@@ -329,10 +327,6 @@ class loop(AbstractLoop):
     def __repr__(self):
         return '<%s at 0x%x %s>' % (self.__class__.__name__, id(self), self._format())
 
-    @property
-    def default(self):
-        return True if libev.ev_is_default_loop(self._ptr) else False
-
     @property
     def iteration(self):
         return libev.ev_iteration(self._ptr)

src/gevent/libuv/loop.py

@@ -86,6 +86,7 @@ class loop(AbstractLoop):
         self._io_watchers = dict()
         self._fork_watchers = set()
         self._pid = os.getpid()
+        self._default = self._ptr == libuv.uv_default_loop()
 
     def _init_loop(self, flags, default):
         if default is None:
@@ -95,6 +96,8 @@ class loop(AbstractLoop):
             # closed.
 
         if default:
+            # XXX: If the default loop had been destroyed, this
+            # will create a new one, but we won't destroy it
             ptr = libuv.uv_default_loop()
         else:
             ptr = libuv.uv_loop_new()
@@ -102,6 +105,10 @@ class loop(AbstractLoop):
 
         if not ptr:
             raise SystemError("Failed to get loop")
+
+        # Track whether or not any object has destroyed
+        # this loop. See _can_destroy_default_loop
+        ptr.data = ptr
         return ptr
 
     _signal_idle = None
@@ -258,10 +265,15 @@ class loop(AbstractLoop):
     def destroy(self):
         if self._ptr:
             ptr = self._ptr
-            super(loop, self).destroy()
-
+            should_destroy = super(loop, self).destroy()
+            ptr.data = ffi.NULL
             assert self._ptr is None
             libuv.uv_stop(ptr)
+            if not should_destroy:
+                # The default loop has already been destroyed.
+                # libuv likes to abort() the process in this case.
+                return
+
             closed_failed = libuv.uv_loop_close(ptr)
             if closed_failed:
                 assert closed_failed == libuv.UV_EBUSY
@@ -284,8 +296,14 @@ class loop(AbstractLoop):
                 closed_failed = libuv.uv_loop_close(ptr)
                 assert closed_failed == 0, closed_failed
 
-            # XXX: Do we need to uv_loop_delete the non-default loop?
-            # Probably...
+    def _can_destroy_default_loop(self):
+        # We're being asked to destroy a loop that's,
+        # at the time it was constructed, was the default loop.
+        # If loop objects were constructed more than once,
+        # it may have already been destroyed, though.
+        # We track this in the data member.
+        return self._ptr.data
+
 
     def debug(self):
         """
@@ -391,10 +409,6 @@ class loop(AbstractLoop):
     def update_now(self):
         libuv.uv_update_time(self._ptr)
 
-    @property
-    def default(self):
-        return self._ptr == libuv.uv_default_loop()
-
     def fileno(self):
         if self._ptr:
             fd = libuv.uv_backend_fd(self._ptr)

src/greentest/test__destroy.py

+from __future__ import absolute_import, print_function
+
 import gevent
+import unittest
+
+class TestDestroyHub(unittest.TestCase):
+
+    def test_destroy_hub(self):
+        # Loop of initial Hub is default loop.
+        hub = gevent.get_hub()
+        self.assertTrue(hub.loop.default)
 
-# Loop of initial Hub is default loop.
-hub = gevent.get_hub()
-assert hub.loop.default, hub
+        # Save `gevent.core.loop` object for later comparison.
+        initloop = hub.loop
 
-# Save `gevent.core.loop` object for later comparison.
-initloop = hub.loop
+        # Increase test complexity via threadpool creation.
+        # Implicitly creates fork watcher connected to the current event loop.
+        tp = hub.threadpool
+        self.assertIsNotNone(tp)
 
-# Increase test complexity via threadpool creation.
-# Implicitly creates fork watcher connected to the current event loop.
-tp = hub.threadpool
+        # Destroy hub. Does not destroy libev default loop if not explicitly told to.
+        hub.destroy()
 
-# Destroy hub. Does not destroy libev default loop if not explicitly told to.
-hub.destroy()
+        # Create new hub. Must re-use existing libev default loop.
+        hub = gevent.get_hub()
+        self.assertTrue(hub.loop.default)
 
-# Create new hub. Must re-use existing libev default loop.
-hub = gevent.get_hub()
-assert hub.loop.default, hub
+        # Ensure that loop object is identical to the initial one.
+        self.assertIs(hub.loop, initloop)
 
-# Ensure that loop object is identical to the initial one.
-assert hub.loop is initloop
+        # Destroy hub including default loop.
+        hub.destroy(destroy_loop=True)
 
-# Destroy hub including default loop.
-hub.destroy(destroy_loop=True)
+        # Create new hub and explicitly request creation of a new default loop.
+        hub = gevent.get_hub(default=True)
+        self.assertTrue(hub.loop.default)
 
-# Create new hub and explicitly request creation of a new default loop.
-hub = gevent.get_hub(default=True)
-assert hub.loop.default, hub
+        # `gevent.core.loop` objects as well as libev loop pointers must differ.
+        self.assertIsNot(hub.loop, initloop)
+        self.assertIsNot(hub.loop.ptr, initloop.ptr)
+        self.assertNotEqual(hub.loop.ptr, initloop.ptr)
 
-# `gevent.core.loop` objects as well as libev loop pointers must differ.
-assert hub.loop is not initloop
-assert hub.loop.ptr != initloop.ptr
+        # Destroy hub including default loop, create new hub with non-default loop.
+        hub.destroy(destroy_loop=True)
+        hub = gevent.get_hub()
+        if not getattr(hub.loop, 'DEFAULT_LOOP_REGENERATES', False):
+            self.assertFalse(hub.loop.default)
+        else:
+            self.assertTrue(hub.loop.default)
 
-# Destroy hub including default loop, create new hub with non-default loop.
-hub.destroy(destroy_loop=True)
-hub = gevent.get_hub()
-if not getattr(hub.loop, 'DEFAULT_LOOP_REGENERATES', False):
-    assert not hub.loop.default, hub
-else:
-    assert hub.loop.default
+        hub.destroy()
 
-hub.destroy()
+if __name__ == '__main__':
+    unittest.main()

src/greentest/test__destroy_default_loop.py

+from __future__ import print_function
+import gevent
+
+import unittest
+
+class TestDestroyDefaultLoop(unittest.TestCase):
+
+    def test_destroy_gc(self):
+        # Issue 1098: destroying the default loop
+        # while using the C extension could crash
+        # the interpreter when it exits
+
+        # Create the hub greenlet. This creates one loop
+        # object pointing to the default loop.
+        gevent.get_hub()
+
+        # Get a new loop object, but using the default
+        # C loop
+        loop = gevent.config.loop(default=True)
+        self.assertTrue(loop.default)
+        # Destroy it
+        loop.destroy()
+        # It no longer claims to be the default
+        self.assertFalse(loop.default)
+
+        # Delete it
+        del loop
+        # Delete the hub. This prompts garbage
+        # collection of it and its loop object.
+        # (making this test more repeatable; the exit
+        # crash only happened when that greenlet object
+        # was collected at exit time, which was most common
+        # in CPython 3.5)
+        del gevent.hub._threadlocal.hub
+
+
+    def test_destroy_two(self):
+        # Get two new loop object, but using the default
+        # C loop
+        loop1 = gevent.config.loop(default=True)
+        loop2 = gevent.config.loop(default=True)
+        self.assertTrue(loop1.default)
+        self.assertTrue(loop2.default)
+        # Destroy the first
+        loop1.destroy()
+        # It no longer claims to be the default
+        self.assertFalse(loop1.default)
+
+        # Destroy the second. This doesn't crash.
+        loop2.destroy()
+
+
+if __name__ == '__main__':
+    unittest.main()

src/greentest/tests_that_dont_use_resolver.txt

@@ -125,3 +125,4 @@ test_timeout.py
 test_asyncore.py
 
 test___config.py
+test__destroy_default_loop.py