Merge pull request #919 from gevent/issue918

Clean up _DummyThread for greenlet.greenlet

.gitignore

@@ -18,7 +18,6 @@ src/greentest/.coverage\.*
 src/greentest/htmlcov
 src/greentest/.coverage
 
-doc/changelog.rst
 doc/_build
 doc/__pycache__
 doc/gevent.*.rst

changelog.rst → CHANGES.rst

@@ -4,6 +4,17 @@
 
 .. currentmodule:: gevent
 
+1.2.1 (unreleased)
+==================
+
+- The ``_DummyThread`` objects created by calling
+  :func:`threading.current_thread` from inside a raw
+  :class:`greenlet.greenlet` now clean up after themselves when the
+  greenlet dies (:class:`gevent.Greenlet`-based ``_DummyThreads`` have
+  always cleaned up). This requires the use of a :class:`weakref.ref`
+  (and may not be timely on PyPy).
+  Reported in :issue:`918` by frozenoctobeer.
+
 1.2.0 (2016-12-23)
 ==================
 

doc/changelog.rst

+.. include:: ../CHANGES.rst

doc/conf.py

@@ -19,15 +19,6 @@ os.system('%s generate_rst.py generate' % sys.executable)
 
 sys.path.append('.')  # for mysphinxext
 
-if not os.path.exists('changelog.rst') and os.path.exists('../changelog.rst'):
-    print('Linking ../changelog.rst to changelog.rst')
-    if hasattr(os, 'symlink'):
-        os.symlink('../changelog.rst', 'changelog.rst')
-    else:
-        import shutil
-        shutil.copyfile('../changelog.rst', 'changelog.rst')
-
-
 # If extensions (or modules to document with autodoc) are in another directory,
 # add these directories to sys.path here. If the directory is relative to the
 # documentation root, use os.path.abspath to make it absolute, like shown here.

doc/reference.rst

@@ -19,6 +19,7 @@ API reference
    gevent.server
    gevent.subprocess
    gevent.thread
+   gevent.threading
    gevent.threadpool
    gevent.util
    lowlevel

src/gevent/greenlet.py

@@ -568,10 +568,13 @@ class Greenlet(greenlet):
             self._notifier = self.parent.loop.run_callback(self._notify_links)
 
     def link(self, callback, SpawnedLink=SpawnedLink):
-        """Link greenlet's completion to a callable.
+        """
+        Link greenlet's completion to a callable.
 
-        The *callback* will be called with this instance as an argument
-        once this greenlet's dead. A callable is called in its own greenlet.
+        The *callback* will be called with this instance as an
+        argument once this greenlet is dead. A callable is called in
+        its own :class:`greenlet.greenlet` (*not* a
+        :class:`Greenlet`).
         """
         # XXX: Is the redefinition of SpawnedLink supposed to just be an
         # optimization, or do people use it? It's not documented
@@ -586,7 +589,10 @@ class Greenlet(greenlet):
             pass
 
     def link_value(self, callback, SpawnedLink=SuccessSpawnedLink):
-        """Like :meth:`link` but *callback* is only notified when the greenlet has completed successfully."""
+        """
+        Like :meth:`link` but *callback* is only notified when the greenlet
+        has completed successfully.
+        """
         # pylint:disable=redefined-outer-name
         self.link(callback, SpawnedLink=SpawnedLink)
 

src/gevent/thread.py

-"""Implementation of the standard :mod:`thread` module that spawns greenlets.
+"""
+Implementation of the standard :mod:`thread` module that spawns greenlets.
 
 .. note::
 
-    This module is a helper for :mod:`gevent.monkey` and is not intended to be
-    used directly. For spawning greenlets in your applications, prefer
-    :class:`Greenlet` class.
+    This module is a helper for :mod:`gevent.monkey` and is not
+    intended to be used directly. For spawning greenlets in your
+    applications, prefer higher level constructs like
+    :class:`gevent.Greenlet` class or :func:`gevent.spawn`.
 """
 from __future__ import absolute_import
 import sys

src/gevent/threading.py

+"""
+Implementation of the standard :mod:`threading` using greenlets.
+
+.. note::
+
+    This module is a helper for :mod:`gevent.monkey` and is not
+    intended to be used directly. For spawning greenlets in your
+    applications, prefer higher level constructs like
+    :class:`gevent.Greenlet` class or :func:`gevent.spawn`.
+"""
 from __future__ import absolute_import
 
 
