Merge pull request #858 from gevent/double-subproc

Allow resetting the C state for whether libev child handlers are active.

changelog.rst

@@ -90,6 +90,16 @@ Stdlib Compatibility
   returning the errno due to the refactoring of the exception
   hierarchy in Python 3.3. Now the errno is returned. Reported in
   :issue:`841` by Dana Powers.
+- Setting SIGCHLD to SIG_IGN or SIG_DFL after :mod:`gevent.subprocess`
+  had been used previously could not be reversed, causing
+  ``Popen.wait`` and other calls to hang. Now, if SIGCHLD has been
+  ignored, the next time :mod:`gevent.subprocess` is used this will be
+  detected and corrected automatically. (This potentially leads to
+  issues with :func:`os.popen` on Python 2, but the signal can always
+  be reset again. Mixing the low-level process handling calls,
+  low-level signal management and high-level use of
+  :mod:`gevent.subprocess` is tricky.) Reported in :issue:`857` by
+  Chris Utz.
 
 Other Changes
 -------------

src/gevent/libev/_corecffi_cdef.c

@@ -206,6 +206,7 @@ unsigned int ev_pending_count(struct ev_loop*);
 
 struct ev_loop* gevent_ev_default_loop(unsigned int flags);
 void gevent_install_sigchld_handler();
+void gevent_reset_sigchld_handler();
 
 void (*gevent_noop)(struct ev_loop *_loop, struct ev_timer *w, int revents);
 void ev_sleep (ev_tstamp delay); /* sleep for a while */

src/gevent/libev/corecext.ppyx

@@ -504,6 +504,9 @@ cdef public class loop [object PyGeventLoopObject, type PyGeventLoop_Type]:
     def install_sigchld(self):
         libev.gevent_install_sigchld_handler()
 
+    def reset_sigchld(self):
+        libev.gevent_reset_sigchld_handler()
+
 #endif
 
     def stat(self, str path, float interval=0.0, ref=True, priority=None):

src/gevent/libev/corecffi.py

@@ -608,6 +608,9 @@ class loop(object):
         def install_sigchld(self):
             libev.gevent_install_sigchld_handler()
 
+        def reset_sigchld(self):
+            libev.gevent_reset_sigchld_handler()
+
     def stat(self, path, interval=0.0, ref=True, priority=None):
         return stat(self, path, interval, ref, priority)
 

src/gevent/libev/libev.h

@@ -12,6 +12,14 @@
 #ifndef _WIN32
 
 static struct sigaction libev_sigchld;
+/*
+ * Track the state of whether we have installed
+ * the libev sigchld handler specifically.
+ * If it's non-zero, libev_sigchld will be valid and set to the action
+ * that libev needs to do.
+ * If it's 1, we need to install libev_sigchld to make libev
+ * child handlers work (on request).
+ */
 static int sigchld_state = 0;
 
 static struct ev_loop* gevent_ev_default_loop(unsigned int flags)
@@ -22,9 +30,12 @@ static struct ev_loop* gevent_ev_default_loop(unsigned int flags)
     if (sigchld_state)
         return ev_default_loop(flags);
 
+    //  Request the old SIGCHLD handler
     sigaction(SIGCHLD, NULL, &tmp);
+    // Get the loop, which will install a SIGCHLD handler
     result = ev_default_loop(flags);
     // XXX what if SIGCHLD received there?
+    // Now restore the previous SIGCHLD handler
     sigaction(SIGCHLD, &tmp, &libev_sigchld);
     sigchld_state = 1;
     return result;
@@ -38,6 +49,15 @@ static void gevent_install_sigchld_handler(void) {
     }
 }
 
+static void gevent_reset_sigchld_handler(void) {
+   // We could have any state at this point, depending on
+   // whether the default loop has been used. If it has,
+   // then always be in state 1 ("need to install)
+   if (sigchld_state) {
+       sigchld_state = 1;
+   }
+}
+
 #else
 
 #define gevent_ev_default_loop ev_default_loop

src/gevent/libev/libev.pxd

