Correctly propagate signals from cancelled jobs into parse_execution_context

This concerns code like the following:

    while true ; sleep 100; end

Here 'while' is a "simple block execution" and does not create a new job,
or get a pgid. Each 'sleep' however is an external command execution, and
is treated as a distinct job. (bash is the same way). So `while` and
`sleep` are always in different job groups.

The problem comes about if 'sleep' is cancelled through SIGINT or SIGQUIT.
Prior to 2a4c545b21, if *any* process got a SIGINT or SIGQUIT, then fish
would mark a global "stop executing" variable. This obviously prevents
background execution of fish functions.

In 2a4c545b21, this was changed so only the job's group gets marked as
cancelled. However in the case of one job group spawning another, we
weren't propagating the signal.

This adds a signal to parse_execution_context which the parser checks after
execution. It's not ideal since now we have three different places where
signals can be recorded. However it fixes this regression which is too
important to leave unfixed for long.

Fixes #7259
This commit is contained in:
ridiculousfish 2020-08-12 21:35:37 -07:00
parent 1cf835e6e9
commit 82fed6fc2f
4 changed files with 51 additions and 5 deletions

View file

@ -226,12 +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 { maybe_t<end_execution_reason_t> parse_execution_context_t::check_end_execution() const {
if (ctx.check_cancel() || shell_is_exiting()) { if (this->cancel_signal || ctx.check_cancel() || shell_is_exiting()) {
return end_execution_reason_t::cancelled; return end_execution_reason_t::cancelled;
} }
if (nullptr == parser) {
return none();
}
const auto &ld = parser->libdata(); const auto &ld = parser->libdata();
if (ld.returning) { if (ld.returning) {
return end_execution_reason_t::control_flow; return end_execution_reason_t::control_flow;
@ -1343,6 +1340,14 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo
remove_job(*this->parser, job.get()); 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. // Update universal variables on external conmmands.
// TODO: justify this, why not on every command? // TODO: justify this, why not on every command?
if (job_contained_external_command) { if (job_contained_external_command) {

View file

@ -45,6 +45,10 @@ class parse_execution_context_t {
size_t cached_lineno_offset = 0; size_t cached_lineno_offset = 0;
int cached_lineno_count = 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. /// The block IO chain.
/// For example, in `begin; foo ; end < file.txt` this would have the 'file.txt' IO. /// For example, in `begin; foo ; end < file.txt` this would have the 'file.txt' IO.
io_chain_t block_io{}; io_chain_t block_io{};
@ -160,6 +164,9 @@ class parse_execution_context_t {
/// Returns the source offset, or -1. /// Returns the source offset, or -1.
int get_current_source_offset() const; 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. /// Returns the source string.
const wcstring &get_source() const { return pstree->src; } const wcstring &get_source() const { return pstree->src; }

View file

@ -699,12 +699,21 @@ 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_exec_count = libdata().exec_count;
const size_t new_status_count = libdata().status_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(); exc.restore();
this->pop_block(scope_block); this->pop_block(scope_block);
job_reap(*this, false); // reap again job_reap(*this, false); // reap again
if (int sig = check_cancel_signal()) { 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. // We were signalled.
return proc_status_t::from_signal(sig); return proc_status_t::from_signal(sig);
} else { } else {

25
tests/pexpects/sigint.py Normal file
View file

@ -0,0 +1,25 @@
#!/usr/bin/env python3
from pexpect_helper import SpawnedProc
sp = SpawnedProc()
sendline, sleep, expect_prompt, expect_str = (
sp.sendline,
sp.sleep,
sp.expect_prompt,
sp.expect_str,
)
# Ensure that if child processes SIGINT, we exit our loops.
# This is an interactive test because the parser is expected to
# recover from SIGINT in interactive mode.
# Test for #7259.
expect_prompt()
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 it worked")
expect_str("it worked")
expect_prompt()