Revert "finish cleanup of signal blocking code"

This reverts commit fb08fe5f47.

Needed to cleanly apply PR#4268. Will reapply after applying that
change.

src/exec.cpp

@@ -36,6 +36,7 @@
 #include "postfork.h"
 #include "proc.h"
 #include "reader.h"
+#include "signal.h"
 #include "wutil.h"  // IWYU pragma: keep
 
 /// File descriptor redirection error message.
@@ -321,12 +322,16 @@ 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);
@@ -396,8 +401,10 @@ 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.
+        // setup_child_process makes sure signals are properly set up. It will also call
+        // signal_unblock.
 
         // 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
@@ -441,6 +448,8 @@ 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
@@ -611,6 +620,9 @@ 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);
@@ -618,6 +630,8 @@ 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;
@@ -625,7 +639,13 @@ 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) {
@@ -775,8 +795,12 @@ 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);
@@ -1089,7 +1113,9 @@ 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,6 +387,13 @@ 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,6 +39,7 @@
 #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:
@@ -997,6 +998,9 @@ 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.
@@ -1005,6 +1009,7 @@ bool history_t::load_old_if_needed(void) {
         this->populate_from_mmap();
     }
 
+    // signal_unblock();
     return ok;
 }
 
@@ -1404,6 +1409,8 @@ 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
@@ -1491,6 +1498,8 @@ 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();
+            signal_block(true);
             result = tcsetpgrp(STDIN_FILENO, j->pgid);
-            signal_unblock();
+            signal_unblock(true);
         }
         if (result == -1) {
             if (errno == ENOTTY) redirect_tty_output();
@@ -251,6 +251,8 @@ 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();
+    signal_block(true);
     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();
+        signal_unblock(true);
         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();
+            signal_unblock(true);
             return false;
         }
     }
 
-    signal_unblock();
+    signal_unblock(true);
     return true;
 }
 
@@ -835,12 +835,12 @@ static bool terminal_return_from_job(job_t *j) {
         return true;
     }
 
-    signal_block();
+    signal_block(true);
     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();
+        signal_unblock(true);
         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();
+        signal_unblock(true);
         return false;
     }
 
@@ -867,7 +867,7 @@ static bool terminal_return_from_job(job_t *j) {
     }
 #endif
 
-    signal_unblock();
+    signal_unblock(true);
     return true;
 }
 

src/reader.cpp

@@ -1575,12 +1575,20 @@ 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.
+        // 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.
         //
         // In theory, reseting signal handlers could cause us to miss signal deliveries. In
-        // practice, this code should only be run during startup, when we're not waiting for any
+        // practice, this code should only be run suring 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
@@ -1624,6 +1632,10 @@ 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,6 +16,9 @@
 #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 {
@@ -380,7 +383,9 @@ void get_signals_with_handlers(sigset_t *set) {
     }
 }
 
-void signal_block() {
+void signal_block(bool force) {
+    if (!force && ignore_signal_block) return;
+
     ASSERT_IS_MAIN_THREAD();
     sigset_t chldset;
 
@@ -400,10 +405,14 @@ void signal_unblock_all() {
     sigprocmask(SIG_SETMASK, &iset, NULL);
 }
 
-void signal_unblock() {
+void signal_unblock(bool force) {
+    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();
@@ -411,12 +420,13 @@ void signal_unblock() {
     }
 
     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,14 +30,16 @@ void signal_handle(int sig, int do_handle);
 void signal_unblock_all();
 
 /// Block all signals.
-void signal_block();
+void signal_block(bool force = false);
 
 /// Unblock all signals.
-void signal_unblock();
+void signal_unblock(bool force = false);
 
 /// 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