From 52d739c746034e0dcd554001f5927ad2f32a20fc Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Sun, 6 Aug 2017 14:46:12 -0700 Subject: [PATCH] Revert "Revert "finish cleanup of signal blocking code"" This reverts commit 35ee28ff24c7bca933d477029c7f3386cfbaf9a3. Reapply the signal blocking cleanup change on top of the job control changes made by @mqudsi. --- src/exec.cpp | 25 ------------------------- src/fish.cpp | 7 ------- src/history.cpp | 9 --------- src/postfork.cpp | 2 -- src/proc.cpp | 14 +++++++------- src/reader.cpp | 16 ++-------------- src/signal.cpp | 16 +++------------- src/signal.h | 6 ++---- 8 files changed, 14 insertions(+), 81 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 891159005..18b96ffb0 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -37,7 +37,6 @@ #include "postfork.h" #include "proc.h" #include "reader.h" -#include "signal.h" #include "wutil.h" // IWYU pragma: keep /// File descriptor redirection error message. @@ -323,16 +322,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); @@ -454,8 +449,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 @@ -709,9 +702,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); @@ -719,8 +709,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; @@ -728,13 +716,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); verify_buffer_output(); @@ -863,13 +845,8 @@ void exec_job(parser_t &parser, job_t *j) { //main loop may need to perform a blocking read from previous command's output. //make sure read source is not blocked unblock_previous(); - - 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); @@ -1203,9 +1180,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 4688cff73..8b79c4f7f 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -277,8 +277,6 @@ int setup_child_process(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 97098cf81..07e00b618 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -789,7 +789,7 @@ bool terminal_give_to_job(job_t *j, int cont) { return true; } - signal_block(true); + signal_block(); //It may not be safe to call tcsetpgrp if we've already done so, as at that point we are no longer //the controlling process group for the terminal and no longer have permission to set the process @@ -872,12 +872,12 @@ 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"tcsetattr"); - signal_unblock(true); + signal_unblock(); return false; } } - signal_unblock(true); + signal_unblock(); return true; } @@ -890,12 +890,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; } @@ -904,7 +904,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; } @@ -922,7 +922,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 28dcbcf0e..89a0d767d 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