@@ -205,3 +205,4 @@ cdef extern from "libev.h":
 
     ev_loop* gevent_ev_default_loop(unsigned int flags)
     void gevent_install_sigchld_handler()
+    void gevent_reset_sigchld_handler()

src/gevent/signal.py

@@ -15,6 +15,7 @@ information on configuring this not to be the case for advanced uses.
 """
 
 from __future__ import absolute_import
+
 from gevent._util import _NONE as _INITIAL
 from gevent._util import copy_globals
 
@@ -34,6 +35,9 @@ def getsignal(signalnum):
     """
     Exactly the same as :func:`signal.signal` except where
     :const:`signal.SIGCHLD` is concerned.
+
+    For :const:`signal.SIGCHLD`, this cooperates with :func:`signal`
+    to provide consistent answers.
     """
     if signalnum != _signal.SIGCHLD:
         return _signal_getsignal(signalnum)
@@ -67,9 +71,16 @@ def signal(signalnum, handler):
         Use of ``SIG_IGN`` and ``SIG_DFL`` may also have race conditions
         with libev child watchers and the :mod:`gevent.subprocess` module.
 
+    .. versionchanged:: 1.2a1
+         If ``SIG_IGN`` or ``SIG_DFL`` are used to ignore ``SIGCHLD``, a
+         future use of ``gevent.subprocess`` and libev child watchers
+         will once again work. However, on Python 2, use of ``os.popen``
+         will fail.
+
     .. versionchanged:: 1.1rc2
-       Allow using ``SIG_IGN`` and ``SIG_DFL`` to reset and ignore ``SIGCHLD``.
-       However, this allows the possibility of a race condition.
+         Allow using ``SIG_IGN`` and ``SIG_DFL`` to reset and ignore ``SIGCHLD``.
+         However, this allows the possibility of a race condition if ``gevent.subprocess``
+         had already been used.
     """
     if signalnum != _signal.SIGCHLD:
         return _signal_signal(signalnum, handler)
@@ -87,8 +98,11 @@ def signal(signalnum, handler):
     if handler == _signal.SIG_IGN or handler == _signal.SIG_DFL:
         # Allow resetting/ignoring this signal at the process level.
         # Note that this conflicts with gevent.subprocess and other users
-        # of child watchers.
+        # of child watchers, until the next time gevent.subprocess/loop.install_sigchld()
+        # is called.
+        from gevent import get_hub # Are we always safe to import here?
         _signal_signal(signalnum, handler)
+        get_hub().loop.reset_sigchld()
     return old_handler
 
 

src/greentest/test__monkey_sigchld_3.py

+# Mimics what gunicorn workers do *if* the arbiter is also monkey-patched:
+# After forking from the master monkey-patched process, the child
+# resets signal handlers to SIG_DFL. If we then fork and watch *again*,
+# we shouldn't hang. (Note that we carefully handle this so as not to break
+# os.popen)
+from __future__ import print_function
+# Patch in the parent process.
+import gevent.monkey
+gevent.monkey.patch_all()
+
+from gevent import get_hub
+
+import os
+import sys
+
+import signal
+import subprocess
+
+def _waitpid(pid):
+    try:
+        _, stat = os.waitpid(pid, 0)
+    except OSError:
+        # Interrupted system call
+        _, stat = os.waitpid(pid, 0)
+    assert stat == 0, stat
+
+if hasattr(signal, 'SIGCHLD'):
+    # Do what subprocess does and make sure we have the watcher
+    # in the parent
+    get_hub().loop.install_sigchld()
+
+
+    pid = os.fork()
+
+    if pid: # parent
+        _waitpid(pid)
+    else:
+        # Child resets.
+        signal.signal(signal.SIGCHLD, signal.SIG_DFL)
+
+        # Go through subprocess because we expect it to automatically
+        # set up the waiting for us.
+        popen = subprocess.Popen([sys.executable, '-c', 'import sys'],
+                                 stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        popen.stderr.read()
+        popen.stdout.read()
+        popen.wait() # This hangs if it doesn't.
+
+
+        sys.exit(0)
+else:
+    print("No SIGCHLD, not testing")