Fix #878 (#879)

* Fix #878

We can use multiple watchers to avoid any race conditions.

* Specific test cases for lines in os.waitpid.

* Less strict on the test since we may be running in parallel.

* make the leakchecker happy.

changelog.rst

@@ -159,6 +159,8 @@ Other Changes
 
   .. note:: Writing to the *io* property of a FileObject should be
             considered deprecated after it is constructed.
+- The :func:`gevent.os.waitpid` function is cooperative in more
+  circumstances. Reported in :issue:`878` by Heungsub Lee.
 
 1.1.2 (Jul 21, 2016)
 ====================

src/gevent/os.py

@@ -260,30 +260,53 @@ if hasattr(os, 'fork'):
             """
             Wait for a child process to finish.
 
-            If the child process was spawned using :func:`fork_and_watch`, then this
-            function behaves cooperatively. If not, it *may* have race conditions; see
+            If the child process was spawned using
+            :func:`fork_and_watch`, then this function behaves
+            cooperatively. If not, it *may* have race conditions; see
             :func:`fork_gevent` for more information.
 
-            The arguments are as for the underlying :func:`os.waitpid`. Some combinations
-            of *options* may not be supported (as of 1.1 that includes WUNTRACED).
+            The arguments are as for the underlying
+            :func:`os.waitpid`. Some combinations of *options* may not
+            be supported cooperatively (as of 1.1 that includes
+            WUNTRACED). Using a *pid* of 0 to request waiting on only processes
+            from the current process group is not cooperative.
 
             Availability: POSIX.
 
             .. versionadded:: 1.1b1
+            .. versionchanged:: 1.2a1
+               More cases are handled in a cooperative manner.
             """
             # XXX Does not handle tracing children
+
+            # So long as libev's loop doesn't run, it's OK to add
+            # child watchers. The SIGCHLD handler only feeds events
+            # for the next iteration of the loop to handle. (And the
+            # signal handler itself is only called from the next loop
+            # iteration.)
+
             if pid <= 0:
                 # 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.
+                    # we will 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
+                    # We didn't have one that was ready. If there are
+                    # no funky options set, and the pid was -1
+                    # (meaning any process, not 0, which means process
+                    # group--- libev doesn't know about process
+                    # groups) then we can use a child watcher of pid 0; otherwise,
+                    # pass through to the OS.
+                    if pid == -1 and options == 0:
+                        hub = get_hub()
+                        watcher = hub.loop.child(0, False)
+                        hub.wait(watcher)
+                        return watcher.rpid, watcher.rstatus
+                    # There were funky options/pid, so we must go to the OS.
                     return _waitpid(pid, options)
 
             if pid in _watched_children:
@@ -300,17 +323,24 @@ if hasattr(os, 'fork'):
                     # it's not finished
                     return (0, 0)
                 else:
-                    # we should block. Let the underlying OS call block; it should
-                    # eventually die with OSError, depending on signal delivery
-                    try:
-                        return _waitpid(pid, options)
-                    except OSError:
-                        if pid in _watched_children and isinstance(_watched_children, tuple):
-                            result = _watched_children[pid]
-                            del _watched_children[pid]
-                            return result[:2]
-                        raise
-            # we're not watching it
+                    # Ok, we need to "block". Do so via a watcher so that we're
+                    # cooperative. We know it's our child, etc, so this should work.
+                    watcher = _watched_children[pid]
+                    # We can't start a watcher that's already started,
+                    # so we can't reuse the existing watcher.
+                    new_watcher = watcher.loop.child(pid, False)
+                    get_hub().wait(new_watcher)
+                    # Ok, so now the new watcher is done. That means
+                    # the old watcher's callback (_on_child) should
+                    # have fired, potentially taking this child out of
+                    # _watched_children (but that could depend on how
+                    # many callbacks there were to run, so use the
+                    # watcher object directly; libev sets all the
+                    # watchers at the same time).
+                    return watcher.rpid, watcher.rstatus
+
+            # we're not watching it and it may not even  be our child,
+            # so we must go to the OS to be sure to get the right semantics (exception)
             return _waitpid(pid, options)
 
         def fork_and_watch(callback=None, loop=None, ref=False, fork=fork_gevent):

src/greentest/test__os.py

 import sys
 import _six as six
 from os import pipe
+import gevent
 from gevent import os
 from greentest import TestCase, main
 from gevent import Greenlet, joinall
-try:
-    import fcntl
-except ImportError:
-    fcntl = None
-
-try:
-    import errno
-except ImportError:
-    errno = None
 
 
 class TestOS_tp(TestCase):
@@ -87,5 +79,29 @@ if hasattr(os, 'make_nonblocking'):
             return os.nb_write(*args)
 
 
+if hasattr(os, 'fork_and_watch'):
+
+    class TestForkAndWatch(TestCase):
+
+        __timeout__ = 5
+
+        def test_waitpid_all(self):
+            # Cover this specific case.
+            pid = os.fork_and_watch()
+            if pid:
+                os.waitpid(-1, 0)
+                # Can't assert on what the pid actually was,
+                # our testrunner may have spawned multiple children.
+                os._reap_children(0) # make the leakchecker happy
+            else:
+                gevent.sleep(2)
+                os._exit(0)
+
+        def test_waitpid_wrong_neg(self):
+            self.assertRaises(OSError, os.waitpid, -2, 0)
+
+        def test_waitpid_wrong_pos(self):
+            self.assertRaises(OSError, os.waitpid, 1, 0)
+
 if __name__ == '__main__':
     main()