From 7b1321f9a1b4f3736404627bed05821f6d9d622a Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 20 Mar 2022 14:32:18 -0700 Subject: [PATCH] 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 | 16 +++++------ src/job_group.h | 58 +++++++++------------------------------- src/parse_execution.cpp | 13 +++++---- src/parse_execution.h | 8 +++--- src/parser.cpp | 10 +++---- tests/pexpects/sigint.py | 2 ++ 6 files changed, 35 insertions(+), 72 deletions(-) diff --git a/src/job_group.cpp b/src/job_group.cpp index 8576906fe..c2113592c 100644 --- a/src/job_group.cpp +++ b/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, - bool job_control, bool wants_terminal) - : cancel_group(std::move(cg)), - job_control_(job_control), +job_group_t::job_group_t(wcstring command, job_id_t job_id, bool job_control, bool wants_terminal) + : 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, - bool wants_job_id) { +job_group_ref_t job_group_t::create(wcstring command, 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, - bool wants_terminal) { - return job_group_ref_t(new job_group_t(std::move(command), std::move(cg), acquire_job_id(), +job_group_ref_t job_group_t::create_with_job_control(wcstring command, bool wants_terminal) { + return job_group_ref_t(new job_group_t(std::move(command), acquire_job_id(), true /* job_control */, wants_terminal)); } diff --git a/src/job_group.h b/src/job_group.h index 0a10027e0..0b2b9a511 100644 --- a/src/job_group.h +++ b/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 create() { - return std::make_shared(); - } - - private: - /// If we cancelled from a signal, return that signal, else 0. - relaxed_atomic_t signal_{0}; -}; -using cancellation_group_ref_t = std::shared_ptr; - /// 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); } - - /// The cancellation group. This is never null. - const cancellation_group_ref_t cancel_group{}; + void cancel_with_signal(int signal) { + assert(signal > 0 && "Invalid cancel signal"); + signal_.compare_exchange(0, signal); + } /// 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 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, - bool wants_terminal); + static job_group_ref_t create_with_job_control(wcstring command, bool wants_terminal); ~job_group_t(); private: - job_group_t(wcstring command, cancellation_group_ref_t cg, job_id_t job_id, - bool job_control = false, bool wants_terminal = false); + job_group_t(wcstring command, job_id_t job_id, bool job_control = 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 signal_{0}; }; #endif diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index c248caea2..67a8031b6 100644 --- a/src/parse_execution.cpp +++ b/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 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; diff --git a/src/parse_execution.h b/src/parse_execution.h index 9695af66e..19e56b963 100644 --- a/src/parse_execution.h +++ b/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 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 cancel_group, io_chain_t block_io); /// Returns the current line number, indexed from 1. Not const since it touches diff --git a/src/parser.cpp b/src/parser.cpp index bdbc0ac37..8eb1085b4 100644 --- a/src/parser.cpp +++ b/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; - scoped_push exc(&execution_context, make_unique( - ps, op_ctx, cancel_group, block_io)); + scoped_push exc(&execution_context, + make_unique(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; diff --git a/tests/pexpects/sigint.py b/tests/pexpects/sigint.py index 05113031c..35e1a7453 100644 --- a/tests/pexpects/sigint.py +++ b/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")