Fix the threadpool in things launched with python -m gevent.monkey

Fixes #1484.

In general, fixes any use of gevent.monkey.get_original() in such a process.

Be more careful about double-patching in general too. Detecting duplicate arguments to patch_all only patches things that need to be patched.

CHANGES.rst

@@ -72,6 +72,10 @@
   special circumstances, but real applications are unlikely to be
   affected. See :issue:`1493`.
 
+- Fix using the threadpool inside a script or module run with ``python
+  -m gevent.monkey``. Previously it would use greenlets instead of
+  native threads. See :issue:`1484`.
+
 1.5a2 (2019-10-21)
 ==================
 

src/gevent/monkey.py

@@ -1031,16 +1031,26 @@ def patch_signal():
 
 def _check_repatching(**module_settings):
     _warnings = []
-    key = '_gevent_saved_patch_all'
+    key = '_gevent_saved_patch_all_module_settings'
+
     del module_settings['kwargs']
-    if saved.get(key, module_settings) != module_settings:
+    currently_patched = saved.setdefault(key, {})
+    first_time = not currently_patched
+    if not first_time and currently_patched != module_settings:
         _queue_warning("Patching more than once will result in the union of all True"
                        " parameters being patched",
                        _warnings)
 
-    first_time = key not in saved
-    saved[key] = module_settings
-    return _warnings, first_time, module_settings
+    to_patch = {}
+    for k, v in module_settings.items():
+        # If we haven't seen the setting at all, record it and echo it.
+        # If we have seen the setting, but it became true, record it and echo it.
+        if k not in currently_patched:
+            to_patch[k] = currently_patched[k] = v
+        elif v and not currently_patched[k]:
+            to_patch[k] = currently_patched[k] = True
+
+    return _warnings, first_time, to_patch
 
 
 def _subscribe_signal_os(will_patch_all):
