From 2cdcd6c97673e53be2ea502c0ca9967566aaa8b1 Mon Sep 17 00:00:00 2001 From: jfmontanaro Date: Thu, 30 Jan 2025 06:06:50 -0500 Subject: [PATCH] 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 --- news/hup-propagation.rst | 25 +++++++++++++++ tests/shell/test_base_shell.py | 7 +---- tests/shell/test_ptk_shell.py | 7 ++--- tests/test_integrations.py | 56 ++++++++++++++++++++++++++++++++++ xonsh/built_ins.py | 9 ++++++ xonsh/procs/pipelines.py | 5 ++- xonsh/shell.py | 1 + xonsh/shells/base_shell.py | 14 ++++++--- 8 files changed, 108 insertions(+), 16 deletions(-) create mode 100644 news/hup-propagation.rst diff --git a/news/hup-propagation.rst b/news/hup-propagation.rst new file mode 100644 index 000000000..f42deb761 --- /dev/null +++ b/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:** + +* + +**Deprecated:** + +* + +**Removed:** + +* + +**Fixed:** + +* Running a subcommand in an event handler will no longer block xonsh from exiting. + +**Security:** + +* diff --git a/tests/shell/test_base_shell.py b/tests/shell/test_base_shell.py index db3d1e20d..b08af6330 100644 --- a/tests/shell/test_base_shell.py +++ b/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 diff --git a/tests/shell/test_ptk_shell.py b/tests/shell/test_ptk_shell.py index 101090f93..cf596f711 100644 --- a/tests/shell/test_ptk_shell.py +++ b/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 diff --git a/tests/test_integrations.py b/tests/test_integrations.py index acacf1d52..980a11846 100644 --- a/tests/test_integrations.py +++ b/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""" diff --git a/xonsh/built_ins.py b/xonsh/built_ins.py index 4f227b6e3..dbd9148c7 100644 --- a/xonsh/built_ins.py +++ b/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. diff --git a/xonsh/procs/pipelines.py b/xonsh/procs/pipelines.py index eec6c6c9e..fabcd0e57 100644 --- a/xonsh/procs/pipelines.py +++ b/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() diff --git a/xonsh/shell.py b/xonsh/shell.py index bb8117f94..e37ddb901 100644 --- a/xonsh/shell.py +++ b/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: diff --git a/xonsh/shells/base_shell.py b/xonsh/shells/base_shell.py index c4938477c..acb050521 100644 --- a/xonsh/shells/base_shell.py +++ b/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."""