diff --git a/src/global_safety.h b/src/global_safety.h index b0a74320c..ec2c3ba46 100644 --- a/src/global_safety.h +++ b/src/global_safety.h @@ -97,6 +97,11 @@ class relaxed_atomic_t { void operator=(T v) { return value_.store(v, std::memory_order_relaxed); } + // Perform a CAS operation, returning whether it succeeded. + bool compare_exchange(T expected, T desired) { + return value_.compare_exchange_strong(expected, desired, std::memory_order_relaxed); + } + // postincrement T operator++(int) { return value_.fetch_add(1, std::memory_order_relaxed); } diff --git a/src/job_group.cpp b/src/job_group.cpp index 73099298b..2df7f0aba 100644 --- a/src/job_group.cpp +++ b/src/job_group.cpp @@ -49,8 +49,10 @@ maybe_t job_group_t::get_pgid() const { return pgid_; } // static job_group_ref_t job_group_t::resolve_group_for_job(const job_t &job, + const cancellation_group_ref_t &cancel_group, const job_group_ref_t &proposed) { assert(!job.group && "Job already has a group"); + assert(cancel_group && "Null cancel group"); // Note there's three cases to consider: // nullptr -> this is a root job, there is no inherited job group // internal -> the parent is running as part of a simple function execution @@ -77,13 +79,19 @@ job_group_ref_t job_group_t::resolve_group_for_job(const job_t &job, if (!needs_new_group) return proposed; - // We will need to create a new group. + // We share a cancel group unless we are a background job. + // For example, if we write "begin ; true ; sleep 1 &; end" the `begin` and `true` should cancel + // together, but the `sleep` should not. + cancellation_group_ref_t resolved_cg = + initial_bg ? cancellation_group_t::create() : cancel_group; + properties_t props{}; props.job_control = job.wants_job_control(); props.wants_terminal = job.wants_job_control() && !job.from_event_handler(); props.is_internal = can_use_internal; props.job_id = can_use_internal ? -1 : acquire_job_id(); - job_group_ref_t result{new job_group_t(props, job.command())}; + + job_group_ref_t result{new job_group_t(props, resolved_cg, job.command())}; // Mark if it's foreground. result->set_is_foreground(!initial_bg); diff --git a/src/job_group.h b/src/job_group.h index 90da659bd..92b578d70 100644 --- a/src/job_group.h +++ b/src/job_group.h @@ -13,6 +13,42 @@ /// 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 @@ -60,13 +96,13 @@ class job_group_t { 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_; } + int get_cancel_signal() const { return cancel_group->get_cancel_signal(); } /// \return the command which produced this job tree. const wcstring &get_command() const { return command_; } /// Mark that a process in this group got a signal, and so should cancel. - void set_cancel_signal(int sig) { cancel_signal_ = sig; } + void cancel_with_signal(int sig) { cancel_group->cancel_with_signal(sig); } /// Mark the root as constructed. /// This is used to avoid reaping a process group leader while there are still procs that may @@ -78,6 +114,7 @@ class job_group_t { /// The proposed group is the group from the parent job, or null if this is a root. /// This never returns null. static job_group_ref_t resolve_group_for_job(const job_t &job, + const cancellation_group_ref_t &cancel_group, const job_group_ref_t &proposed_group); ~job_group_t(); @@ -87,6 +124,9 @@ class job_group_t { /// stops. maybe_t tmodes{}; + /// The cancellation group. This is never null. + const cancellation_group_ref_t cancel_group{}; + private: // The pgid to assign to jobs, or none if not yet set. maybe_t pgid_{}; @@ -117,11 +157,10 @@ 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_{}; - - job_group_t(const properties_t &props, wcstring command) - : props_(props), command_(std::move(command)) {} + job_group_t(const properties_t &props, cancellation_group_ref_t cg, wcstring command) + : cancel_group(std::move(cg)), props_(props), command_(std::move(command)) { + assert(cancel_group && "Null cancel group"); + } }; #endif diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index eb1864e83..f9dcf2ec8 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -119,10 +119,12 @@ 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 @@ -226,7 +228,7 @@ process_type_t parse_execution_context_t::process_type_for_command( } maybe_t parse_execution_context_t::check_end_execution() const { - if (this->cancel_signal || ctx.check_cancel() || check_cancel_from_fish_signal()) { + if (ctx.check_cancel() || check_cancel_from_fish_signal()) { return end_execution_reason_t::cancelled; } const auto &ld = parser->libdata(); @@ -1343,7 +1345,7 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo // Clean up the job on failure or cancellation. if (pop_result == end_execution_reason_t::ok) { // Resolve the job's group and mark if this job is the first to get it. - job->group = job_group_t::resolve_group_for_job(*job, ctx.job_group); + job->group = job_group_t::resolve_group_for_job(*job, cancel_group, ctx.job_group); assert(job->group && "Should not have a null group"); job->mut_flags().is_group_root = (job->group != ctx.job_group); @@ -1364,14 +1366,6 @@ 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 4af3f95d1..c62ed961f 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -12,6 +12,7 @@ #include "proc.h" class block_t; +class cancellation_group_t; class operation_context_t; class parser_t; @@ -37,6 +38,7 @@ class parse_execution_context_t { parsed_source_ref_t pstree; parser_t *const parser; const operation_context_t &ctx; + const std::shared_ptr cancel_group; // The currently executing job node, used to indicate the line number. const ast::job_t *executing_job_node{}; @@ -45,10 +47,6 @@ 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{}; @@ -153,8 +151,10 @@ class parse_execution_context_t { public: /// Construct a context in preparation for evaluating a node in a tree, with the given block_io. - /// The execution context may access the parser and group through ctx. + /// 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 @@ -164,9 +164,6 @@ 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 1c5b5ce17..fe4a7bdc1 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -659,6 +659,10 @@ 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). @@ -666,7 +670,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 && job_group) sig = job_group->get_cancel_signal(); + if (!sig) sig = cancel_group->get_cancel_signal(); return sig; }; @@ -689,8 +693,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, block_io)); + scoped_push exc(&execution_context, make_unique( + ps, op_ctx, cancel_group, block_io)); // Check the exec count so we know if anything got executed. const size_t prev_exec_count = libdata().exec_count; @@ -699,22 +703,12 @@ 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 (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. + if (int sig = check_cancel_signal()) { return proc_status_t::from_signal(sig); } else { auto status = proc_status_t::from_exit_code(this->get_last_status()); diff --git a/src/proc.cpp b/src/proc.cpp index 6d777e1ce..5a30051be 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -263,7 +263,7 @@ static void handle_child_status(const shared_ptr &job, process_t *proc, if (sig == SIGINT || sig == SIGQUIT) { if (session_interactivity() != session_interactivity_t::not_interactive) { // Mark the job group as cancelled. - job->group->set_cancel_signal(sig); + job->group->cancel_with_signal(sig); } else { // Deliver the SIGINT or SIGQUIT signal to ourself since we're not interactive. struct sigaction act;