Merge pull request #985 from gevent/issue984

Defer adjusting the stdlib's list of active threads until threading is monkey-patched

CHANGES.rst

@@ -13,6 +13,20 @@
   locals after the greenlet exited. Introduce a weak reference to
   avoid that. Reported in :issue:`981` by Heungsub Lee.
 
+- Defer adjusting the stdlib's list of active threads until
+  ``threading`` is monkey patched. Previously this was done when
+  :mod:`gevent.threading` was imported. That module is documented to
+  be used as a helper for monkey patching, so this should generally
+  functionally be the same, but some applications ignore the directly
+  import that module anyway.
+
+  A positive consequence is that ``import gevent.threading, threading;
+  threading.current_thread()`` will no longer return a DummyThread
+  before monkey-patching. Another positive consequence is that PyPy
+  will no longer print a ``KeyError`` on exit if
+  :mod:`gevent.threading` was imported *without* monkey-patching.
+
+  See :issue:`984`.
 
 1.2.2 (2017-06-05)
 ==================

src/gevent/monkey.py

@@ -24,6 +24,9 @@ this code, ideally before any other imports::
     from gevent import monkey
     monkey.patch_all()
 
+A corollary of the above is that patching **should be done on the main
+thread** and **should be done while the program is single-threaded**.
+
 .. tip::
 
     Some frameworks, such as gunicorn, handle monkey-patching for you.
@@ -148,7 +151,19 @@ def remove_item(module, attr):
     delattr(module, attr)
 
 
-def patch_module(name, items=None):
+def __call_module_hook(gevent_module, name, module, items, warn):
+    func_name = '_gevent_' + name + '_monkey_patch'
+    try:
+        func = getattr(gevent_module, func_name)
+    except AttributeError:
+        pass
+    else:
+        func(module, items, warn)
+
+def patch_module(name, items=None, _warnings=None):
+    def warn(message):
+        _queue_warning(message, _warnings)
+
     gevent_module = getattr(__import__('gevent.' + name), name)
     module_name = getattr(gevent_module, '__target__', name)
     module = __import__(module_name)
@@ -156,8 +171,14 @@ def patch_module(name, items=None):
         items = getattr(gevent_module, '__implements__', None)
         if items is None:
             raise AttributeError('%r does not have __implements__' % gevent_module)
+
+    __call_module_hook(gevent_module, 'will', module, items, warn)
+
     for attr in items:
         patch_item(module, attr, getattr(gevent_module, attr))
+
+    __call_module_hook(gevent_module, 'did', module, items, warn)
+
     return module
 
 

src/gevent/threading.py

