[signal-handling] Don't clobber shell's SIGWINCH handler with user trap

Unfortunately there is no test for this, but it's obviously better.

test/interactive: Bail early if you have enough successes

core/process.py

@@ -1436,7 +1436,12 @@ def WaitForOne(self, eintr_retry):
         # Examples:
         # - 128 + SIGUSR1 = 128 + 10 = 138
         # - 128 + SIGUSR2 = 128 + 12 = 140
-        return 0 if eintr_retry else (128 + self.sig_state.last_sig_num)
+        if eintr_retry:
+          # Caller should keep waiting.  Note: only Process::Wait() calls this?
+          # It might not be necessary anymore?
+          return 0
+        else:
+          return 128 + self.sig_state.last_sig_num
       else:
         # The signature of waitpid() means this shouldn't happen
         raise AssertionError()

core/pyos.py

@@ -21,6 +21,7 @@
 
 if TYPE_CHECKING:
   from core.comp_ui import _IDisplay
+  from osh.builtin_process import _TrapHandler
 
 
 EOF_SENTINEL = 256  # bigger than any byte
@@ -240,6 +241,23 @@ def InputAvailable(fd):
   return len(r) != 0
 
 
+class SigwinchHandler(object):
+  """Wrapper to call user handler."""
+
+  def __init__(self, display):
+    # type: (_IDisplay) -> None
+    self.display = display
+    self.user_handler = None  # type: _TrapHandler
+
+  def __call__(self, sig_num, unused_frame):
+    # type: (int, Any) -> None
+    """For Python's signal module."""
+
+    self.display.OnWindowChange()
+    if self.user_handler:
+      self.user_handler(sig_num, unused_frame)
+
+
 def SignalState_AfterForkingChild():
   # type: () -> None
   """Not a member of SignalState since we didn't do dependency injection."""
@@ -261,9 +279,7 @@ class SignalState(object):
 
   def __init__(self):
     # type: () -> None
-    # Before doing anything else, save the original handler that raises
-    # KeyboardInterrupt.
-    self.orig_sigint_handler = signal.getsignal(signal.SIGINT)
+    self.sigwinch_handler = None  # type: SigwinchHandler
     self.last_sig_num = 0  # MUTABLE GLOBAL, for interrupted 'wait'
 
   def InitShell(self):
@@ -282,15 +298,30 @@ def InitInteractiveShell(self, display):
 
     # Register a callback to receive terminal width changes.
     # NOTE: In line_input.c, we turned off rl_catch_sigwinch.
-    signal.signal(signal.SIGWINCH, lambda x, y: display.OnWindowChange())
+
+    # This is ALWAYS on, which means that it can cause EINTR, and wait() and
+    # read() have to handle it
+    self.sigwinch_handler = SigwinchHandler(display)
+    signal.signal(signal.SIGWINCH, self.sigwinch_handler)
 
   def AddUserTrap(self, sig_num, handler):
     # type: (int, Any) -> None
     """For user-defined handlers registered with the 'trap' builtin."""
-    signal.signal(sig_num, handler)
+
+    if sig_num == signal.SIGWINCH:
+      assert self.sigwinch_handler is not None
+      self.sigwinch_handler.user_handler = handler
+    else:
+      signal.signal(sig_num, handler)
+    # TODO: SIGINT is similar: set a flag, then optionally call user _TrapHandler
 
   def RemoveUserTrap(self, sig_num):
     # type: (int) -> None
     """For user-defined handlers registered with the 'trap' builtin."""
     # Restore default
-    signal.signal(sig_num, signal.SIG_DFL)
+    if sig_num == signal.SIGWINCH:
+      assert self.sigwinch_handler is not None
+      self.sigwinch_handler.user_handler = None
+    else:
+      signal.signal(sig_num, signal.SIG_DFL)
+    # TODO: SIGINT is similar: set a flag, then optionally call user _TrapHandler

test/interactive.sh

@@ -62,16 +62,15 @@ soil-run() {
     if test "$status" -eq 0; then
       num_success=$((num_success + 1))
     fi
+    if test "$num_success" -ge 2; then
+      echo "test/interactive OK: 2 of $i tries succeeded"
+      return 0
+    fi
   done
 
   # This test is flaky, so only require 2 of 5 successes
-  echo "test/interactive: $num_success of $n tries succeeded"
-
-  if test "$num_success" -ge 2; then
-    return 0
-  else
-    return 1
-  fi
+  echo "test/interactive FAIL: got $num_success successes after $n tries"
+  return 1
 }
 
 "$@"