Reuse an existing loop instance when a new hub is created, fixing a

crash in a particular use cases. Fixes #237 and fixes #238.

Also clean up usage of hub._threadlocal, eliminating many try/except cases.

The particular crash seems to be in a corner-case usage (generally,
destroying a hub and/or loop seems to be advanced or rare usage), so I
feel pretty safe merging it into a release-track branch. The lifetime
management of the loop may not be totally ideal, but it solves the
issue. loop objects may now live longer (until the death of their
thread) if the hub was destroyed but the loop wasn't.

changelog.rst

@@ -19,7 +19,12 @@
 - PyPy: Fix a potential crash. Reported in :issue:`676` by Jay Oster.
 - PyPy: Exceptions raised while handling an error raised by a loop
   callback function behave like the CPython implementation: the
-  exception is printed, and the rest of the callbacks continue processing.
+  exception is printed, and the rest of the callbacks continue
+  processing.
+- If a Hub object with active watchers, was destroyed and then another
+  one created for the same thread which itself was then destroyed with
+  ``destroy_loop=True``, the process could crash. Documented in
+  :issue:`237` and fix based on :pr:`238`, both by Jan-Philip Gehrcke.
 
 1.1b6 (Oct 17, 2015)
 ====================

gevent/hub.py

@@ -58,9 +58,22 @@ if sys.version_info[0] <= 2:
     import thread
 else:
     import _thread as thread
+
+# These must be the "real" native thread versions,
+# not monkey-patched.
 threadlocal = thread._local
-_threadlocal = threadlocal()
-_threadlocal.Hub = None
+
+
+class _threadlocal(threadlocal):
+
+    def __init__(self):
+        threadlocal.__init__(self)
+        self.Hub = None
+        self.loop = None
+        self.hub = None
+
+_threadlocal = _threadlocal()
+
 get_ident = thread.get_ident
 MAIN_THREAD = get_ident()
 
@@ -311,11 +324,7 @@ def get_hub_class():
 
     If there's no type of hub for the current thread yet, 'gevent.hub.Hub' is used.
     """
-    global _threadlocal
-    try:
-        hubtype = _threadlocal.Hub
-    except AttributeError:
-        hubtype = None
+    hubtype = _threadlocal.Hub
     if hubtype is None:
         hubtype = _threadlocal.Hub = Hub
     return hubtype
@@ -328,13 +337,11 @@ def get_hub(*args, **kwargs):
     If a hub does not exist in the current thread, a new one is
     created of the type returned by :func:`get_hub_class`.
     """
-    global _threadlocal
-    try:
-        return _threadlocal.hub
-    except AttributeError:
+    hub = _threadlocal.hub
+    if hub is None:
         hubtype = get_hub_class()
         hub = _threadlocal.hub = hubtype(*args, **kwargs)
-        return hub
+    return hub
 
 
 def _get_hub():
@@ -342,11 +349,7 @@ def _get_hub():
 
     Return ``None`` if no hub has been created yet.
     """
-    global _threadlocal
-    try:
-        return _threadlocal.hub
-    except AttributeError:
-        pass
+    return _threadlocal.hub
 
 
 def set_hub(hub):
@@ -446,6 +449,11 @@ class Hub(greenlet):
             if default is not None:
                 raise TypeError("Unexpected argument: default")
             self.loop = loop
+        elif _threadlocal.loop is not None:
+            # Reuse a loop instance previously set by
+            # destroying a hub without destroying the associated
+            # loop. See #237 and #238.
+            self.loop = _threadlocal.loop
         else:
             if default is None and get_ident() != MAIN_THREAD:
                 default = False
@@ -632,7 +640,6 @@ class Hub(greenlet):
         return False
 
     def destroy(self, destroy_loop=None):
-        global _threadlocal
         if self._resolver is not None:
             self._resolver.close()
             del self._resolver
@@ -642,10 +649,18 @@ class Hub(greenlet):
         if destroy_loop is None:
             destroy_loop = not self.loop.default
         if destroy_loop:
+            if _threadlocal.loop is self.loop:
+                # Don't let anyone try to reuse this
+                _threadlocal.loop = None
             self.loop.destroy()
+        else:
+            # Store in case another hub is created for this
+            # thread.
+            _threadlocal.loop = self.loop
+
         self.loop = None
-        if getattr(_threadlocal, 'hub', None) is self:
-            del _threadlocal.hub
+        if _threadlocal.hub is self:
+            _threadlocal.hub = None
 
     def _get_resolver(self):
         if self._resolver is None:

greentest/test__destroy.py

-from __future__ import print_function
 import gevent
+
 # Loop of initial Hub is default loop.
 hub = gevent.get_hub()
 assert hub.loop.default, hub
 
-# Destroy hub. Does not destroy default loop if not explicitly told to.
+# 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
+
+# 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()
 assert hub.loop.default, hub
 
-saved_loop = hub.loop
+# 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)
-assert saved_loop.fileno() is None, saved_loop
-print(hub, saved_loop)
 
 # Create new hub and explicitly request creation of a new default loop.
 hub = gevent.get_hub(default=True)
 assert hub.loop.default, hub
 
-# Destroy hub including default loop.
-hub.destroy(destroy_loop=True)
+# `gevent.core.loop` objects as well as libev loop pointers must differ.
+assert hub.loop is not initloop
+assert hub.loop.ptr != initloop.ptr
 
-# Create new non-default loop in new hub.
+# Destroy hub including default loop, create new hub with non-default loop.
+hub.destroy(destroy_loop=True)
 hub = gevent.get_hub()
 assert not hub.loop.default, hub
+
 hub.destroy()