Merge pull request #758 from gevent/monkey-signal+os-warnings

Issue some warnings for patch_all with invalid parameters

changelog.rst

@@ -30,6 +30,16 @@
   requires having Cython installed first. (Note that the binary installation
   formats (wheels, exes, msis) are preferred on Windows.) Reported in
   :issue:`757` by Ned Batchelder.
+- Issue a warning when :func:`~gevent.monkey.patch_all` is called with
+  ``os`` set to False (*not* the default) but ``signal`` is still True
+  (the default). This combination of parameters will cause signal
+  handlers for ``SIGCHLD`` to not get called. In the future this might
+  raise an error. Reported by Josh Zuech.
+- Issue a warning when :func:`~gevent.monkey.patch_all` is called more
+  than once with different arguments. That causes the cumulative set of all True
+  arguments to be patched, which may cause unexpected results.
+- Fix returning the original values of certain ``threading``
+  attributes from :func:`gevent.monkey.get_original`.
 
 .. _bug 13502: http://bugs.python.org/issue13502
 

doc/whatsnew_1_1.rst

@@ -119,6 +119,11 @@ downstream libraries, notably `gunicorn`_.
   :func:`gevent.os.waitpid` (again monkey patched by default) and
   :func:`gevent.signal.signal` (which is monkey patched only for the
   :data:`signal.SIGCHLD` case). The latter two patches are new in 1.1.
+- In gevent 1.0, use of libev child watchers (which are used
+  internally by ``gevent.subprocess``) had race conditions with
+  user-provided ``SIGCHLD`` handlers, causing many types of
+  unpredictable breakage. The two new APIs described above are
+  intended to rectify this.
 - Fork-watchers will be called, even in multi-threaded programs
   (except on Windows).
 - The default threadpool and threaded resolver work in child
@@ -138,6 +143,12 @@ possible in a monkey patched system, at least on POSIX platforms.
 .. caution:: It is not possible to use :mod:`gevent.subprocess` from
              native threads. See :mod:`gevent.subprocess` for details.
 
+.. note:: If the ``SIGCHLD`` signal is to be handled, it is important
+          to monkey patch (or directly use) both :mod:`os` and
+          :mod:`signal`; this is the default for
+          :func:`~gevent.monkey.patch_all`. Failure to do so can
+          result in the ``SIGCHLD`` signal being lost.
+
 .. tip:: All of the above entail forking a child process. Forking
 		 a child process that uses gevent, greenlets, and libev
 		 can have some unexpected consequences if the child

gevent/monkey.py

@@ -89,8 +89,8 @@ if sys.platform.startswith("win"):
 else:
     WIN = False
 
-# maps module name -> attribute name -> original item
-# e.g. "time" -> "sleep" -> built-in function sleep
+# maps module name -> {attribute name: original item}
+# e.g. "time" -> {"sleep": built-in function sleep}
 saved = {}
 
 
@@ -161,6 +161,23 @@ def patch_module(name, items=None):
             raise AttributeError('%r does not have __implements__' % gevent_module)
     for attr in items:
         patch_item(module, attr, getattr(gevent_module, attr))
+    return module
+
+
+def _queue_warning(message, _warnings):
+    # Queues a warning to show after the monkey-patching process is all done.
+    # Done this way to avoid extra imports during the process itself, just
+    # in case. If we're calling a function one-off (unusual) go ahead and do it
+    if _warnings is None:
+        _process_warnings([message])
+    else:
+        _warnings.append(message)
+
+
+def _process_warnings(_warnings):
+    import warnings
+    for warning in _warnings:
+        warnings.warn(warning, RuntimeWarning, stacklevel=3)
 
 
 def _patch_sys_std(name):
@@ -202,6 +219,9 @@ def patch_os():
     :func:`os.waitpid` with :func:`gevent.os.waitpid` (if the
     environment variable ``GEVENT_NOWAITPID`` is not defined). Does
     nothing if fork is not available.
+
+    This method must be used with :func:`patch_signal` to have proper SIGCHLD
+    handling. :func:`patch_all` calls both by default.
     """
     patch_module('os')
 
@@ -252,7 +272,8 @@ def _patch_existing_locks(threading):
 
 
 def patch_thread(threading=True, _threading_local=True, Event=False, logging=True,
-                 existing_locks=True):
+                 existing_locks=True,
+                 _warnings=None):
     """
     Replace the standard :mod:`thread` module to make it greenlet-based.
 
