remove unnecessary signal management

The shell was doing a log of signal blocking/unblocking that hurts
performance and can be avoided. This reduced the elapsed time for a
simple benchmark by 25%.

Partial fix for #2007

src/common.cpp

@@ -500,15 +500,23 @@ __sentinel bool contains_internal(const wcstring &needle, int vararg_handle, ...
 }
 
 long read_blocked(int fd, void *buf, size_t count) {
-    ssize_t res;
-    sigset_t chldset, oldset;
+    long bytes_read = 0;
 
-    sigemptyset(&chldset);
-    sigaddset(&chldset, SIGCHLD);
-    VOMIT_ON_FAILURE(pthread_sigmask(SIG_BLOCK, &chldset, &oldset));
-    res = read(fd, buf, count);
-    VOMIT_ON_FAILURE(pthread_sigmask(SIG_SETMASK, &oldset, NULL));
-    return res;
+    while (count) {
+        ssize_t res = read(fd, (char *)buf + bytes_read, count);
+        if (res == 0) {
+            break;
+        } else if (res == -1) {
+            if (errno == EINTR) continue;
+            if (errno == EAGAIN) return bytes_read ? bytes_read : -1;
+            return -1;
+        } else {
+            bytes_read += res;
+            count -= res;
+        }
+    }
+
+    return bytes_read;
 }
 
 /// Loop a write request while failure is non-critical. Return -1 and set errno in case of critical

src/common.h

@@ -260,6 +260,8 @@ extern bool has_working_tty_timestamps;
 
 /// Check if signals are blocked. If so, print an error message and return from the function
 /// performing this check.
+#define CHECK_BLOCK(retval)
+#if 0
 #define CHECK_BLOCK(retval)                                                \
     if (signal_is_blocked()) {                                             \
         debug(0, "function %s called while blocking signals. ", __func__); \
@@ -267,6 +269,7 @@ extern bool has_working_tty_timestamps;
         show_stackframe(L'E');                                             \
         return retval;                                                     \
     }
+#endif
 
 /// Shorthand for wgettext call in situations where a C-style string is needed (e.g., fwprintf()).
 #define _(wstr) wgettext(wstr).c_str()

src/exec.cpp

