finish cleanup of signal blocking code

PR #3691 made most calls to `signal_block()` and `signal_unblock()`
no-ops unless a magic env var is set when fish starts running. It's
been seven months since that change was made and no problems have been
reported. This finishes that work by removing those no-op function calls
and support for the magic env var in our next major release (which won't
happen till at least six months from now).

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);
@@ -399,10 +394,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
@@ -446,8 +439,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
@@ -618,9 +609,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);
@@ -628,8 +616,6 @@ void exec_job(parser_t &parser, job_t *j) {
                 const std::map<wcstring, env_var_t> inherit_vars =
                     function_get_inherit_vars(func_name);
 
-                signal_block();
-
                 if (!function_exists) {
                     debug(0, _(L"Unknown function '%ls'"), p->argv0());
                     break;
@@ -637,13 +623,7 @@ void exec_job(parser_t &parser, job_t *j) {
 
                 function_block_t *fb =
                     parser.push_block<function_block_t>(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) {
@@ -794,12 +774,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);
@@ -1108,9 +1084,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;

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<bool>(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);
 

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();

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;
 }
 

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;
 }
 

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.

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<bool>(block_count);
 }

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