@@ -293,10 +314,20 @@ def patch_thread(threading=True, _threading_local=True, Event=False, logging=Tru
     #   return r, w
     # os.pipe = _pipe
 
+    # The 'threading' module copies some attributes from the
+    # thread module the first time it is imported. If we patch 'thread'
+    # before that happens, then we store the wrong values in 'saved',
+    # So if we're going to patch threading, we either need to import it
+    # before we patch thread, or manually clean up the attributes that
+    # are in trouble. The latter is tricky because of the different names
+    # on different versions.
+    if threading:
+        __import__('threading')
+
     patch_module('thread')
     if threading:
-        patch_module('threading')
-        threading = __import__('threading')
+        threading = patch_module('threading')
+
         if Event:
             from gevent.event import Event
             patch_item(threading, 'Event', Event)
@@ -348,11 +379,20 @@ def patch_thread(threading=True, _threading_local=True, Event=False, logging=Tru
                         sleep(0.01)
 
             main_thread.join = join
+
+            # Patch up the ident of the main thread to match. This
+            # matters if threading was imported before monkey-patching
+            # thread
+            oldid = main_thread.ident
+            main_thread._ident = threading.get_ident()
+            if oldid in threading._active:
+                threading._active[main_thread.ident] = threading._active[oldid]
+            if oldid != main_thread.ident:
+                del threading._active[oldid]
         else:
-            # TODO: Can we use warnings here or does that mess up monkey patching?
-            print("Monkey-patching not on the main thread; "
-                  "threading.main_thread().join() will hang from a greenlet",
-                  file=sys.stderr)
+            _queue_warning("Monkey-patching not on the main thread; "
+                           "threading.main_thread().join() will hang from a greenlet",
+                           _warnings)
 
 
 def patch_socket(dns=True, aggressive=True):
@@ -478,15 +518,50 @@ def patch_signal():
     """
     Make the signal.signal function work with a monkey-patched os.
 
+    This method must be used with :func:`patch_os` to have proper SIGCHLD
+    handling. :func:`patch_all` calls both by default.
+
     .. seealso:: :mod:`gevent.signal`
     """
     patch_module("signal")
 
 
+def _check_repatching(**module_settings):
+    _warnings = []
+    key = '_gevent_saved_patch_all'
+    if saved.get(key, module_settings) != 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
+
+
 def patch_all(socket=True, dns=True, time=True, select=True, thread=True, os=True, ssl=True, httplib=False,
               subprocess=True, sys=False, aggressive=True, Event=False,
               builtins=True, signal=True):
-    """Do all of the default monkey patching (calls every other applicable function in this module)."""
+    """
+    Do all of the default monkey patching (calls every other applicable
+    function in this module).
+
+    .. versionchanged:: 1.1
+       Issue a :mod:`warning <warnings>` if this function is called multiple times
+       with different arguments. The second and subsequent calls will only add more
+       patches, they can never remove existing patches by setting an argument to ``False``.
+    .. versionchanged:: 1.1
+       Issue a :mod:`warning <warnings>` if this function is called with ``os=False``
+       and ``signal=True``. This will cause SIGCHLD handlers to not be called. This may
+       be an error in the future.
+    """
+    # Check to see if they're changing the patched list
+    _warnings, first_time = _check_repatching(**locals())
+    if not _warnings and not first_time:
+        # Nothing to do, identical args to what we just
+        # did
+        return
+
     # order is important
     if os:
         patch_os()
@@ -511,8 +586,16 @@ def patch_all(socket=True, dns=True, time=True, select=True, thread=True, os=Tru
     if builtins:
         patch_builtins()
     if signal:
+        if not os:
+            _queue_warning('Patching signal but not os will result in SIGCHLD handlers'
+                           ' installed after this not being called and os.waitpid may not'
+                           ' function correctly if gevent.subprocess is used. This may raise an'
+                           ' error in the future.',
+                           _warnings)
         patch_signal()
 
+    _process_warnings(_warnings)
+
 
 def main():
     args = {}

gevent/os.py

@@ -210,6 +210,7 @@ if hasattr(os, 'fork'):
         _waitpid = os.waitpid
         _WNOHANG = os.WNOHANG
 
+        # replaced by the signal module.
         _on_child_hook = lambda: None
 
         # {pid -> watcher or tuple(pid, rstatus, timestamp)}

gevent/subprocess.py

@@ -45,7 +45,7 @@ __implements__ = [
     'check_call',
     'check_output',
 ]
-if PY3:
+if PY3 and not sys.platform.startswith('win32'):
     __implements__.append("_posixsubprocess")
     _posixsubprocess = None
 

greentest/test__monkey.py

@@ -39,3 +39,33 @@ for modname in monkey.saved:
 
     for objname in monkey.saved[modname]:
         assert monkey.is_object_patched(modname, objname)
+
+orig_saved = {}
+for k, v in monkey.saved.items():
+    orig_saved[k] = v.copy()
+
+import warnings
+with warnings.catch_warnings(record=True) as issued_warnings:
+    # Patch again, triggering two warnings, on for os=False/signal=True,
+    # one for repeated monkey-patching.
+    monkey.patch_all(os=False)
+    assert len(issued_warnings) == 2, len(issued_warnings)
+    assert 'SIGCHLD' in str(issued_warnings[-1].message), issued_warnings[-1]
+    assert 'more than once' in str(issued_warnings[0].message), issued_warnings[0]
+
+    # Patching with the exact same argument doesn't issue a second warning.
+    # in fact, it doesn't do anything
+    del issued_warnings[:]
+    monkey.patch_all(os=False)
+    orig_saved['_gevent_saved_patch_all'] = monkey.saved['_gevent_saved_patch_all']
+
+    assert len(issued_warnings) == 0, len(issued_warnings)
+
+# Make sure that re-patching did not change the monkey.saved
+# attribute, overwriting the original functions.
+assert orig_saved == monkey.saved, (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():
+    assert 'gevent' not in str(v), (k, v)