@@ -367,12 +367,10 @@ static bool can_use_posix_spawn_for_job(const job_t *job, const process_t *proce
 
 void exec_job(parser_t &parser, job_t *j) {
     pid_t pid = 0;
-    sigset_t chldset;
 
     // Set to true if something goes wrong while exec:ing the job, in which case the cleanup code
     // will kick in.
     bool exec_error = false;
-
     bool needs_keepalive = false;
     process_t keepalive;
 
@@ -384,9 +382,6 @@ void exec_job(parser_t &parser, job_t *j) {
         return;
     }
 
-    sigemptyset(&chldset);
-    sigaddset(&chldset, SIGCHLD);
-
     debug(4, L"Exec job '%ls' with id %d", j->command_wcstr(), j->job_id);
 
     // Verify that all IO_BUFFERs are output. We used to support a (single, hacked-in) magical input

src/fish.cpp

@@ -392,6 +392,11 @@ 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()) ignore_signal_block = true;
+
         // Stomp the exit status of any initialization commands (issue #635).
         proc_set_last_status(STATUS_BUILTIN_OK);
 

src/postfork.cpp

@@ -105,7 +105,11 @@ bool set_child_group(job_t *j, process_t *p, int print_errors) {
     }
 
     if (job_get_flag(j, JOB_TERMINAL) && job_get_flag(j, JOB_FOREGROUND)) {  //!OCLINT(early exit)
-        int result = tcsetpgrp(STDIN_FILENO, j->pgid);  // to avoid "collapsible if statements" warn
+        int result = -1;
+        errno = EINTR;
+        while (result == -1 && errno == EINTR) {
+            result = tcsetpgrp(STDIN_FILENO, j->pgid);
+        }
         if (result == -1) {
             if (errno == ENOTTY) redirect_tty_output();
             if (print_errors) {

src/proc.cpp

@@ -792,19 +792,32 @@ static void read_try(job_t *j) {
 /// \param cont If this variable is set, we are giving back control to a job that has previously
 /// been stopped. In that case, we need to set the terminal attributes to those saved in the job.
 static bool terminal_give_to_job(job_t *j, int cont) {
-    if (tcsetpgrp(STDIN_FILENO, j->pgid) == -1) {
+    int result = -1;
+    errno = EINTR;
+    while (result == -1 && errno == EINTR) {
+        result = tcsetpgrp(STDIN_FILENO, j->pgid);
+    }
+    if (result == -1) {
         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");
         return false;
     }
 
-    if (cont && tcsetattr(STDIN_FILENO, TCSADRAIN, &j->tmodes)) {
+    if (cont) {
+    int result = -1;
+    errno = EINTR;
+    while (result == -1 && errno == EINTR) {
+        result = tcsetattr(STDIN_FILENO, TCSADRAIN, &j->tmodes);
+    }
+    if (result == -1) {
         if (errno == ENOTTY) redirect_tty_output();
-        debug(1, _(L"Could not send job %d ('%ls') to foreground"), j->job_id, j->command_wcstr());
+        debug(1, _(L"terminal_give_to_job(): Could not send job %d ('%ls') to foreground"), j->job_id, j->command_wcstr());
         wperror(L"tcsetattr");
         return false;
     }
+    }
+
     return true;
 }
 

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 = false;
+
 /// Struct describing an entry for the lookup table used to convert between signal names and signal
 /// ids, etc.
 struct lookup_entry {
@@ -252,88 +255,77 @@ void signal_reset_handlers() {
     }
 }
 
-/// Sets appropriate signal handlers.
-void signal_set_handlers() {
+static void set_interactive_handlers() {
     struct sigaction act;
-
     sigemptyset(&act.sa_mask);
-    act.sa_flags = SA_SIGINFO;
-    act.sa_sigaction = &default_handler;
 
-    // First reset everything to a use default_handler, a function whose sole action is to fire of
-    // an event.
+    // Interactive mode. Ignore interactive signals.  We are a shell, we know what is best for
+    // the user.
+    act.sa_handler = SIG_IGN;
     sigaction(SIGINT, &act, 0);
     sigaction(SIGQUIT, &act, 0);
     sigaction(SIGTSTP, &act, 0);
-    sigaction(SIGTTIN, &act, 0);
     sigaction(SIGTTOU, &act, 0);
-    sigaction(SIGCHLD, &act, 0);
 
-    // Ignore sigpipe, which we may get from the universal variable notifier.
-    sigaction(SIGPIPE, &act, 0);
+    // We don't ignore SIGTTIN because we might send it to ourself.
+    act.sa_sigaction = &default_handler;
+    act.sa_flags = SA_SIGINFO;
+    sigaction(SIGTTIN, &act, 0);
 
-    if (shell_is_interactive()) {
-        // Interactive mode. Ignore interactive signals.  We are a shell, we know what is best for
-        // the user.
-        act.sa_handler = SIG_IGN;
+    act.sa_sigaction = &handle_int;
+    act.sa_flags = SA_SIGINFO;
+    sigaction(SIGINT, &act, 0);
 
-        sigaction(SIGINT, &act, 0);
-        sigaction(SIGQUIT, &act, 0);
-        sigaction(SIGTSTP, &act, 0);
-        sigaction(SIGTTIN, &act, 0);
-        sigaction(SIGTTOU, &act, 0);
+    // SIGTERM restores the terminal controlling process before dying.
+    act.sa_sigaction = &handle_term;
+    act.sa_flags = SA_SIGINFO;
+    sigaction(SIGTERM, &act, 0);
 
-        act.sa_sigaction = &handle_int;
-        act.sa_flags = SA_SIGINFO;
-        if (sigaction(SIGINT, &act, 0)) {
-            wperror(L"sigaction");
-            FATAL_EXIT();
-        }
-
-        act.sa_sigaction = &handle_chld;
-        act.sa_flags = SA_SIGINFO;
-        if (sigaction(SIGCHLD, &act, 0)) {
-            wperror(L"sigaction");
-            FATAL_EXIT();
-        }
+    act.sa_sigaction = &handle_hup;
+    act.sa_flags = SA_SIGINFO;
+    sigaction(SIGHUP, &act, 0);
 
 #ifdef SIGWINCH
-        act.sa_flags = SA_SIGINFO;
-        act.sa_sigaction = &handle_winch;
-        if (sigaction(SIGWINCH, &act, 0)) {
-            wperror(L"sigaction");
-            FATAL_EXIT();
-        }
+    act.sa_sigaction = &handle_winch;
+    act.sa_flags = SA_SIGINFO;
+    sigaction(SIGWINCH, &act, 0);
 #endif
+}
 
-        act.sa_flags = SA_SIGINFO;
-        act.sa_sigaction = &handle_hup;
-        if (sigaction(SIGHUP, &act, 0)) {
-            wperror(L"sigaction");
-            FATAL_EXIT();
-        }
+static void set_non_interactive_handlers() {
+    struct sigaction act;
+    sigemptyset(&act.sa_mask);
 
-        // SIGTERM restores the terminal controlling process before dying.
-        act.sa_flags = SA_SIGINFO;
-        act.sa_sigaction = &handle_term;
-        if (sigaction(SIGTERM, &act, 0)) {
-            wperror(L"sigaction");
-            FATAL_EXIT();
-        }
+    // Non-interactive. Ignore interrupt, check exit status of processes to determine result
+    // instead.
+    act.sa_handler = SIG_IGN;
+    sigaction(SIGINT, &act, 0);
+    sigaction(SIGQUIT, &act, 0);
+}
+
+/// Sets up appropriate signal handlers.
+void signal_set_handlers() {
+    struct sigaction act;
+    sigemptyset(&act.sa_mask);
+
+    // Ignore SIGPIPE. We'll detect failed writes and deal with them appropriately. We don't want
+    // this signal interrupting other syscalls or terminating us.
+    act.sa_handler = SIG_IGN;
+    sigaction(SIGPIPE, &act, 0);
+
+    // Whether or not we're interactive we want SIGCHLD to not interrupt restartable syscalls.
+    act.sa_flags = SA_SIGINFO;
+    act.sa_sigaction = &handle_chld;
+    act.sa_flags = SA_SIGINFO | SA_RESTART;
+    if (sigaction(SIGCHLD, &act, 0)) {
+        wperror(L"sigaction");
+        FATAL_EXIT();
+    }
+
+    if (shell_is_interactive()) {
+        set_interactive_handlers();
     } else {
-        // Non-interactive. Ignore interrupt, check exit status of processes to determine result
-        // instead.
-        act.sa_handler = SIG_IGN;
-        sigaction(SIGINT, &act, 0);
-        sigaction(SIGQUIT, &act, 0);
-
-        act.sa_handler = SIG_DFL;
-        act.sa_sigaction = &handle_chld;
-        act.sa_flags = SA_SIGINFO;
-        if (sigaction(SIGCHLD, &act, 0)) {
-            wperror(L"sigaction");
-            exit_without_destructors(1);
-        }
+        set_non_interactive_handlers();
     }
 }
 
@@ -367,6 +359,8 @@ void get_signals_with_handlers(sigset_t *set) {
 }
 
 void signal_block() {
+    if (ignore_signal_block) return;
+
     ASSERT_IS_MAIN_THREAD();
     sigset_t chldset;
 
@@ -376,10 +370,12 @@ void signal_block() {
     }
 
     block_count++;
-    //	debug( 0, L"signal block level increased to %d", block_count );
+    // debug( 0, L"signal block level increased to %d", block_count );
 }
 
 void signal_unblock() {
+    if (ignore_signal_block) return;
+
     ASSERT_IS_MAIN_THREAD();
     sigset_t chldset;
 
@@ -395,7 +391,10 @@ void signal_unblock() {
         sigfillset(&chldset);
         VOMIT_ON_FAILURE(pthread_sigmask(SIG_UNBLOCK, &chldset, 0));
     }
-    //	debug( 0, L"signal block level decreased to %d", block_count );
+    // debug( 0, L"signal block level decreased to %d", block_count );
 }
 
-bool signal_is_blocked() { return static_cast<bool>(block_count); }
+bool signal_is_blocked() {
+    if (ignore_signal_block) return false;
+    return static_cast<bool>(block_count);
+}

src/signal.h

@@ -38,4 +38,5 @@ bool signal_is_blocked();
 /// Returns signals with non-default handlers.
 void get_signals_with_handlers(sigset_t *set);
 
+extern bool ignore_signal_block;
 #endif