From 82fed6fc2f8b7b2cc9bc249fbe349acd0a5d7721 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Wed, 12 Aug 2020 21:35:37 -0700 Subject: [PATCH] Correctly propagate signals from cancelled jobs into parse_execution_context This concerns code like the following: while true ; sleep 100; end Here 'while' is a "simple block execution" and does not create a new job, or get a pgid. Each 'sleep' however is an external command execution, and is treated as a distinct job. (bash is the same way). So `while` and `sleep` are always in different job groups. The problem comes about if 'sleep' is cancelled through SIGINT or SIGQUIT. Prior to 2a4c545b21, if *any* process got a SIGINT or SIGQUIT, then fish would mark a global "stop executing" variable. This obviously prevents background execution of fish functions. In 2a4c545b21, this was changed so only the job's group gets marked as cancelled. However in the case of one job group spawning another, we weren't propagating the signal. This adds a signal to parse_execution_context which the parser checks after execution. It's not ideal since now we have three different places where signals can be recorded. However it fixes this regression which is too important to leave unfixed for long. Fixes #7259 --- src/parse_execution.cpp | 13 +++++++++---- src/parse_execution.h | 7 +++++++ src/parser.cpp | 11 ++++++++++- tests/pexpects/sigint.py | 25 +++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 5 deletions(-) create mode 100644 tests/pexpects/sigint.py diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 82f0735fe..4f297732e 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -226,12 +226,9 @@ process_type_t parse_execution_context_t::process_type_for_command( } maybe_t parse_execution_context_t::check_end_execution() const { - if (ctx.check_cancel() || shell_is_exiting()) { + if (this->cancel_signal || ctx.check_cancel() || shell_is_exiting()) { return end_execution_reason_t::cancelled; } - if (nullptr == parser) { - return none(); - } const auto &ld = parser->libdata(); if (ld.returning) { return end_execution_reason_t::control_flow; @@ -1343,6 +1340,14 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo remove_job(*this->parser, job.get()); } + // Check if the job's group got a SIGINT or SIGQUIT. + // If so we need to mark that ourselves so as to cancel the rest of the execution. + // See #7259. + int cancel_sig = job->group->get_cancel_signal(); + if (cancel_sig == SIGINT || cancel_sig == SIGQUIT) { + this->cancel_signal = cancel_sig; + } + // Update universal variables on external conmmands. // TODO: justify this, why not on every command? if (job_contained_external_command) { diff --git a/src/parse_execution.h b/src/parse_execution.h index acb32fa28..4af3f95d1 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -45,6 +45,10 @@ class parse_execution_context_t { size_t cached_lineno_offset = 0; int cached_lineno_count = 0; + /// If a process dies due to a SIGINT or SIGQUIT, then store the corresponding signal here. + /// Note this latches to SIGINT or SIGQUIT; it is never cleared. + int cancel_signal{0}; + /// The block IO chain. /// For example, in `begin; foo ; end < file.txt` this would have the 'file.txt' IO. io_chain_t block_io{}; @@ -160,6 +164,9 @@ class parse_execution_context_t { /// Returns the source offset, or -1. int get_current_source_offset() const; + /// \return the signal that triggered cancellation, or 0 if none. + int get_cancel_signal() const { return cancel_signal; } + /// Returns the source string. const wcstring &get_source() const { return pstree->src; } diff --git a/src/parser.cpp b/src/parser.cpp index 8150b81ca..1c5b5ce17 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -699,12 +699,21 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, const T &node, const size_t new_exec_count = libdata().exec_count; const size_t new_status_count = libdata().status_count; + // Check if the execution context stopped due to a signal from a job it created. + // This may come about if the context created a new job group. + // TODO: there are way too many signals flying around, we need to rationalize this. + int signal_from_exec = execution_context->get_cancel_signal(); + exc.restore(); this->pop_block(scope_block); job_reap(*this, false); // reap again - if (int sig = check_cancel_signal()) { + if (signal_from_exec) { + // A job spawned by the execution context got SIGINT or SIGQUIT, which stopped all + // execution. + return proc_status_t::from_signal(signal_from_exec); + } else if (int sig = check_cancel_signal()) { // We were signalled. return proc_status_t::from_signal(sig); } else { diff --git a/tests/pexpects/sigint.py b/tests/pexpects/sigint.py new file mode 100644 index 000000000..05113031c --- /dev/null +++ b/tests/pexpects/sigint.py @@ -0,0 +1,25 @@ +#!/usr/bin/env python3 +from pexpect_helper import SpawnedProc + +sp = SpawnedProc() +sendline, sleep, expect_prompt, expect_str = ( + sp.sendline, + sp.sleep, + sp.expect_prompt, + sp.expect_str, +) + +# Ensure that if child processes SIGINT, we exit our loops. +# This is an interactive test because the parser is expected to +# recover from SIGINT in interactive mode. +# Test for #7259. + +expect_prompt() +sendline("while true; sh -c 'echo Here we go; sleep .25; kill -s INT $$'; end") +sleep(0.30) +expect_str("Here we go") +expect_prompt() + +sendline("echo it worked") +expect_str("it worked") +expect_prompt()