[builtin/wait] wait builtin can be interrupted.

Addresses issue #858.

Also improve test case.  Found the same bug in dash!  bash/mksh/zsh have
special cases that allow you to interrupt the 'wait' builtin.

Also make note of bug with Ctrl-C / SIGINT, due to KeyboardInterrupt.

Author: Andy Chu

core/process.py

@@ -883,19 +883,20 @@ def Start(self, why):
 
     return pid
 
-  def Wait(self, waiter):
-    # type: (Waiter) -> int
+  def Wait(self, waiter, eintr_retry=True):
+    # type: (Waiter, bool) -> int
     """Wait for this process to finish."""
     while True:
-      if not waiter.WaitForOne():
+      if not waiter.WaitForOne(eintr_retry):
         break
       if self.state != job_state_e.Running:
         break
     return self.status
 
   def JobWait(self, waiter):
     # type: (Waiter) -> job_status_t
-    exit_code = self.Wait(waiter)
+    # wait builtin can be interrupted
+    exit_code = self.Wait(waiter, eintr_retry=False)
     return job_status.Proc(exit_code)
 
   def WhenStopped(self):
@@ -1011,8 +1012,8 @@ def LastPid(self):
     """
     return self.pids[-1]
 
-  def Wait(self, waiter):
-    # type: (Waiter) -> List[int]
+  def Wait(self, waiter, eintr_retry=True):
+    # type: (Waiter, bool) -> List[int]
     """Wait for this pipeline to finish.
 
     Called by the 'wait' builtin.
@@ -1022,7 +1023,7 @@ def Wait(self, waiter):
     assert self.procs, "no procs for Wait()"
     while True:
       #log('WAIT pipeline')
-      if not waiter.WaitForOne():
+      if not waiter.WaitForOne(eintr_retry):
         break
       if self.state != job_state_e.Running:
         #log('Pipeline DONE')
@@ -1032,7 +1033,8 @@ def Wait(self, waiter):
 
   def JobWait(self, waiter):
     # type: (Waiter) -> job_status_t
-    pipe_status = self.Wait(waiter)
+    # wait builtin can be interrupted
+    pipe_status = self.Wait(waiter, eintr_retry=False)
     return job_status.Pipeline(pipe_status)
 
   def Run(self, waiter, fd_state):
@@ -1288,31 +1290,58 @@ def __init__(self, job_state, exec_opts, tracer):
     self.tracer = tracer
     self.last_status = 127  # wait -n error code
 
-  def WaitForOne(self):
-    # type: () -> bool
+  def WaitForOne(self, eintr_retry):
+    # type: (bool) -> bool
     """Wait until the next process returns (or maybe Ctrl-C).
 
+    Args:
+      Should be True to prevent zombies
+
     Returns:
       True if we got a notification, or False if there was nothing to wait for.
 
       In the interactive shell, we return True if we get a Ctrl-C, so the
       caller will try again.
+
+    Callers:
+      wait -n  -- WaitForOne() just once
+      wait     -- WaitForOne() in a loop
+      wait $!  -- job.JobWait()
+
+    Comparisons:
+      bash: jobs.c waitchld() Has a special case macro(!) CHECK_WAIT_INTR for
+      the wait builtin
+
+      dash: jobs.c waitproc() uses sigfillset(), sigprocmask(), etc.  Runs in a
+      loop while (gotsigchld), but that might be a hack for System V!
+
+    You could imagine a clean API like posix::wait_for_one() 
+
+    wait_result =
+      ECHILD                     -- nothing to wait for
+    | Done(int pid, int status)  -- process done
+    | EINTR(bool sigint)         -- may or may not retry
+
+    But I think we want to keep KeyboardInterrupt as an exception for now.
     """
     # This is a list of async jobs
     try:
-      # -1 makes it like wait(), which waits for any process.
-      # NOTE: WUNTRACED is necessary to get stopped jobs.  What about
-      # WCONTINUED?
+      # Notes:
+      # - The arg -1 makes it like wait(), which waits for any process.
+      # - WUNTRACED is necessary to get stopped jobs.  What about WCONTINUED?
+      # - Unlike other syscalls, we do NOT try on EINTR, because the 'wait'
+      #   builtin should be interruptable.  This doesn't appear to cause any
+      #   problems for other WaitForOne callers?
       pid, status = posix.waitpid(-1, WUNTRACED)
     except OSError as e:
       #log('wait() error: %s', e)
       if e.errno == errno_.ECHILD:
         return False  # nothing to wait for caller should stop
