Remove cancellation groups

Cancellation groups were meant to reflect the following idea: if you ran a
simple block:

    begin
        cmd1
        cmd2
    end

then under job control, cmd1 and cmd2 would get separate groups; however if
either exits due to SIGINT or SIGQUIT we also want to propagate that to the
outer block. So the outermost block and its interior jobs would share a
cancellation group. However this is more complex than necessary; it's
sufficient for the execution context to just store an int internally.

This ought not to affect anything user-visible.

src/job_group.cpp

@@ -31,10 +31,8 @@ static void release_job_id(job_id_t jid) {
     consumed_job_ids->erase(where);
 }
 
-job_group_t::job_group_t(wcstring command, cancellation_group_ref_t cg, job_id_t job_id,
+job_group_t::job_group_t(wcstring command, job_id_t job_id, bool job_control, bool wants_terminal)
-                         bool job_control, bool wants_terminal)
+    : job_control_(job_control),
-    : cancel_group(std::move(cg)),
-      job_control_(job_control),
       wants_terminal_(wants_terminal),
       command_(std::move(command)),
       job_id_(job_id) {}
@@ -46,16 +44,14 @@ job_group_t::~job_group_t() {
 }
 
 // static
-job_group_ref_t job_group_t::create(wcstring command, cancellation_group_ref_t cg,
+job_group_ref_t job_group_t::create(wcstring command, bool wants_job_id) {
-                                    bool wants_job_id) {
     job_id_t jid = wants_job_id ? acquire_job_id() : 0;
-    return job_group_ref_t(new job_group_t(std::move(command), std::move(cg), jid));
+    return job_group_ref_t(new job_group_t(std::move(command), jid));
 }
 
 // static
-job_group_ref_t job_group_t::create_with_job_control(wcstring command, cancellation_group_ref_t cg,
+job_group_ref_t job_group_t::create_with_job_control(wcstring command, bool wants_terminal) {
-                                                     bool wants_terminal) {
+    return job_group_ref_t(new job_group_t(std::move(command), acquire_job_id(),
-    return job_group_ref_t(new job_group_t(std::move(command), std::move(cg), acquire_job_id(),
                                            true /* job_control */, wants_terminal));
 }
 

src/job_group.h

@@ -13,42 +13,6 @@
 /// 1 is the first valid job ID.
 using job_id_t = int;
 
-/// A cancellation group is "a set of jobs that should cancel together." It's effectively just a
-/// shared pointer to a bool which latches to true on cancel.
-/// For example, in `begin ; true ; end | false`, we have two jobs: the outer pipline and the inner
-/// 'true'. These share a cancellation group.
-/// Note this is almost but not quite a job group. A job group is a "a set of jobs which share a
-/// pgid" but cancellation groups may be bigger. For example in `begin ; sleep 1; sleep 2; end` we
-/// have that 'begin' is an internal group (a simple function/block execution) without a pgid,
-/// while each 'sleep' will be a different job, with its own pgid, and so be in a different job
-/// group. But all share a cancellation group.
-/// Note that a background job will always get a new cancellation group.
-/// Cancellation groups must be thread safe.
-class cancellation_group_t {
-   public:
-    /// \return true if we should cancel.
-    bool should_cancel() const { return get_cancel_signal() != 0; }
-
-    /// \return the signal indicating cancellation, or 0 if none.
-    int get_cancel_signal() const { return signal_; }
-
-    /// If we have not already cancelled, then trigger cancellation with the given signal.
-    void cancel_with_signal(int signal) {
-        assert(signal > 0 && "Invalid cancel signal");
-        signal_.compare_exchange(0, signal);
-    }
-
-    /// Helper to return a new group.
-    static std::shared_ptr<cancellation_group_t> create() {
-        return std::make_shared<cancellation_group_t>();
-    }
-
-   private:
-    /// If we cancelled from a signal, return that signal, else 0.
-    relaxed_atomic_t<int> signal_{0};
-};
-using cancellation_group_ref_t = std::shared_ptr<cancellation_group_t>;
-
 /// job_group_t is conceptually similar to the idea of a process group. It represents data which
 /// is shared among all of the "subjobs" that may be spawned by a single job.
 /// For example, two fish functions in a pipeline may themselves spawn multiple jobs, but all will
@@ -86,13 +50,13 @@ class job_group_t {
     bool has_job_id() const { return job_id_ > 0; }
 
     /// Get the cancel signal, or 0 if none.
-    int get_cancel_signal() const { return cancel_group->get_cancel_signal(); }
+    int get_cancel_signal() const { return signal_; }
 
     /// Mark that a process in this group got a signal, and so should cancel.
-    void cancel_with_signal(int sig) { cancel_group->cancel_with_signal(sig); }
+    void cancel_with_signal(int signal) {
-
+        assert(signal > 0 && "Invalid cancel signal");
-    /// The cancellation group. This is never null.
+        signal_.compare_exchange(0, signal);
-    const cancellation_group_ref_t cancel_group{};
+    }
 
     /// If set, the saved terminal modes of this job. This needs to be saved so that we can restore
     /// the terminal to the same state when resuming a stopped job.
@@ -108,17 +72,16 @@ class job_group_t {
     maybe_t<pid_t> get_pgid() const { return pgid_; }
 
     /// Construct a group for a job that will live internal to fish, optionally claiming a job ID.
-    static job_group_ref_t create(wcstring command, cancellation_group_ref_t cg, bool wants_job_id);
+    static job_group_ref_t create(wcstring command, bool wants_job_id);
 
     /// Construct a group for a job which will assign its first process as pgroup leader.
-    static job_group_ref_t create_with_job_control(wcstring command, cancellation_group_ref_t cg,
+    static job_group_ref_t create_with_job_control(wcstring command, bool wants_terminal);
-                                                   bool wants_terminal);
 
     ~job_group_t();
 
    private:
-    job_group_t(wcstring command, cancellation_group_ref_t cg, job_id_t job_id,
+    job_group_t(wcstring command, job_id_t job_id, bool job_control = false,
-                bool job_control = false, bool wants_terminal = false);
+                bool wants_terminal = false);
 
     // Whether job control is enabled.
     // If this is set, then the first process in the root job must be external.
@@ -140,6 +103,9 @@ class job_group_t {
 
     /// Our job ID. -1 if none.
     const job_id_t job_id_;
+
+    /// The signal causing us the group to cancel, or 0.
+    relaxed_atomic_t<int> signal_{0};
 };
 
 #endif

src/parse_execution.cpp

@@ -119,12 +119,10 @@ static redirection_spec_t get_stderr_merge() {
 
 parse_execution_context_t::parse_execution_context_t(parsed_source_ref_t pstree,
                                                      const operation_context_t &ctx,
-                                                     cancellation_group_ref_t cancel_group,
                                                      io_chain_t block_io)
     : pstree(std::move(pstree)),
       parser(ctx.parser.get()),
       ctx(ctx),
-      cancel_group(std::move(cancel_group)),
       block_io(std::move(block_io)) {}
 
 // Utilities
@@ -228,7 +226,9 @@ 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 (ctx.check_cancel() || check_cancel_from_fish_signal()) {
+    // If one of our jobs ended with SIGINT, we stop execution.
+    // Likewise if fish itself got a SIGINT, or if something ran exit, etc.
+    if (cancel_signal || ctx.check_cancel() || check_cancel_from_fish_signal()) {
         return end_execution_reason_t::cancelled;
     }
     const auto &ld = parser->libdata();
@@ -1387,6 +1387,9 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo
         if (job->has_external_proc()) {
             parser->vars().universal_barrier();
         }
+
+        // If the job got a SIGINT or SIGQUIT, then we're going to start unwinding.
+        if (!cancel_signal) cancel_signal = job->group->get_cancel_signal();
     }
 
     if (profile_item != nullptr) {
@@ -1535,12 +1538,12 @@ void parse_execution_context_t::setup_group(job_t *j) {
 
     if (j->processes.front()->is_internal() || !this->use_job_control()) {
         // This job either doesn't have a pgroup (e.g. a simple block), or lives in fish's pgroup.
-        j->group = job_group_t::create(j->command(), cancel_group, j->wants_job_id());
+        j->group = job_group_t::create(j->command(), j->wants_job_id());
     } else {
         // This is a "real job" that gets its own pgroup.
         j->processes.front()->leads_pgrp = true;
         bool wants_terminal = !parser->libdata().is_event;
-        j->group = job_group_t::create_with_job_control(j->command(), cancel_group, wants_terminal);
+        j->group = job_group_t::create_with_job_control(j->command(), wants_terminal);
     }
     j->group->set_is_foreground(!j->is_initially_background());
     j->mut_flags().is_group_root = true;

src/parse_execution.h

@@ -12,7 +12,6 @@
 #include "proc.h"
 
 class block_t;
-class cancellation_group_t;
 class operation_context_t;
 class parser_t;
 
@@ -38,7 +37,10 @@ class parse_execution_context_t : noncopyable_t {
     parsed_source_ref_t pstree;
     parser_t *const parser;
     const operation_context_t &ctx;
-    const std::shared_ptr<cancellation_group_t> cancel_group;
+
+    // If set, one of our processes received a cancellation signal (INT or QUIT) so we are
+    // unwinding.
+    int cancel_signal{0};
 
     // The currently executing job node, used to indicate the line number.
     const ast::job_t *executing_job_node{};
@@ -153,10 +155,8 @@ class parse_execution_context_t : noncopyable_t {
 
    public:
     /// Construct a context in preparation for evaluating a node in a tree, with the given block_io.
-    /// The cancel group is never null and should be provided when resolving job groups.
     /// The execution context may access the parser and parent job group (if any) through ctx.
     parse_execution_context_t(parsed_source_ref_t pstree, const operation_context_t &ctx,
-                              std::shared_ptr<cancellation_group_t> cancel_group,
                               io_chain_t block_io);
 
     /// Returns the current line number, indexed from 1. Not const since it touches

src/parser.cpp

@@ -631,10 +631,6 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, const T &node,
         }
     }
 
-    // If we are provided a cancellation group, use it; otherwise create one.
-    cancellation_group_ref_t cancel_group =
-        job_group ? job_group->cancel_group : cancellation_group_t::create();
-
     // 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).
@@ -642,7 +638,7 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, const T &node,
         // Did fish itself get a signal?
         int sig = signal_check_cancel();
         // Has this job group been cancelled?
-        if (!sig) sig = cancel_group->get_cancel_signal();
+        if (!sig && job_group) sig = job_group->get_cancel_signal();
         return sig;
     };
 
@@ -665,8 +661,8 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, const T &node,
 
     // 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, make_unique<parse_execution_context_t>(
+    scoped_push<exc_ctx_ref_t> exc(&execution_context,
-                                                           ps, op_ctx, cancel_group, block_io));
+                                   make_unique<parse_execution_context_t>(ps, op_ctx, block_io));
 
     // Check the exec count so we know if anything got executed.
     const size_t prev_exec_count = libdata().exec_count;

tests/pexpects/sigint.py

@@ -19,6 +19,8 @@ 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 $status")
+expect_str("130")
 
 sendline("echo it worked")
 expect_str("it worked")