From fd6d814ea4000d50414c4cf3f7f7ce5497acea78 Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Wed, 28 Dec 2016 18:52:33 -0800 Subject: [PATCH] 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 | 24 ++++++--- src/common.h | 3 ++ src/exec.cpp | 5 -- src/fish.cpp | 5 ++ src/postfork.cpp | 6 ++- src/proc.cpp | 19 +++++-- src/signal.cpp | 135 +++++++++++++++++++++++------------------------ src/signal.h | 1 + 8 files changed, 113 insertions(+), 85 deletions(-) diff --git a/src/common.cpp b/src/common.cpp index b194e1cff..6c01b2433 100644 --- a/src/common.cpp +++ b/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 diff --git a/src/common.h b/src/common.h index f0ec546df..f2f21fa81 100644 --- a/src/common.h +++ b/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() diff --git a/src/exec.cpp b/src/exec.cpp index 26f5d9165..50b09d558 100644 --- a/src/exec.cpp +++ b/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 diff --git a/src/fish.cpp b/src/fish.cpp index d3390f568..8223a9e3e 100644 --- a/src/fish.cpp +++ b/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); diff --git a/src/postfork.cpp b/src/postfork.cpp index 06bb81d02..f18ba1d7f 100644 --- a/src/postfork.cpp +++ b/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) { diff --git a/src/proc.cpp b/src/proc.cpp index 2d445c5eb..082c70af7 100644 --- a/src/proc.cpp +++ b/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; } diff --git a/src/signal.cpp b/src/signal.cpp index 9d48d1121..64a7c80af 100644 --- a/src/signal.cpp +++ b/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(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 2800caf64..3c4da3990 100644 --- a/src/signal.h +++ b/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