@@ -6,7 +6,21 @@ Implementation of the standard :mod:`threading` using greenlets.
     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`.
+    :class:`gevent.Greenlet` class or :func:`gevent.spawn`. Attributes
+    in this module like ``__threading__`` are implementation artifacts subject
+    to change at any time.
+
+.. versionchanged:: 1.2.3
+
+   Defer adjusting the stdlib's list of active threads until we are
+   monkey patched. Previously this was done at import time. We are
+   documented to only be used as a helper for monkey patching, so this should
+   functionally be the same, but some applications ignore the documentation and
+   directly import this module anyway.
+
+   A positive consequence is that ``import gevent.threading,
+   threading; threading.current_thread()`` will no longer return a DummyThread
+   before monkey-patching.
 """
 from __future__ import absolute_import
 
@@ -26,7 +40,6 @@ import threading as __threading__
 _DummyThread_ = __threading__._DummyThread
 from gevent.local import local
 from gevent.thread import start_new_thread as _start_new_thread, allocate_lock as _allocate_lock, get_ident as _get_ident
-from gevent._compat import PYPY
 from gevent.hub import sleep as _sleep, getcurrent
 
 # Exports, prevent unused import warnings
@@ -126,45 +139,12 @@ if hasattr(__threading__, 'main_thread'): # py 3.4+
     def main_native_thread():
         return __threading__.main_thread() # pylint:disable=no-member
 else:
-    _main_threads = [(_k, _v) for _k, _v in __threading__._active.items()
-                     if isinstance(_v, __threading__._MainThread)]
-    assert len(_main_threads) == 1, "Too many main threads"
-
     def main_native_thread():
-        return _main_threads[0][1]
-
-# Make sure the MainThread can be found by our current greenlet ID,
-# otherwise we get a new DummyThread, which cannot be joined.
-# Fixes tests in test_threading_2 under PyPy, and generally makes things nicer
-# when gevent.threading is imported before monkey patching or not at all
-# XXX: This assumes that the import is happening in the "main" greenlet/thread.
-# XXX: We should really only be doing this from gevent.monkey.
-if _get_ident() not in __threading__._active:
-    _v = main_native_thread()
-    _k = _v.ident
-    del __threading__._active[_k]
-    _v._ident = _v._Thread__ident = _get_ident()
-    __threading__._active[_get_ident()] = _v
-    del _k
-    del _v
-
-    # Avoid printing an error on shutdown trying to remove the thread entry
-    # we just replaced if we're not fully monkey patched in
-    # XXX: This causes a hang on PyPy for some unknown reason (as soon as class _active
-    # defines __delitem__, shutdown hangs. Maybe due to something with the GC?
-    # XXX: This may be fixed in 2.6.1+
-    if not PYPY:
-        # pylint:disable=no-member
-        _MAIN_THREAD = __threading__._get_ident() if hasattr(__threading__, '_get_ident') else __threading__.get_ident()
-
-        class _active(dict):
-            def __delitem__(self, k):
-                if k == _MAIN_THREAD and k not in self:
-                    return
-                dict.__delitem__(self, k)
-
-        __threading__._active = _active(__threading__._active)
+        main_threads = [v for v in __threading__._active.values()
+                        if isinstance(v, __threading__._MainThread)]
+        assert len(main_threads) == 1, "Too many main threads"
 
+        return main_threads[0]
 
 import sys
 if sys.version_info[:2] >= (3, 4):
@@ -229,3 +209,19 @@ if sys.version_info[:2] >= (3, 3):
     assert hasattr(__threading__, '_CRLock'), "Unsupported Python version"
     _CRLock = None
     __implements__.append('_CRLock')
+
+def _gevent_will_monkey_patch(native_module, items, warn): # pylint:disable=unused-argument
+    # Make sure the MainThread can be found by our current greenlet ID,
+    # otherwise we get a new DummyThread, which cannot be joined.
+    # Fixes tests in test_threading_2 under PyPy.
+    main_thread = main_native_thread()
+    if __threading__.current_thread() != main_thread:
+        warn("Monkey-patching outside the main native thread. Some APIs "
+             "will not be available. Expect a KeyError to be printed at shutdown.")
+        return
+
+    if _get_ident() not in __threading__._active:
+        main_id = main_thread.ident
+        del __threading__._active[main_id]
+        main_thread._ident = main_thread._Thread__ident = _get_ident()
+        __threading__._active[_get_ident()] = main_thread

src/greentest/test__threading_monkey_in_thread.py

+# We can monkey-patch in a thread, but things don't work as expected.
+import sys
+import threading
+from gevent import monkey
+import greentest
+
+
+class Test(greentest.TestCase):
+
+    @greentest.ignores_leakcheck # can't be run multiple times
+    def test_patch_in_thread(self):
+        all_warnings = []
+        try:
+            get_ident = threading.get_ident
+        except AttributeError:
+            get_ident = threading._get_ident
+
+        def process_warnings(warnings):
+            all_warnings.extend(warnings)
+        monkey._process_warnings = process_warnings
+
+        current = threading.current_thread()
+        current_id = get_ident()
+
+        def target():
+            tcurrent = threading.current_thread()
+            monkey.patch_all()
+            tcurrent2 = threading.current_thread()
+            self.assertIsNot(tcurrent, current)
+            # We get a dummy thread now
+            self.assertIsNot(tcurrent, tcurrent2)
+
+        thread = threading.Thread(target=target)
+        thread.start()
+        thread.join()
+
+        self.assertFalse(isinstance(current, threading._DummyThread))
+        self.assertTrue(isinstance(current, monkey.get_original('threading', 'Thread')))
+
+
+        # We generated some warnings
+        if sys.version_info >= (3, 4):
+            self.assertEqual(all_warnings,
+                             ['Monkey-patching outside the main native thread. Some APIs will not be '
+                              'available. Expect a KeyError to be printed at shutdown.',
+                              'Monkey-patching not on the main thread; threading.main_thread().join() '
+                              'will hang from a greenlet'])
+        else:
+            self.assertEqual(all_warnings,
+                             ['Monkey-patching outside the main native thread. Some APIs will not be '
+                              'available. Expect a KeyError to be printed at shutdown.'])
+
+
+        # Manual clean up so we don't get a KeyError
+        del threading._active[current_id]
+        threading._active[(getattr(threading, 'get_ident', None) or threading._get_ident)()] = current
+
+
+if __name__ == '__main__':
+    greentest.main()