+      elif e.errno == errno_.EINTR:  # Bug #858 fix
+        return eintr_retry
       else:
-        # We should never get here.  EINTR was handled by the 'posix'
-        # module.  The only other error is EINVAL, which doesn't apply to
-        # this call.
-        raise
+        # The signature of waitpid() means this shouldn't happen
+        raise AssertionError()
     except KeyboardInterrupt:
       # NOTE: Another way to handle this is to disable SIGINT when a process is
       # running.  Not sure if there's any real difference.  bash and dash

demo/bug-858-trap.sh

@@ -2,11 +2,28 @@ action() {
   echo "from signal"
 }
 
-trap "action" USR1
-
+trap 'action' USR1
 echo "Run: kill -USR1 $$; date"
-while true; do
-  date
-	sleep 5 &
-	wait $!
-done
+
+# bash/zsh/mksh: wait is interrupted by USR1
+# dash: wait is NOT interrupted!
+
+async_test() {
+  while true; do
+    date
+    sleep 5 &
+    wait $!
+    echo 'next'
+  done
+}
+
+# bash/dash/zsh/mksh: signal handler run after sleep
+sync_test() {
+  while true; do
+    date
+    sleep 5
+    echo 'next'
+  done
+}
+
+"$@"

demo/trap.sh

@@ -2,9 +2,15 @@
 #
 # Demo of traps.
 
+# BUG: OSH doesn't run the trap because of KeyboardInterrupt!
+
+# BUG interactively too:
+#
+# osh$ trap 'echo INT' SIGINT; sleep 5 & wait
+
 sigint-batch() {
   trap 'echo [got SIGINT]' INT
-  echo "Registered SIGINT trap.  Run 'kill -INT $$' to see a message."
+  echo "Registered SIGINT trap.  Hit Ctrl-C or Run 'kill -INT $$' to see a message."
   sleep 5
 }
 

native/posixmodule.c

@@ -1668,6 +1668,7 @@ PyDoc_STRVAR_remove(posix_waitpid__doc__,
 "waitpid(pid, options) -> (pid, status)\n\n\
 Wait for completion of a given child process.");
 
+// NO PATCH HERE FOR EINTR !!!  See core/process.py.
 static PyObject *
 posix_waitpid(PyObject *self, PyObject *args)
 {
@@ -1678,23 +1679,11 @@ posix_waitpid(PyObject *self, PyObject *args)
 
     if (!PyArg_ParseTuple(args, PARSE_PID "i:waitpid", &pid, &options))
         return NULL;
-    while (1) {
-        Py_BEGIN_ALLOW_THREADS
-        pid = waitpid(pid, &status, options);
-        Py_END_ALLOW_THREADS
-
-        if (pid >= 0) {  // success
-            break;
-        } else {
-            if (PyErr_CheckSignals()) {
-                return NULL;  // Propagate KeyboardInterrupt
-            }
-            if (errno != EINTR) {  // e.g. ECHILD
-                return posix_error();
-            }
-        }
-        // Otherwise, try again on EINTR.
-    }
+    Py_BEGIN_ALLOW_THREADS
+    pid = waitpid(pid, &status, options);
+    Py_END_ALLOW_THREADS
+    if (pid == -1)
+        return posix_error();
 
     return Py_BuildValue("Ni", PyLong_FromPid(pid), WAIT_STATUS_INT(status));
 }

osh/builtin_process.py

@@ -141,7 +141,7 @@ def _Run(self, cmd_val):
       #    
       #log('wait next')
 
-      if self.waiter.WaitForOne():
+      if self.waiter.WaitForOne(False):
         return self.waiter.last_status
       else:
         return 127  # nothing to wait for
@@ -155,7 +155,7 @@ def _Run(self, cmd_val):
         # we don't get ECHILD.
         # Not sure it matters since you can now Ctrl-C it.
 
-        if not self.waiter.WaitForOne():
+        if not self.waiter.WaitForOne(False):
           break  # nothing to wait for
         i += 1
         if self.job_state.NoneAreRunning():