@@ -1053,7 +1063,6 @@ def _subscribe_signal_os(will_patch_all):
                        warnings)
 
 def patch_all(socket=True, dns=True, time=True, select=True, thread=True, os=True, ssl=True,
-              httplib=False, # Deprecated, to be removed.
               subprocess=True, sys=False, aggressive=True, Event=True,
               builtins=True, signal=True,
               queue=True,
@@ -1083,16 +1092,26 @@ def patch_all(socket=True, dns=True, time=True, select=True, thread=True, os=Tru
        for kwarg values to be interpreted by plugins, for example, `patch_all(mylib_futures=True)`.
     .. versionchanged:: 1.3.5
        Add *queue*, defaulting to True, for Python 3.7.
+    .. versionchanged:: 1.5
+       Remove the ``httplib`` argument. Previously, setting it raised a ``ValueError``.
+    .. versionchanged:: 1.5
+       Better handling of patching more than once.
     """
     # pylint:disable=too-many-locals,too-many-branches
 
     # Check to see if they're changing the patched list
     _warnings, first_time, modules_to_patch = _check_repatching(**locals())
-    if not _warnings and not first_time:
-        # Nothing to do, identical args to what we just
-        # did
+
+    if not modules_to_patch:
+        # Nothing to do. Either the arguments were identical to what
+        # we previously did, or they specified false values
+        # for things we had previously patched.
+        _process_warnings(_warnings)
         return
 
+    for k, v in modules_to_patch.items():
+        locals()[k] = v
+
     from gevent import events
     try:
         _notify_patch(events.GeventWillPatchAllEvent(modules_to_patch, kwargs), _warnings)
@@ -1116,8 +1135,6 @@ def patch_all(socket=True, dns=True, time=True, select=True, thread=True, os=Tru
         patch_select(aggressive=aggressive)
     if ssl:
         patch_ssl(_warnings=_warnings, _first_time=first_time)
-    if httplib:
-        raise ValueError('gevent.httplib is no longer provided, httplib must be False')
     if subprocess:
         patch_subprocess()
     if builtins:
@@ -1143,7 +1160,7 @@ def main():
     while argv and argv[0].startswith('--'):
         option = argv[0][2:]
         if option == 'verbose':
-            verbose = True
+            verbose += 1
         elif option == 'module':
             run_fn = "run_module"
         elif option.startswith('no-') and option.replace('no-', '') in patch_all_args:
@@ -1166,18 +1183,40 @@ def main():
         print('sys.modules=%s' % pprint.pformat(sorted(sys.modules.keys())))
         print('cwd=%s' % os.getcwd())
 
-    patch_all(**args)
-    if argv:
-        import runpy
-        sys.argv = argv
-        # Use runpy.run_path to closely (exactly) match what the
-        # interpreter does given 'python <path>'. This includes allowing
-        # passing .pyc/.pyo files and packages with a __main__ and
-        # potentially even zip files. Previously we used exec, which only
-        # worked if we directly read a python source file.
-        getattr(runpy, run_fn)(sys.argv[0], run_name='__main__')
-    else:
+    if not argv:
         print(script_help)
+        return
+
+    sys.argv[:] = argv
+    # Make sure that we don't get imported again under a different
+    # name (usually it's ``__main__`` here) because that could lead to
+    # double-patching, and making monkey.get_original() not work.
+    try:
+        mod_name = __spec__.name
+    except NameError:
+        # Py2: __spec__ is not defined as standard
+        mod_name = 'gevent.monkey'
+    sys.modules[mod_name] = sys.modules[__name__]
+    # On Python 2, we have to set the gevent.monkey attribute
+    # manually; putting gevent.monkey into sys.modules stops the
+    # import machinery from making that connection, and ``from gevent
+    # import monkey`` is broken. On Python 3 (.8 at least) that's not
+    # necessary.
+    if 'gevent' in sys.modules:
+        sys.modules['gevent'].monkey = sys.modules[mod_name]
+    # Running ``patch_all()`` will load pkg_resources entry point plugins
+    # which may attempt to import ``gevent.monkey``, so it is critical that
+    # we have established the correct saved module name first.
+    patch_all(**args)
+
+    import runpy
+    # Use runpy.run_path to closely (exactly) match what the
+    # interpreter does given 'python <path>'. This includes allowing
+    # passing .pyc/.pyo files and packages with a __main__ and
+    # potentially even zip files. Previously we used exec, which only
+    # worked if we directly read a python source file.
+    run_meth = getattr(runpy, run_fn)
+    return run_meth(sys.argv[0], run_name='__main__')
 
 
 def _get_script_help():
@@ -1193,12 +1232,12 @@ def _get_script_help():
 
 USAGE: ``python -m gevent.monkey [MONKEY OPTIONS] [--module] (script|module) [SCRIPT OPTIONS]``
 
-If no OPTIONS present, monkey patches all the modules it can patch.
+If no MONKEY OPTIONS are present, monkey patches all the modules as if by calling ``patch_all()``.
 You can exclude a module with --no-<module>, e.g. --no-thread. You can
 specify a module to patch with --<module>, e.g. --socket. In the latter
 case only the modules specified on the command line will be patched.
 
-The default behavior is to execute the script passed as argument. If you with
+The default behavior is to execute the script passed as argument. If you wish
 to run a module instead, pass the `--module` argument before the module name.
 
 .. versionchanged:: 1.3b1

src/gevent/testing/testcase.py

@@ -167,6 +167,10 @@ class SubscriberCleanupMixin(object):
         from gevent import events
         self.__old_subscribers = events.subscribers[:]
 
+    def addSubscriber(self, sub):
+        from gevent import events
+        events.subscribers.append(sub)
+
     def tearDown(self):
         from gevent import events
         events.subscribers[:] = self.__old_subscribers

src/gevent/tests/monkey_package/__init__.py

+# -*- coding: utf-8 -*-
+"""
+Make a package.
+
+"""
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function

src/gevent/tests/monkey_package/__main__.py

 from __future__ import print_function
-# This file makes this directory into a package.
+# This file makes this directory into a runnable package.
 # it exists to test 'python -m gevent.monkey monkey_package'
 print(__file__)
 print(__name__)

src/gevent/tests/monkey_package/threadpool_monkey_patches.py

+# -*- coding: utf-8 -*-
+"""
+This file runs ``gevent.monkey.patch_all()``.
+
+It is intended to be used by ``python -m gevent.monkey <this file>``
+to prove that monkey-patching twice doesn't have unfortunate sife effects (such as
+breaking the threadpool).
+"""
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+
+import sys
+from gevent import monkey
+from gevent import get_hub
+
+monkey.patch_all(thread=False, sys=True)
+
+def thread_is_greenlet():
+    from gevent.thread import get_ident as gr_ident
+    std_thread_mod = 'thread' if bytes is str else '_thread'
+    thr_ident = monkey.get_original(std_thread_mod, 'get_ident')
+    return thr_ident() == gr_ident()
+
+
+is_greenlet = get_hub().threadpool.apply(thread_is_greenlet)
+print(is_greenlet)
+print(len(sys._current_frames()))

src/gevent/tests/monkey_package/threadpool_no_monkey.py

+# -*- coding: utf-8 -*-
+"""
+This file *does not* run ``gevent.monkey.patch_all()``.
+
+It is intended to be used by ``python -m gevent.monkey <this file>``
+to prove that the threadpool and getting the original value of things
+works.
+"""
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+
+import sys
+from gevent import monkey
+from gevent import get_hub
+
+from gevent.thread import get_ident as gr_ident
+
+std_thread_mod = 'thread' if bytes is str else '_thread'
+thr_ident = monkey.get_original(std_thread_mod, 'get_ident')
+
+print(thr_ident is gr_ident)
+
+def thread_is_greenlet():
+    return thr_ident() == gr_ident()
+
+
+is_greenlet = get_hub().threadpool.apply(thread_is_greenlet)
+print(is_greenlet)
+print(len(sys._current_frames()))

src/gevent/tests/test__monkey.py

@@ -11,6 +11,22 @@ class TestMonkey(SubscriberCleanupMixin, unittest.TestCase):
 
     maxDiff = None
 
+    def setUp(self):
+        super(TestMonkey, self).setUp()
+
+        self.all_events = []
+        self.addSubscriber(self.all_events.append)
+        self.orig_saved = orig_saved = {}
+        for k, v in monkey.saved.items():
+            orig_saved[k] = v.copy()
+
+
+    def tearDown(self):
+        monkey.saved = self.orig_saved
+        del self.orig_saved
+        del self.all_events
+        super(TestMonkey, self).tearDown()
+
     def test_time(self):
         import time
         from gevent import time as gtime
@@ -70,49 +86,51 @@ class TestMonkey(SubscriberCleanupMixin, unittest.TestCase):
 
     def test_patch_twice_warnings_events(self):
         import warnings
-        from gevent.testing import verify
-
-        orig_saved = {}
-        for k, v in monkey.saved.items():
-            orig_saved[k] = v.copy()
-
-        from gevent import events
-        all_events = []
-        events.subscribers.append(all_events.append)
-
-        def veto(event):
-            if isinstance(event, events.GeventWillPatchModuleEvent) and event.module_name == 'ssl':
-                raise events.DoNotPatch
 
-        events.subscribers.append(veto)
+        all_events = self.all_events
 
         with warnings.catch_warnings(record=True) as issued_warnings:
-            # Patch again, triggering three warnings, one for os=False/signal=True,
-            # one for repeated monkey-patching, one for patching after ssl (on python >= 2.7.9)
+            # Patch again, triggering just one warning, for
+            # a different set of arguments. Because we're going to False instead of
+            # turning something on, nothing is actually done, no events are issued.
             monkey.patch_all(os=False, extra_kwarg=42)
-            self.assertGreaterEqual(len(issued_warnings), 2)
-            self.assertIn('SIGCHLD', str(issued_warnings[-1].message))
+            self.assertEqual(len(issued_warnings), 1)
             self.assertIn('more than once', str(issued_warnings[0].message))
+            self.assertEqual(all_events, [])
 
-            # Patching with the exact same argument doesn't issue a second warning.
-            # in fact, it doesn't do anything
+            # Same warning again, but still nothing is done.
             del issued_warnings[:]
             monkey.patch_all(os=False)
-            orig_saved['_gevent_saved_patch_all'] = monkey.saved['_gevent_saved_patch_all']
-
-            self.assertFalse(issued_warnings)
+            self.assertEqual(len(issued_warnings), 1)
+            self.assertIn('more than once', str(issued_warnings[0].message))
+            self.assertEqual(all_events, [])
+            self.orig_saved['_gevent_saved_patch_all_module_settings'] = monkey.saved[
+                '_gevent_saved_patch_all_module_settings']
 
         # Make sure that re-patching did not change the monkey.saved
         # attribute, overwriting the original functions.
-        if 'logging' in monkey.saved and 'logging' not in orig_saved:
+        if 'logging' in monkey.saved and 'logging' not in self.orig_saved:
             # some part of the warning or unittest machinery imports logging
-            orig_saved['logging'] = monkey.saved['logging']
-        self.assertEqual(orig_saved, monkey.saved)
+            self.orig_saved['logging'] = monkey.saved['logging']
+        self.assertEqual(self.orig_saved, monkey.saved)
 
         # Make sure some problematic attributes stayed correct.
         # NOTE: This was only a problem if threading was not previously imported.
         for k, v in monkey.saved['threading'].items():
-            self.assertNotIn('gevent', str(v))
+            self.assertNotIn('gevent', str(v), (k, v))
+
+    def test_patch_events(self):
+        from gevent import events
+        from gevent.testing import verify
+        all_events = self.all_events
+
+        def veto(event):
+            if isinstance(event, events.GeventWillPatchModuleEvent) and event.module_name == 'ssl':
+                raise events.DoNotPatch
+        self.addSubscriber(veto)
+
+        monkey.saved = {} # Reset
+        monkey.patch_all(thread=False, select=False, extra_kwarg=42) # Go again
 
         self.assertIsInstance(all_events[0], events.GeventWillPatchAllEvent)
         self.assertEqual({'extra_kwarg': 42}, all_events[0].patch_all_kwargs)

src/gevent/tests/test__monkey_scope.py → src/gevent/tests/test__monkey_module_run.py

+"""
+Tests for running ``gevent.monkey`` as a module to launch a
+patched script.
+
+Uses files in the ``monkey_package/`` directory.
+"""
+from __future__ import print_function
+from __future__ import absolute_import
+from __future__ import division
+
+
 import os
 import os.path
 import sys
@@ -25,8 +36,8 @@ class TestRun(unittest.TestCase):
             args.append('--module')
         args += [script, 'patched']
         p = Popen(args, stdout=PIPE, stderr=PIPE, env=env)
-        gout, gerr = p.communicate()
-        self.assertEqual(0, p.returncode, (gout, gerr))
+        monkey_out, monkey_err = p.communicate()
+        self.assertEqual(0, p.returncode, (monkey_out, monkey_err))
 
         if module:
             args = [sys.executable, "-m", script, 'stdlib']
@@ -34,32 +45,32 @@ class TestRun(unittest.TestCase):
             args = [sys.executable, script, 'stdlib']
         p = Popen(args, stdout=PIPE, stderr=PIPE)
 
-        pout, perr = p.communicate()
-        self.assertEqual(0, p.returncode, (pout, perr))
+        std_out, std_err = p.communicate()
+        self.assertEqual(0, p.returncode, (std_out, std_err))
 
-        glines = gout.decode("utf-8").splitlines()
-        plines = pout.decode('utf-8').splitlines()
-        self.assertEqual(glines, plines)
-        self.assertEqual(gerr, perr)
+        monkey_out_lines = monkey_out.decode("utf-8").splitlines()
+        std_out_lines = std_out.decode('utf-8').splitlines()
+        self.assertEqual(monkey_out_lines, std_out_lines)
+        self.assertEqual(monkey_err, std_err)
 
-        return glines, gerr
+        return monkey_out_lines, monkey_err
 
     def test_run_simple(self):
         self._run(os.path.join('monkey_package', 'script.py'))
 
-    def test_run_package(self):
-        # Run a __main__ inside a package.
-        lines, _ = self._run('monkey_package')
+    def _run_package(self, module):
+        lines, _ = self._run('monkey_package', module=module)
 
         self.assertTrue(lines[0].endswith('__main__.py'), lines[0])
         self.assertEqual(lines[1], '__main__')
 
-    def test_run_module(self):
-        # Run a __main__ inside a module
-        lines, _ = self._run('monkey_package', module=True)
+    def test_run_package(self):
+        # Run a __main__ inside a package, even without specifying -m
+        self._run_package(module=False)
 
-        self.assertTrue(lines[0].endswith('__main__.py'), lines[0])
-        self.assertEqual(lines[1], '__main__')
+    def test_run_module(self):
+        # Run a __main__ inside a package, when specifying -m
+        self._run_package(module=True)
 
     def test_issue_302(self):
         lines, _ = self._run(os.path.join('monkey_package', 'issue302monkey.py'))
@@ -69,6 +80,26 @@ class TestRun(unittest.TestCase):
         self.assertEqual(lines[1], 'monkey_package/issue302monkey.py')
         self.assertEqual(lines[2], 'True', lines)
 
+    def test_threadpool_in_patched_after_patch(self):
+        # Issue 1484
+        # If we don't have this correct, then we get exceptions
+        out, err = self._run(os.path.join('monkey_package', 'threadpool_monkey_patches.py'))
+        self.assertEqual(out, ['False', '2'])
+        self.assertEqual(err, b'')
+
+    def test_threadpool_in_patched_after_patch_module(self):
+        # Issue 1484
+        # If we don't have this correct, then we get exceptions
+        out, err = self._run('monkey_package.threadpool_monkey_patches', module=True)
+        self.assertEqual(out, ['False', '2'])
+        self.assertEqual(err, b'')
+
+    def test_threadpool_not_patched_after_patch_module(self):
+        # Issue 1484
+        # If we don't have this correct, then we get exceptions
+        out, err = self._run('monkey_package.threadpool_no_monkey', module=True)
+        self.assertEqual(out, ['False', 'False', '2'])
+        self.assertEqual(err, b'')
 
 if __name__ == '__main__':
     unittest.main()