Merge pull request #645 from gevent/sigchld-patcher

Patch signal.signal(SIGCHLD) to cooperate with child watchers

changelog.rst

@@ -14,6 +14,9 @@
   erratic, difficult to debug behaviour.
 - Fix an ``AttributeError`` from ``gevent.queue.Queue`` when ``peek``
   was called on an empty ``Queue``. Reported in :issue:`643` by michaelvol.
+- Make ``SIGCHLD`` handlers specified to ``signal.signal`` work with
+  the child watchers that are used by default. Also make
+  ``os.waitpid`` work with a first argument of -1.
 
 1.1b3 (Aug 16, 2015)
 ====================

doc/reference.rst

@@ -13,6 +13,7 @@ API reference
    gevent.local
    gevent.monkey
    gevent.os
+   gevent.signal
    gevent.pool
    gevent.queue
    gevent.server

gevent/hub.py

@@ -191,6 +191,11 @@ class signal(object):
 
     This returns an object with the useful method ``cancel``, which, when called,
     will prevent future deliveries of *signalnum* from calling *handler*.
+
+    .. note::
+
+        This may not operate correctly with SIGCHLD if libev child watchers
+        are used (as they are by default with os.fork).
     """
 
     greenlet_class = None

gevent/monkey.py

@@ -13,6 +13,7 @@ __all__ = ['patch_all',
            'patch_thread',
            'patch_subprocess',
            'patch_sys',
+           'patch_signal',
            # query functions
            'get_original',
            'is_module_patched',
@@ -356,9 +357,18 @@ def patch_builtins():
         patch_module('builtins')
 
 
+def patch_signal():
+    """
+    Make the signal.signal function work with a monkey-patched os.
+
+    .. seealso:: :mod:`gevent.signal`
+    """
+    patch_module("signal")
+
+
 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):
+              builtins=True, signal=True):
     """Do all of the default monkey patching (calls every other applicable function in this module)."""
     # order is important
     if os:
@@ -383,6 +393,8 @@ def patch_all(socket=True, dns=True, time=True, select=True, thread=True, os=Tru
         patch_subprocess()
     if builtins:
         patch_builtins()
+    if signal:
+        patch_signal()
 
 
 if __name__ == '__main__':

gevent/os.py

@@ -180,6 +180,8 @@ if hasattr(os, 'fork'):
         _waitpid = os.waitpid
         _WNOHANG = os.WNOHANG
 
+        _on_child_hook = lambda: None
+
         # {pid -> watcher or tuple(pid, rstatus, timestamp)}
         _watched_children = {}
 
@@ -190,6 +192,8 @@ if hasattr(os, 'fork'):
             _watched_children[watcher.pid] = (watcher.pid, watcher.rstatus, time.time())
             if callback:
                 callback(watcher)
+            # dispatch an "event"; used by gevent.signal.signal
+            _on_child_hook()
             # now is as good a time as any to reap children
             _reap_children()
 
@@ -234,8 +238,18 @@ if hasattr(os, 'fork'):
             """
             # XXX Does not handle tracing children
             if pid <= 0:
-                # magic functions for multiple children. Pass.
-                return _waitpid(pid, options)
+                # magic functions for multiple children.
+                if pid == -1:
+                    # Any child. If we have one that we're watching and that finished,
+                    # we need to use that one. Otherwise, let the OS take care of it.
+                    for k, v in _watched_children.items():
+                        if isinstance(v, tuple):
+                            pid = k
+                            break
+                if pid <= 0:
+                    # If we didn't find anything, go to the OS. Otherwise,
+                    # handle waiting
+                    return _waitpid(pid, options)
 
             if pid in _watched_children:
                 # yes, we're watching it

gevent/signal.py

