Rework how signals trigger cancellation

When fish receives a "cancellation inducing" signal (SIGINT in particular)
it has to unwind execution - for example while loops or whatever else that
is executing. There are two ways this may come about:

1. The fish process received the signal
2. A child process received the signal

An example of the second case is:

    some_command | some_function

Here `some_command` is the tty owner and so will receive control-C, but
then fish has to cancel function execution.

Prior to this change, these were handled uniformly: both would just set a
cancellation signal inside the parser. However in the future we will have
multiple parsers and it may not be obvious which one to set the flag in.
So instead distinguish these cases: if a process receives SIGINT we mark
the signal in its job group, and if fish receives it we set a global
variable.

src/event.cpp

@@ -289,7 +289,7 @@ void event_fire_delayed(parser_t &parser) {
     // Do not invoke new event handlers from within event handlers.
     if (ld.is_event) return;
     // Do not invoke new event handlers if we are unwinding (#6649).
-    if (parser.get_cancel_signal()) return;
+    if (signal_check_cancel()) return;
 
     std::vector<shared_ptr<event_t>> to_send;
     to_send.swap(ld.blocked_events);

src/fish_tests.cpp

@@ -1251,7 +1251,6 @@ static void test_cancellation() {
     say(L"Testing Ctrl-C cancellation. If this hangs, that's a bug!");
 
     // Enable fish's signal handling here.
-    parser_t &parser = parser_t::principal_parser();
     signal_set_handlers(true);
 
     // This tests that we can correctly ctrl-C out of certain loop constructs, and that nothing gets
@@ -1271,7 +1270,7 @@ static void test_cancellation() {
 
     // Ensure that we don't think we should cancel.
     reader_reset_interrupted();
-    parser.clear_cancel();
+    signal_clear_cancel();
 }
 
 namespace indent_tests {

src/parse_execution.cpp

@@ -225,15 +225,12 @@ process_type_t parse_execution_context_t::process_type_for_command(
 }
 
 maybe_t<end_execution_reason_t> parse_execution_context_t::check_end_execution() const {
-    if (shell_is_exiting()) {
+    if (ctx.check_cancel() || shell_is_exiting()) {
         return end_execution_reason_t::cancelled;
     }
     if (nullptr == parser) {
         return none();
     }
-    if (parser->cancellation_signal) {
-        return end_execution_reason_t::cancelled;
-    }
     const auto &ld = parser->libdata();
     if (ld.returning) {
         return end_execution_reason_t::control_flow;
@@ -675,7 +672,7 @@ end_execution_reason_t parse_execution_context_t::report_error(int status, const
 
 end_execution_reason_t parse_execution_context_t::report_errors(
     int status, const parse_error_list_t &error_list) const {
-    if (!parser->cancellation_signal) {
+    if (!ctx.check_cancel()) {
         if (error_list.empty()) {
             FLOG(error, L"Error reported but no error text found.");
         }

src/parser.cpp

@@ -26,6 +26,7 @@
 #include "proc.h"
 #include "reader.h"
 #include "sanity.h"
+#include "signal.h"
 #include "wutil.h"  // IWYU pragma: keep
 
 class io_chain_t;
@@ -102,11 +103,6 @@ parser_t &parser_t::principal_parser() {
     return *principal;
 }
 
-void parser_t::cancel_requested(int sig) {
-    assert(sig != 0 && "Signal must not be 0");
-    principal->cancellation_signal = sig;
-}
-
 int parser_t::set_var_and_fire(const wcstring &key, env_mode_flags_t mode, wcstring_list_t vals) {
     std::vector<event_t> events;
     int res = vars().set(key, mode, std::move(vals), &events);
@@ -350,7 +346,7 @@ completion_list_t parser_t::expand_argument_list(const wcstring &arg_list_src,
 std::shared_ptr<parser_t> parser_t::shared() { return shared_from_this(); }
 
 cancel_checker_t parser_t::cancel_checker() const {
-    return [this]() { return this->cancellation_signal != 0; };
+    return [] { return signal_check_cancel() != 0; };
 }
 
 operation_context_t parser_t::context() {
@@ -674,20 +670,40 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, const T &node,
     static_assert(
         std::is_same<T, ast::statement_t>::value || std::is_same<T, ast::job_list_t>::value,
         "Unexpected node type");
-    // Handle cancellation requests. If our block stack is currently empty, then we already did
-    // successfully cancel (or there was nothing to cancel); clear the flag. If our block stack is
-    // not empty, we are still in the process of cancelling; refuse to evaluate anything.
-    if (this->cancellation_signal) {
-        if (!block_list.empty()) {
-            return proc_status_t::from_signal(this->cancellation_signal);
-        }
-        this->cancellation_signal = 0;
-    }
 
     // Only certain blocks are allowed.
     assert((block_type == block_type_t::top || block_type == block_type_t::subst) &&
            "Invalid block type");
 
+    // If fish itself got a cancel signal, then we want to unwind back to the principal parser.
+    // If we are the principal parser and our block stack is empty, then we want to clear the
+    // signal.
+    // Note this only happens in interactive sessions. In non-interactive sessions, SIGINT will
+    // cause fish to exit.
+    if (int sig = signal_check_cancel()) {
+        if (this == principal.get() && block_list.empty()) {
+            signal_clear_cancel();
+        } else {
+            return proc_status_t::from_signal(sig);
+        }
+    }
+
+    // A helper to detect if we got a signal.
+    // This includes both signals sent to fish (user hit control-C while fish is foreground) and
+    // signals from the job group (e.g. some external job terminated with SIGQUIT).
+    auto check_cancel_signal = [=] {
+        // Did fish itself get a signal?
+        int sig = signal_check_cancel();
+        // Has this job group been cancelled?
+        if (!sig && job_group) sig = job_group->get_cancel_signal();
+        return sig;
+    };
+
+    // If we have a job group which is cancelled, then do nothing.
+    if (int sig = check_cancel_signal()) {
+        return proc_status_t::from_signal(sig);
+    }
+
     job_reap(*this, false);  // not sure why we reap jobs here
 
     // Start it up
@@ -697,6 +713,9 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, const T &node,
     // Propogate our job group.
     op_ctx.job_group = job_group;
 
+    // Replace the context's cancel checker with one that checks the job group's signal.
+    op_ctx.cancel_checker = [=] { return check_cancel_signal() != 0; };
+
     // Create and set a new execution context.
     using exc_ctx_ref_t = std::unique_ptr<parse_execution_context_t>;
     scoped_push<exc_ctx_ref_t> exc(&execution_context,
@@ -712,9 +731,9 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, const T &node,
 
     job_reap(*this, false);  // reap again
 
-    if (this->cancellation_signal) {
+    if (int sig = check_cancel_signal()) {
         // We were signalled.
-        return proc_status_t::from_signal(this->cancellation_signal);
+        return proc_status_t::from_signal(sig);
     } else {
         auto status = proc_status_t::from_exit_code(this->get_last_status());
         bool break_expand = (reason == end_execution_reason_t::error);

src/parser.h

@@ -220,10 +220,7 @@ struct eval_res_t {
 
 class parser_t : public std::enable_shared_from_this<parser_t> {
     friend class parse_execution_context_t;
-
    private:
-    /// If not zero, the signal triggering cancellation.
-    volatile sig_atomic_t cancellation_signal = 0;
     /// The current execution context.
     std::unique_ptr<parse_execution_context_t> execution_context;
     /// The jobs associated with this parser.
@@ -269,12 +266,6 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
     /// Get the "principal" parser, whatever that is.
     static parser_t &principal_parser();
 
-    /// Indicates that we should stop execution due to the given signal.
-    static void cancel_requested(int sig);
-
-    /// Clear any cancel.
-    void clear_cancel() { cancellation_signal = 0; }
-
     /// Global event blocks.
     event_blockage_list_t global_event_blocks;
 
@@ -386,9 +377,6 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
     void get_backtrace(const wcstring &src, const parse_error_list_t &errors,
                        wcstring &output) const;
 
-    /// \return the signal triggering cancellation, or 0 if none.
-    int get_cancel_signal() const { return cancellation_signal; }
-
     /// Output profiling data to the given filename.
     void emit_profiling(const char *path) const;
 

src/proc.cpp

@@ -321,7 +321,8 @@ void job_mark_process_as_failed(const std::shared_ptr<job_t> &job, const process
 }
 
 /// Set the status of \p proc to \p status.
-static void handle_child_status(process_t *proc, proc_status_t status) {
+static void handle_child_status(const shared_ptr<job_t> &job, process_t *proc,
+                                proc_status_t status) {
     proc->status = status;
     if (status.stopped()) {
         proc->stopped = true;
@@ -336,9 +337,8 @@ static void handle_child_status(process_t *proc, proc_status_t status) {
         int sig = status.signal_code();
         if (sig == SIGINT || sig == SIGQUIT) {
             if (session_interactivity() != session_interactivity_t::not_interactive) {
-                // In an interactive session, tell the principal parser to skip all blocks we're
+                // Mark the job group as cancelled.
-                // executing so control-C returns control to the user.
+                job->group->set_cancel_signal(sig);
-                parser_t::cancel_requested(sig);
             } else {
                 // Deliver the SIGINT or SIGQUIT signal to ourself since we're not interactive.
                 struct sigaction act;
@@ -485,7 +485,7 @@ static void process_mark_finished_children(parser_t &parser, bool block_ok) {
                     if (proc->internal_proc_) {
                         // Try reaping an internal process.
                         if (proc->internal_proc_->exited()) {
-                            handle_child_status(proc.get(), proc->internal_proc_->get_status());
+                            handle_child_status(j, proc.get(), proc->internal_proc_->get_status());
                             FLOGF(proc_reap_internal,
                                   "Reaped internal process '%ls' (id %llu, status %d)",
                                   proc->argv0(), proc->internal_proc_->get_id(),
@@ -497,7 +497,7 @@ static void process_mark_finished_children(parser_t &parser, bool block_ok) {
                         auto pid = waitpid(proc->pid, &status, WNOHANG | WUNTRACED | WCONTINUED);
                         if (pid > 0) {
                             assert(pid == proc->pid && "Unexpcted waitpid() return");
-                            handle_child_status(proc.get(), proc_status_t::from_waitpid(status));
+                            handle_child_status(j, proc.get(), proc_status_t::from_waitpid(status));
                             if (proc->status.stopped()) {
                                 j->group->set_is_foreground(false);
                             }

src/proc.h

@@ -209,6 +209,12 @@ class job_group_t {
     /// \return the job ID, or -1 if none.
     job_id_t get_id() const { return props_.job_id; }
 
+    /// Get the cancel signal, or 0 if none.
+    int get_cancel_signal() const { return cancel_signal_; }
+
+    /// Mark that a process in this group got a signal, and so should cancel.
+    void set_cancel_signal(int sig) { cancel_signal_ = sig; }
+
     /// Mark the root as constructed.
     /// This is used to avoid reaping a process group leader while there are still procs that may
     /// want to enter its group.
@@ -248,6 +254,9 @@ class job_group_t {
     // Whether the root job is constructed. If not, we cannot reap it yet.
     relaxed_atomic_bool_t root_constructed_{};
 
+    // If not zero, a signal indicating cancellation.
+    int cancel_signal_{};
+
     explicit job_group_t(const properties_t &props) : props_(props) {}
 };
 

src/reader.cpp

@@ -886,7 +886,6 @@ void reader_data_t::kill(editable_line_t *el, size_t begin_idx, size_t length, i
 
 // This is called from a signal handler!
 void reader_handle_sigint() {
-    parser_t::cancel_requested(SIGINT);
     interrupted = SIGINT;
 }
 
@@ -2529,7 +2528,7 @@ static int read_i(parser_t &parser) {
             wcstring_list_t argv(1, command);
             event_fire_generic(parser, L"fish_preexec", &argv);
             reader_run_command(parser, command);
-            parser.clear_cancel();
+            signal_clear_cancel();
             event_fire_generic(parser, L"fish_postexec", &argv);
             // Allow any pending history items to be returned in the history array.
             if (data->history) {
@@ -3650,7 +3649,7 @@ maybe_t<wcstring> reader_data_t::readline(int nchars_or_0) {
 
             // Readline commands may be bound to \cc which also sets the cancel flag.
             // See #6937.
-            parser().clear_cancel();
+            signal_clear_cancel();
 
             rls.last_cmd = readline_cmd;
         } else {
@@ -3768,7 +3767,6 @@ int reader_reading_interrupted() {
     reader_data_t *data = current_data_or_null();
     if (res && data && data->exit_on_interrupt) {
         reader_set_end_loop(true);
-        parser_t::cancel_requested(res);
         // We handled the interrupt ourselves, our caller doesn't need to handle it.
         return 0;
     }

src/signal.cpp

@@ -199,6 +199,14 @@ static bool reraise_if_forked_child(int sig) {
     return true;
 }
 
+/// The cancellation signal we have received.
+/// Of course this is modified from a signal handler.
+static volatile sig_atomic_t s_cancellation_signal = 0;
+
+void signal_clear_cancel() { s_cancellation_signal = 0; }
+
+int signal_check_cancel() { return s_cancellation_signal; }
+
 /// The single signal handler. By centralizing signal handling we ensure that we can never install
 /// the "wrong" signal handler (see #5969).
 static void fish_signal_handler(int sig, siginfo_t *info, void *context) {
@@ -244,6 +252,7 @@ static void fish_signal_handler(int sig, siginfo_t *info, void *context) {
         case SIGINT:
             /// Interactive mode ^C handler. Respond to int signal by setting interrupted-flag and
             /// stopping all loops and conditionals.
+            s_cancellation_signal = SIGINT;
             reader_handle_sigint();
             topic_monitor_t::principal().post(topic_t::sighupint);
             break;

src/signal.h

@@ -33,6 +33,15 @@ void signal_unblock_all();
 /// Returns signals with non-default handlers.
 void get_signals_with_handlers(sigset_t *set);
 
+/// \return the most recent cancellation signal received by the fish process.
+/// Currently only SIGINT is considered a cancellation signal.
+/// This is thread safe.
+int signal_check_cancel();
+
+/// Set the cancellation signal to zero.
+/// In generaly this should only be done in interactive sessions.
+void signal_clear_cancel();
+
 enum class topic_t : uint8_t;
 /// A sigint_detector_t can be used to check if a SIGINT (or SIGHUP) has been delivered.
 class sigchecker_t {