diff --git a/src/event.cpp b/src/event.cpp index d9791fd6f..3a208735c 100644 --- a/src/event.cpp +++ b/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> to_send; to_send.swap(ld.blocked_events); diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index b676c1f58..e85d2aa05 100644 --- a/src/fish_tests.cpp +++ b/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 { diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index a9115036e..9cf937fc3 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -225,15 +225,12 @@ process_type_t parse_execution_context_t::process_type_for_command( } maybe_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."); } diff --git a/src/parser.cpp b/src/parser.cpp index ceca6172e..67ece63e0 100644 --- a/src/parser.cpp +++ b/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 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::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::value || std::is_same::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; scoped_push 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); diff --git a/src/parser.h b/src/parser.h index 4987c706d..0bfb23689 100644 --- a/src/parser.h +++ b/src/parser.h @@ -220,10 +220,7 @@ struct eval_res_t { class parser_t : public std::enable_shared_from_this { 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 execution_context; /// The jobs associated with this parser. @@ -269,12 +266,6 @@ class parser_t : public std::enable_shared_from_this { /// 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 { 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; diff --git a/src/proc.cpp b/src/proc.cpp index b8d71bfbc..9fd8810c1 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -321,7 +321,8 @@ void job_mark_process_as_failed(const std::shared_ptr &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, 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 - // executing so control-C returns control to the user. - parser_t::cancel_requested(sig); + // Mark the job group as cancelled. + job->group->set_cancel_signal(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); } diff --git a/src/proc.h b/src/proc.h index 2ba6a1ebb..416778011 100644 --- a/src/proc.h +++ b/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) {} }; diff --git a/src/reader.cpp b/src/reader.cpp index 2e2af7c41..679be8bd7 100644 --- a/src/reader.cpp +++ b/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 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; } diff --git a/src/signal.cpp b/src/signal.cpp index 367879d6f..65424ec58 100644 --- a/src/signal.cpp +++ b/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; diff --git a/src/signal.h b/src/signal.h index 6292b464a..abc3a4a57 100644 --- a/src/signal.h +++ b/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 {