+"""
+Cooperative implementation of special cases of :func:`signal.signal`.
+
+This module is designed to work with libev's child watchers, as used by
+default in :func:`gevent.os.fork` Note that each SIGCHLD handler will be run
+in a new greenlet when the signal is delivered (just like :class:`gevent.hub.signal`)
+
+.. versionadded:: 1.1b4
+"""
+
+from __future__ import absolute_import
+
+import signal as _signal
+
+__implements__ = []
+__extensions__ = []
+
+
+_INITIAL = object()
+
+_child_handler = _INITIAL
+
+_signal_signal = _signal.signal
+_signal_getsignal = _signal.getsignal
+
+
+def getsignal(signalnum):
+    if signalnum != _signal.SIGCHLD:
+        return _signal_getsignal(signalnum)
+
+    global _child_handler
+    if _child_handler is _INITIAL:
+        _child_handler = _signal_getsignal(_signal.SIGCHLD)
+
+    return _child_handler
+
+
+def signal(signalnum, handler):
+    if signalnum != _signal.SIGCHLD:
+        return _signal_signal(signalnum, handler)
+
+    # TODO: raise value error if not called from the main
+    # greenlet, just like threads
+
+    if handler != _signal.SIG_IGN and handler != _signal.SIG_DFL and not callable(handler):
+        raise TypeError("signal handler must be signal.SIG_IGN, signal.SIG_DFL, or a callable object")
+
+    old_handler = getsignal(signalnum)
+    global _child_handler
+    _child_handler = handler
+    return old_handler
+
+
+def _on_child_hook():
+    # This is called in the hub greenlet. To let the function
+    # do more useful work, like use blocking functions,
+    # we run it in a new greenlet; see gevent.hub.signal
+    if callable(_child_handler):
+        # None is a valid value for the frame argument
+        from gevent import Greenlet
+        greenlet = Greenlet(_child_handler, _signal.SIGCHLD, None)
+        greenlet.switch()
+
+
+import gevent.os
+
+if 'waitpid' in gevent.os.__implements__ and hasattr(_signal, 'SIGCHLD'):
+    # Tightly coupled here to gevent.os and its waitpid implementation
+    gevent.os._on_child_hook = _on_child_hook
+    __implements__.append("signal")
+    __implements__.append("getsignal")
+else:
+    __extensions__.append("signal")
+    __extensions__.append("getsignal")
+
+__all__ = __implements__ + __extensions__

greentest/test__all__.py

@@ -7,15 +7,18 @@ import types
 from greentest import walk_modules
 
 
-MAPPING = {'gevent.local': '_threading_local',
-           'gevent.socket': 'socket',
-           'gevent.select': 'select',
-           'gevent.ssl': 'ssl',
-           'gevent.thread': '_thread' if six.PY3 else 'thread',
-           'gevent.subprocess': 'subprocess',
-           'gevent.os': 'os',
-           'gevent.threading': 'threading',
-           'gevent.builtins': 'builtins' if six.PY3 else '__builtin__', }
+MAPPING = {
+    'gevent.local': '_threading_local',
+    'gevent.socket': 'socket',
+    'gevent.select': 'select',
+    'gevent.ssl': 'ssl',
+    'gevent.thread': '_thread' if six.PY3 else 'thread',
+    'gevent.subprocess': 'subprocess',
+    'gevent.os': 'os',
+    'gevent.threading': 'threading',
+    'gevent.builtins': 'builtins' if six.PY3 else '__builtin__',
+    'gevent.signal': 'signal',
+}
 
 
 class ANY(object):
@@ -30,7 +33,9 @@ NOT_IMPLEMENTED = {
     'select': ANY,
     'os': ANY,
     'threading': ANY,
-    'builtins' if six.PY3 else '__builtin__': ANY, }
+    'builtins' if six.PY3 else '__builtin__': ANY,
+    'signal': ANY,
+}
 
 COULD_BE_MISSING = {
     'socket': ['create_connection', 'RAND_add', 'RAND_egd', 'RAND_status']}

greentest/test__monkey_sigchld.py

+import errno
+import os
+import sys
+#os.environ['GEVENT_NOWAITPID'] = 'True'
+
+import gevent
+import gevent.monkey
+gevent.monkey.patch_all()
+
+pid = None
+awaiting_child = [True]
+
+
+def handle_sigchld(*args):
+    # Make sure we can do a blocking operation
+    gevent.sleep()
+    # Signal completion
+    awaiting_child.pop()
+    # Raise an ignored error
+    raise TypeError("This should be ignored but printed")
+
+import signal
+if hasattr(signal, 'SIGCHLD'):
+    assert signal.getsignal(signal.SIGCHLD) == signal.SIG_DFL
+    signal.signal(signal.SIGCHLD, handle_sigchld)
+    handler = signal.getsignal(signal.SIGCHLD)
+    assert signal.getsignal(signal.SIGCHLD) is handle_sigchld, handler
+
+    pid = os.fork()
+    if not pid:
+        # child
+        gevent.sleep(0.2)
+        sys.exit(0)
+    else:
+        with gevent.Timeout(1):
+            while awaiting_child:
+                gevent.sleep(0.01)
+            # We should now be able to waitpid() for an arbitrary child
+            wpid, status = os.waitpid(-1, os.WNOHANG)
+            assert wpid == pid
+            # And a second call should raise ECHILD
+            try:
+                wpid, status = os.waitpid(-1, os.WNOHANG)
+                raise AssertionError("Should not be able to wait again")
+            except OSError as e:
+                assert e.errno == errno.ECHILD
+            sys.exit(0)
+else:
+    print("No SIGCHLD, not testing")