Revert "Revert "finish cleanup of signal blocking code""

This reverts commit 35ee28ff24.

Reapply the signal blocking cleanup change on top of the job control
changes made by @mqudsi.

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<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;
@@ -728,13 +716,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);
 
                 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;

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

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

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

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
+        // it.
-        // to reset a bunch of signal state, making this coda a but unobvious.
         //
         // 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) {
+void signal_block() {
-    if (!force && ignore_signal_block) return;
-
     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) {
+void signal_unblock() {
-    if (!force && ignore_signal_block) return;
-
     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