@@ -33,6 +43,12 @@ Lock = _allocate_lock
 def _cleanup(g):
     __threading__._active.pop(id(g), None)
 
+def _make_cleanup_id(gid):
+    def _(_r):
+        __threading__._active.pop(gid, None)
+    return _
+
+_weakref = None
 
 class _DummyThread(_DummyThread_):
     # We avoid calling the superclass constructor. This makes us about
@@ -80,11 +96,22 @@ class _DummyThread(_DummyThread_):
         self._name = self._Thread__name = __threading__._newname("DummyThread-%d")
         self._set_ident()
 
-        __threading__._active[_get_ident()] = self
         g = getcurrent()
+        gid = _get_ident(g) # same as id(g)
+        __threading__._active[gid] = self
         rawlink = getattr(g, 'rawlink', None)
         if rawlink is not None:
+            # raw greenlet.greenlet greenlets don't
+            # have rawlink...
             rawlink(_cleanup)
+        else:
+            # ... so for them we use weakrefs.
+            # See https://github.com/gevent/gevent/issues/918
+            global _weakref
+            if _weakref is None:
+                _weakref = __import__('weakref')
+            ref = _weakref.ref(g, _make_cleanup_id(gid))
+            self.__raw_ref = ref
 
     def _Thread__stop(self):
         pass

src/greentest/greentest.py

@@ -96,6 +96,11 @@ RUNNING_ON_TRAVIS = os.environ.get('TRAVIS')
 RUNNING_ON_APPVEYOR = os.environ.get('APPVEYOR')
 RUNNING_ON_CI = RUNNING_ON_TRAVIS or RUNNING_ON_APPVEYOR
 
+def _do_not_skip(reason):
+    def dec(f):
+        return f
+    return dec
+
 if RUNNING_ON_APPVEYOR:
     # See comments scattered around about timeouts and the timer
     # resolution available on appveyor (lots of jitter). this
@@ -110,19 +115,18 @@ if RUNNING_ON_APPVEYOR:
     # 'develop' mode (i.e., we install)
     NON_APPLICABLE_SUFFIXES.append('corecext')
 else:
-    def skipOnAppVeyor(reason):
-        def dec(f):
-            return f
-        return dec
+    skipOnAppVeyor = _do_not_skip
 
 if PYPY3 and RUNNING_ON_CI:
     # Same as above, for PyPy3.3-5.5-alpha
     skipOnPyPy3OnCI = unittest.skip
 else:
-    def skipOnPyPy3OnCI(reason):
-        def dec(f):
-            return f
-        return dec
+    skipOnPyPy3OnCI = _do_not_skip
+
+if PYPY:
+    skipOnPyPy = unittest.skip
+else:
+    skipOnPyPy = _do_not_skip
 
 EXPECT_POOR_TIMER_RESOLUTION = PYPY3 or RUNNING_ON_APPVEYOR
 

src/greentest/test__threading.py

@@ -16,14 +16,35 @@ def helper():
 
 class Test(greentest.TestCase):
 
-    def test(self):
+    def _do_test(self, spawn):
         before = len(threading._active)
-        g = gevent.spawn(helper)
+        g = spawn(helper)
         gevent.sleep(0.1)
         self.assertEqual(len(threading._active), before + 1)
-        g.join()
+        try:
+            g.join()
+        except AttributeError:
+            while not g.dead:
+                gevent.sleep()
+            # Raw greenlet has no join(), uses a weakref to cleanup.
+            # so the greenlet has to die. On CPython, it's enough to
+            # simply delete our reference.
+            del g
+            # On PyPy, it might take a GC, but for some reason, even
+            # running several GC's doesn't clean it up under 5.6.0.
+            # So we skip the test.
+            #import gc
+            #gc.collect()
+
         self.assertEqual(len(threading._active), before)
 
 
+    def test_cleanup_gevent(self):
+        self._do_test(gevent.spawn)
+
+    @greentest.skipOnPyPy("weakref is not cleaned up in a timely fashion")
+    def test_cleanup_raw(self):
+        self._do_test(gevent.spawn_raw)
+
 if __name__ == '__main__':
     greentest.main()