Forward SIGHUP to child processes and process SystemExit more carefully (#5786)

* propagate SIGHUP to child processes

* do not wait for active job if shell is exiting

* move on_postcommand event out of _append_history and document behavior

* add changelog

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update built_ins.py

* Update hup-propagation.rst

* use sys.exc_info instead of sys.exception for version compatibility

* update test_default_append_history to work with new history/postcommand interaction

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add tests for SIGHUP forwarding and postcommand blocking

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fix linting errors in news entry

* use tmpdir fixture in sighup tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update test_integrations.py

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Andy Kipp <anki-code@users.noreply.github.com>

news/hup-propagation.rst

@@ -0,0 +1,25 @@
+**Added:**
+
+* SIGHUP will now be forwarded to child processes when received by the main xonsh process.
+  This matches the behavior of other shells e.g. bash.
+* Documented the fact that the ``on_postcommand`` event only fires in interactive mode.
+
+**Changed:**
+
+* <news item>
+
+**Deprecated:**
+
+* <news item>
+
+**Removed:**
+
+* <news item>
+
+**Fixed:**
+
+* Running a subcommand in an event handler will no longer block xonsh from exiting.
+
+**Security:**
+
+* <news item>

tests/shell/test_base_shell.py

@@ -53,12 +53,7 @@ def test_default_append_history(cmd, exp_append_history, xonsh_session, monkeypa
     """Test that running an empty line or a comment does not append to history"""
     append_history_calls = []
 
-    def mock_append_history(**info):
-        append_history_calls.append(info)
-
-    monkeypatch.setattr(
-        xonsh_session.shell.shell, "_append_history", mock_append_history
-    )
+    monkeypatch.setattr(xonsh_session.history, "append", append_history_calls.append)
     xonsh_session.shell.default(cmd)
     if exp_append_history:
         assert len(append_history_calls) == 1

tests/shell/test_ptk_shell.py

@@ -137,10 +137,9 @@ def test_ptk_default_append_history(cmd, exp_append_history, ptk_shell, monkeypa
     inp, out, shell = ptk_shell
     append_history_calls = []
 
-    def mock_append_history(**info):
-        append_history_calls.append(info)
-
-    monkeypatch.setattr(shell, "_append_history", mock_append_history)
+    monkeypatch.setattr(
+        "xonsh.built_ins.XSH.history.append", append_history_calls.append
+    )
     shell.default(cmd)
     if exp_append_history:
         assert len(append_history_calls) == 1

tests/test_integrations.py

@@ -64,6 +64,7 @@ def run_xonsh(
     args=None,
     timeout=20,
     env=None,
+    blocking=True,
 ):
     # Env
     popen_env = dict(os.environ)
@@ -107,6 +108,9 @@ def run_xonsh(
         proc.stdin.write(stdin_cmd)
         proc.stdin.flush()
 
+    if not blocking:
+        return proc
+
     try:
         out, err = proc.communicate(input=input, timeout=timeout)
     except sp.TimeoutExpired:
@@ -1356,6 +1360,58 @@ def test_catching_exit_signal():
     assert ret > 0
 
 
+@skip_if_on_windows
+def test_forwarding_sighup(tmpdir):
+    """We want to make sure that SIGHUP is forwarded to subprocesses when
+    received, so we spin up a Bash process that waits for SIGHUP and then
+    writes `SIGHUP` to a file, then exits. Then we check the content of
+    that file to ensure that the Bash process really did get SIGHUP."""
+    outfile = tmpdir.mkdir("xonsh_test_dir").join("sighup_test.out")
+
+    stdin_cmd = f"""
+sleep 0.2
+(sleep 1 && kill -SIGHUP @(__import__('os').getppid())) &
+bash -c "trap 'echo SIGHUP > {outfile}; exit 0' HUP; sleep 30 & wait $!"
+"""
+    proc = run_xonsh(
+        cmd=None,
+        stdin_cmd=stdin_cmd,
+        stderr=sp.PIPE,
+        interactive=True,
+        single_command=False,
+        blocking=False,
+    )
+    proc.wait(timeout=5)
+    # if this raises FileNotFoundError, then the Bash subprocess probably did not get SIGHUP
+    assert outfile.read_text("utf-8").strip() == "SIGHUP"
+
+
+@skip_if_on_windows
+def test_on_postcommand_waiting(tmpdir):
+    """Ensure that running a subcommand in the on_postcommand hook doesn't
+    block xonsh from exiting when there is a running foreground process."""
+    outdir = tmpdir.mkdir("xonsh_test_dir")
+
+    stdin_cmd = f"""
+sleep 0.2
+@events.on_postcommand
+def postcmd_hook(**kwargs):
+    touch {outdir}/sighup_test_postcommand
+
+(sleep 1 && kill -SIGHUP @(__import__('os').getppid())) &
+bash -c "trap '' HUP; sleep 30"
+"""
+    proc = run_xonsh(
+        cmd=None,
+        stdin_cmd=stdin_cmd,
+        stderr=sp.PIPE,
+        interactive=True,
+        single_command=False,
+        blocking=False,
+    )
+    proc.wait(timeout=5)
+
+
 @skip_if_on_windows
 def test_suspended_captured_process_pipeline():
     """See also test_specs.py:test_specs_with_suspended_captured_process_pipeline"""

xonsh/built_ins.py

@@ -58,6 +58,15 @@ def resetting_signal_handle(sig, f):
     def new_signal_handler(s=None, frame=None):
         f(s, frame)
         signal.signal(sig, prev_signal_handler)
+        if sig == signal.SIGHUP:
+            """
+            SIGHUP means the controlling terminal has been lost. This should be
+            propagated to child processes so that they can decide what to do about it.
+            See also: https://www.gnu.org/software/bash/manual/bash.html#Signals
+            """
+            import xonsh.procs.jobs as xj
+
+            xj.hup_all_jobs()
         if sig != 0:
             """
             There is no immediate exiting here.

xonsh/procs/pipelines.py

@@ -267,7 +267,10 @@ class CommandPipeline:
             # we get here if the process is not threadable or the
             # class is the real Popen
             PrevProcCloser(pipeline=self)
-            task = xj.wait_for_active_job()
+            task = None
+            if not isinstance(sys.exc_info()[1], SystemExit):
+                task = xj.wait_for_active_job()
+
             if task is None or task["status"] != "stopped":
                 proc.wait()
                 self._endtime()

xonsh/shell.py

@@ -46,6 +46,7 @@ events.doc(
 on_postcommand(cmd: str, rtn: int, out: str or None, ts: list) -> None
 
 Fires just after a command is executed. The arguments are the same as history.
+This event only fires in interactive mode.
 
 Parameters:
 

xonsh/shells/base_shell.py

@@ -406,14 +406,20 @@ class BaseShell:
         finally:
             ts1 = ts1 or time.time()
             tee_out = tee.getvalue()
-            self._append_history(
+            info = self._append_history(
                 inp=src,
                 ts=[ts0, ts1],
                 spc=self.src_starts_with_space,
                 tee_out=tee_out,
                 cwd=self.precwd,
             )
-            self.accumulated_inputs += src
+            if not isinstance(exc_info[1], SystemExit):
+                events.on_postcommand.fire(
+                    cmd=info["inp"],
+                    rtn=info["rtn"],
+                    out=info.get("out", None),
+                    ts=info["ts"],
+                )
             if (
                 tee_out
                 and env.get("XONSH_APPEND_NEWLINE")
@@ -444,12 +450,10 @@ class BaseShell:
             info["out"] = last_out
         else:
             info["out"] = tee_out + "\n" + last_out
-        events.on_postcommand.fire(
-            cmd=info["inp"], rtn=info["rtn"], out=info.get("out", None), ts=info["ts"]
-        )
         if hist is not None:
             hist.append(info)
             hist.last_cmd_rtn = hist.last_cmd_out = None
+        return info
 
     def _fix_cwd(self):
         """Check if the cwd changed out from under us."""