From 7dd2cd7de76ffe1305823d737022a9bf01e1511b Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Sun, 13 Aug 2017 15:30:34 -0700 Subject: [PATCH] Revert "Revert "finish cleanup of signal blocking code"" This reverts commit 35ee28ff24c7bca933d477029c7f3386cfbaf9a3. It was meant for the major branch. --- src/exec.cpp | 28 +--------------------------- src/fish.cpp | 7 ------- src/history.cpp | 9 --------- src/postfork.cpp | 6 ++---- src/proc.cpp | 16 ++++++++-------- src/reader.cpp | 16 ++-------------- src/signal.cpp | 16 +++------------- src/signal.h | 6 ++---- 8 files changed, 18 insertions(+), 86 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 047df6f71..5af251572 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -36,7 +36,6 @@ #include "postfork.h" #include "proc.h" #include "reader.h" -#include "signal.h" #include "wutil.h" // IWYU pragma: keep /// File descriptor redirection error message. @@ -322,16 +321,12 @@ static void internal_exec_helper(parser_t &parser, const wcstring &def, node_off return; } - signal_unblock(); - if (node_offset == NODE_OFFSET_INVALID) { parser.eval(def, morphed_chain, block_type); } else { parser.eval_block_node(node_offset, morphed_chain, block_type); } - signal_block(); - morphed_chain.clear(); io_cleanup_fds(opened_fds); job_reap(0); @@ -401,10 +396,8 @@ void exec_job(parser_t &parser, job_t *j) { if (j->processes.front()->type == INTERNAL_EXEC) { // Do a regular launch - but without forking first... - signal_block(); - // setup_child_process makes sure signals are properly set up. It will also call - // signal_unblock. + // setup_child_process makes sure signals are properly set up. // PCA This is for handling exec. Passing all_ios here matches what fish 2.0.0 and 1.x did. // It's known to be wrong - for example, it means that redirections bound for subsequent @@ -448,8 +441,6 @@ void exec_job(parser_t &parser, job_t *j) { } } - signal_block(); - // See if we need to create a group keepalive process. This is a process that we create to make // sure that the process group doesn't die accidentally, and is often needed when a // builtin/block/function is inside a pipeline, since that usually means we have to wait for one @@ -620,9 +611,6 @@ void exec_job(parser_t &parser, job_t *j) { switch (p->type) { case INTERNAL_FUNCTION: { - // Calls to function_get_definition might need to source a file as a part of - // autoloading, hence there must be no blocks. - signal_unblock(); const wcstring func_name = p->argv0(); wcstring def; bool function_exists = function_get_definition(func_name, &def); @@ -630,8 +618,6 @@ void exec_job(parser_t &parser, job_t *j) { const std::map inherit_vars = function_get_inherit_vars(func_name); - signal_block(); - if (!function_exists) { debug(0, _(L"Unknown function '%ls'"), p->argv0()); break; @@ -639,13 +625,7 @@ void exec_job(parser_t &parser, job_t *j) { function_block_t *fb = parser.push_block(p, func_name, shadow_scope); - - // Setting variables might trigger an event handler, hence we need to unblock - // signals. - signal_unblock(); function_prepare_environment(func_name, p->get_argv() + 1, inherit_vars); - signal_block(); - parser.forbid_function(func_name); if (!p->is_last_in_job) { @@ -795,12 +775,8 @@ void exec_job(parser_t &parser, job_t *j) { const int fg = j->get_flag(JOB_FOREGROUND); j->set_flag(JOB_FOREGROUND, false); - signal_unblock(); - p->status = builtin_run(parser, p->get_argv(), *builtin_io_streams); - signal_block(); - // Restore the fg flag, which is temporarily set to false during builtin // execution so as not to confuse some job-handling builtins. j->set_flag(JOB_FOREGROUND, fg); @@ -1113,9 +1089,7 @@ void exec_job(parser_t &parser, job_t *j) { kill(keepalive.pid, SIGKILL); } - signal_unblock(); debug(3, L"Job is constructed"); - j->set_flag(JOB_CONSTRUCTED, true); if (!j->get_flag(JOB_FOREGROUND)) { proc_last_bg_pid = j->pgid; diff --git a/src/fish.cpp b/src/fish.cpp index 273c85ab8..096805a6e 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -387,13 +387,6 @@ int main(int argc, char **argv) { const io_chain_t empty_ios; if (read_init(paths)) { - // TODO: Remove this once we're confident that not blocking/unblocking every signal around - // some critical sections is no longer necessary. - env_var_t fish_no_signal_block = env_get_string(L"FISH_NO_SIGNAL_BLOCK"); - if (!fish_no_signal_block.missing_or_empty() && !from_string(fish_no_signal_block)) { - ignore_signal_block = false; - } - // Stomp the exit status of any initialization commands (issue #635). proc_set_last_status(STATUS_CMD_OK); diff --git a/src/history.cpp b/src/history.cpp index 5b748c8ce..a293933b9 100644 --- a/src/history.cpp +++ b/src/history.cpp @@ -39,7 +39,6 @@ #include "parse_util.h" #include "path.h" #include "reader.h" -#include "signal.h" #include "wutil.h" // IWYU pragma: keep // Our history format is intended to be valid YAML. Here it is: @@ -998,9 +997,6 @@ bool history_t::load_old_if_needed(void) { if (loaded_old) return true; loaded_old = true; - // PCA not sure why signals were blocked here - // signal_block(); - bool ok = false; if (map_file(name, &mmap_start, &mmap_length, &mmap_file_id)) { // Here we've mapped the file. @@ -1009,7 +1005,6 @@ bool history_t::load_old_if_needed(void) { this->populate_from_mmap(); } - // signal_unblock(); return ok; } @@ -1409,8 +1404,6 @@ bool history_t::save_internal_via_appending() { return true; } - signal_block(); - // We are going to open the file, lock it, append to it, and then close it // After locking it, we need to stat the file at the path; if there is a new file there, it // means @@ -1498,8 +1491,6 @@ bool history_t::save_internal_via_appending() { close(history_fd); } - signal_unblock(); - // If someone has replaced the file, forget our file state. if (file_changed) { this->clear_file_state(); diff --git a/src/postfork.cpp b/src/postfork.cpp index 9f29963b0..8dc49b298 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -109,9 +109,9 @@ bool set_child_group(job_t *j, process_t *p, int print_errors) { int result = -1; errno = EINTR; while (result == -1 && errno == EINTR) { - signal_block(true); + signal_block(); result = tcsetpgrp(STDIN_FILENO, j->pgid); - signal_unblock(true); + signal_unblock(); } if (result == -1) { if (errno == ENOTTY) redirect_tty_output(); @@ -251,8 +251,6 @@ int setup_child_process(job_t *j, process_t *p, const io_chain_t &io_chain) { signal_reset_handlers(); } - signal_unblock(); // remove all signal blocks - return ok ? 0 : -1; } diff --git a/src/proc.cpp b/src/proc.cpp index 063f01feb..cdbea292b 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -789,7 +789,7 @@ static bool terminal_give_to_job(job_t *j, int cont) { return true; } - signal_block(true); + signal_block(); int result = -1; errno = EINTR; while (result == -1 && errno == EINTR) { @@ -799,7 +799,7 @@ static bool terminal_give_to_job(job_t *j, int cont) { if (errno == ENOTTY) redirect_tty_output(); debug(1, _(L"Could not send job %d ('%ls') to foreground"), j->job_id, j->command_wcstr()); wperror(L"tcsetpgrp"); - signal_unblock(true); + signal_unblock(); return false; } @@ -817,12 +817,12 @@ static bool terminal_give_to_job(job_t *j, int cont) { debug(1, _(L"terminal_give_to_job(): Could not send job %d ('%ls') to foreground"), j->job_id, j->command_wcstr()); wperror(L"tcsetattr"); - signal_unblock(true); + signal_unblock(); return false; } } - signal_unblock(true); + signal_unblock(); return true; } @@ -835,12 +835,12 @@ static bool terminal_return_from_job(job_t *j) { return true; } - signal_block(true); + signal_block(); if (tcsetpgrp(STDIN_FILENO, getpgrp()) == -1) { if (errno == ENOTTY) redirect_tty_output(); debug(1, _(L"Could not return shell to foreground")); wperror(L"tcsetpgrp"); - signal_unblock(true); + signal_unblock(); return false; } @@ -849,7 +849,7 @@ static bool terminal_return_from_job(job_t *j) { if (errno == EIO) redirect_tty_output(); debug(1, _(L"Could not return shell to foreground")); wperror(L"tcgetattr"); - signal_unblock(true); + signal_unblock(); return false; } @@ -867,7 +867,7 @@ static bool terminal_return_from_job(job_t *j) { } #endif - signal_unblock(true); + signal_unblock(); return true; } diff --git a/src/reader.cpp b/src/reader.cpp index c7d3d8afd..7daccce2a 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -1575,20 +1575,12 @@ static void reader_interactive_init() { // Check if we are in control of the terminal, so that we don't do semi-expensive things like // reset signal handlers unless we really have to, which we often don't. if (tcgetpgrp(STDIN_FILENO) != shell_pgid) { - int block_count = 0; - int i; - // Bummer, we are not in control of the terminal. Stop until parent has given us control of - // it. Stopping in fish is a bit of a challange, what with all the signal fidgeting, we need - // to reset a bunch of signal state, making this coda a but unobvious. + // it. // // In theory, reseting signal handlers could cause us to miss signal deliveries. In - // practice, this code should only be run suring startup, when we're not waiting for any + // practice, this code should only be run during startup, when we're not waiting for any // signals. - while (signal_is_blocked()) { - signal_unblock(); - block_count++; - } signal_reset_handlers(); // Ok, signal handlers are taken out of the picture. Stop ourself in a loop until we are in @@ -1632,10 +1624,6 @@ static void reader_interactive_init() { } signal_set_handlers(); - - for (i = 0; i < block_count; i++) { - signal_block(); - } } // Put ourselves in our own process group. diff --git a/src/signal.cpp b/src/signal.cpp index 1b937825c..f36847b59 100644 --- a/src/signal.cpp +++ b/src/signal.cpp @@ -16,9 +16,6 @@ #include "reader.h" #include "wutil.h" // IWYU pragma: keep -// This is a temporary var while we explore whether signal_block() and friends is needed. -bool ignore_signal_block = true; - /// Struct describing an entry for the lookup table used to convert between signal names and signal /// ids, etc. struct lookup_entry { @@ -383,9 +380,7 @@ void get_signals_with_handlers(sigset_t *set) { } } -void signal_block(bool force) { - if (!force && ignore_signal_block) return; - +void signal_block() { ASSERT_IS_MAIN_THREAD(); sigset_t chldset; @@ -405,14 +400,10 @@ void signal_unblock_all() { sigprocmask(SIG_SETMASK, &iset, NULL); } -void signal_unblock(bool force) { - if (!force && ignore_signal_block) return; - +void signal_unblock() { ASSERT_IS_MAIN_THREAD(); - sigset_t chldset; block_count--; - if (block_count < 0) { debug(0, _(L"Signal block mismatch")); bugreport(); @@ -420,13 +411,12 @@ void signal_unblock(bool force) { } if (!block_count) { + sigset_t chldset; sigfillset(&chldset); DIE_ON_FAILURE(pthread_sigmask(SIG_UNBLOCK, &chldset, 0)); } - // debug( 0, L"signal block level decreased to %d", block_count ); } bool signal_is_blocked() { - if (ignore_signal_block) return false; return static_cast(block_count); } diff --git a/src/signal.h b/src/signal.h index 06fe37cf9..dcbc3be75 100644 --- a/src/signal.h +++ b/src/signal.h @@ -30,16 +30,14 @@ void signal_handle(int sig, int do_handle); void signal_unblock_all(); /// Block all signals. -void signal_block(bool force = false); +void signal_block(); /// Unblock all signals. -void signal_unblock(bool force = false); +void signal_unblock(); /// Returns true if signals are being blocked. bool signal_is_blocked(); /// Returns signals with non-default handlers. void get_signals_with_handlers(sigset_t *set); - -extern bool ignore_signal_block; #endif