From d25df9c00b5b34827fd7b286534a6bebc4b39512 Mon Sep 17 00:00:00 2001 From: JT <547158+jntrnr@users.noreply.github.com> Date: Wed, 2 Aug 2023 11:24:28 +1200 Subject: [PATCH] Revert 9693 to prevent CPU hangs (#9893) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description This reverts #9693 as it lead to CPU hangs. (btw, did the revert by hand as it couldn't be done automatically. Hopefully I didn't miss anything 😅 ) Fixes #9859 cc @IanManske # User-Facing Changes # Tests + Formatting # After Submitting --- crates/nu-command/src/system/run_external.rs | 9 ++- crates/nu-system/src/foreground.rs | 36 ++++-------- src/main.rs | 8 +-- src/terminal.rs | 62 +++++++++----------- 4 files changed, 47 insertions(+), 68 deletions(-) diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index e675ad86da..46e9ab5b10 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -213,7 +213,7 @@ impl ExternalCommand { // fails to be run as a normal executable: // 1. "shell out" to cmd.exe if the command is a known cmd.exe internal command // 2. Otherwise, use `which-rs` to look for batch files etc. then run those in cmd.exe - match fg_process.spawn(engine_state.is_interactive) { + match fg_process.spawn() { Err(err) => { // set the default value, maybe we'll override it later child = Err(err); @@ -235,7 +235,7 @@ impl ExternalCommand { cmd, engine_state.pipeline_externals_state.clone(), ); - child = cmd_process.spawn(engine_state.is_interactive); + child = cmd_process.spawn(); } else { #[cfg(feature = "which-support")] { @@ -269,8 +269,7 @@ impl ExternalCommand { cmd, engine_state.pipeline_externals_state.clone(), ); - child = - cmd_process.spawn(engine_state.is_interactive); + child = cmd_process.spawn(); } } } @@ -287,7 +286,7 @@ impl ExternalCommand { #[cfg(not(windows))] { - child = fg_process.spawn(engine_state.is_interactive) + child = fg_process.spawn() } match child { diff --git a/crates/nu-system/src/foreground.rs b/crates/nu-system/src/foreground.rs index 02dd72b66e..af2265b981 100644 --- a/crates/nu-system/src/foreground.rs +++ b/crates/nu-system/src/foreground.rs @@ -28,7 +28,6 @@ pub struct ForegroundProcess { pub struct ForegroundChild { inner: Child, pipeline_state: Arc<(AtomicU32, AtomicU32)>, - interactive: bool, } impl ForegroundProcess { @@ -39,14 +38,14 @@ impl ForegroundProcess { } } - pub fn spawn(&mut self, interactive: bool) -> std::io::Result { + pub fn spawn(&mut self) -> std::io::Result { let (ref pgrp, ref pcnt) = *self.pipeline_state; let existing_pgrp = pgrp.load(Ordering::SeqCst); - fg_process_setup::prepare_to_foreground(&mut self.inner, existing_pgrp, interactive); + fg_process_setup::prepare_to_foreground(&mut self.inner, existing_pgrp); self.inner .spawn() .map(|child| { - fg_process_setup::set_foreground(&child, existing_pgrp, interactive); + fg_process_setup::set_foreground(&child, existing_pgrp); let _ = pcnt.fetch_add(1, Ordering::SeqCst); if existing_pgrp == 0 { pgrp.store(child.id(), Ordering::SeqCst); @@ -54,11 +53,10 @@ impl ForegroundProcess { ForegroundChild { inner: child, pipeline_state: self.pipeline_state.clone(), - interactive, } }) .map_err(|e| { - fg_process_setup::reset_foreground_id(interactive); + fg_process_setup::reset_foreground_id(); e }) } @@ -75,7 +73,7 @@ impl Drop for ForegroundChild { let (ref pgrp, ref pcnt) = *self.pipeline_state; if pcnt.fetch_sub(1, Ordering::SeqCst) == 1 { pgrp.store(0, Ordering::SeqCst); - fg_process_setup::reset_foreground_id(self.interactive) + fg_process_setup::reset_foreground_id() } } } @@ -103,10 +101,8 @@ mod fg_process_setup { pub(super) fn prepare_to_foreground( external_command: &mut std::process::Command, existing_pgrp: u32, - interactive: bool, ) { let tty = TtyHandle(unistd::dup(nix::libc::STDIN_FILENO).expect("dup")); - let interactive = interactive && std::io::stdin().is_terminal(); unsafe { // Safety: // POSIX only allows async-signal-safe functions to be called. @@ -128,9 +124,7 @@ mod fg_process_setup { // According to glibc's job control manual: // https://www.gnu.org/software/libc/manual/html_node/Launching-Jobs.html // This has to be done *both* in the parent and here in the child due to race conditions. - if interactive { - set_foreground_pid(unistd::getpid(), existing_pgrp, tty.0); - } + set_foreground_pid(unistd::getpid(), existing_pgrp, tty.0); // Now let the child process have all the signals by resetting with SIG_SETMASK. let mut sigset = signal::SigSet::empty(); @@ -143,13 +137,9 @@ mod fg_process_setup { } } - pub(super) fn set_foreground( - process: &std::process::Child, - existing_pgrp: u32, - interactive: bool, - ) { + pub(super) fn set_foreground(process: &std::process::Child, existing_pgrp: u32) { // called from the parent shell process - do the stdin tty check here - if interactive && std::io::stdin().is_terminal() { + if std::io::stdin().is_terminal() { set_foreground_pid( Pid::from_raw(process.id() as i32), existing_pgrp, @@ -173,8 +163,8 @@ mod fg_process_setup { } /// Reset the foreground process group to the shell - pub(super) fn reset_foreground_id(interactive: bool) { - if interactive && std::io::stdin().is_terminal() { + pub(super) fn reset_foreground_id() { + if std::io::stdin().is_terminal() { if let Err(e) = nix::unistd::tcsetpgrp(nix::libc::STDIN_FILENO, unistd::getpgrp()) { println!("ERROR: reset foreground id failed, tcsetpgrp result: {e:?}"); } @@ -184,9 +174,9 @@ mod fg_process_setup { #[cfg(any(not(target_family = "unix"), target_os = "macos"))] mod fg_process_setup { - pub(super) fn prepare_to_foreground(_: &mut std::process::Command, _: u32, _: bool) {} + pub(super) fn prepare_to_foreground(_: &mut std::process::Command, _: u32) {} - pub(super) fn set_foreground(_: &std::process::Child, _: u32, _: bool) {} + pub(super) fn set_foreground(_: &std::process::Child, _: u32) {} - pub(super) fn reset_foreground_id(_: bool) {} + pub(super) fn reset_foreground_id() {} } diff --git a/src/main.rs b/src/main.rs index 1efa9ac295..e52b7184f0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -82,10 +82,7 @@ fn main() -> Result<()> { .unwrap_or_else(|_| std::process::exit(1)); // keep this condition in sync with the branches at the end - engine_state.is_interactive = parsed_nu_cli_args.interactive_shell.is_some() - || (parsed_nu_cli_args.testbin.is_none() - && parsed_nu_cli_args.commands.is_none() - && script_name.is_empty()); + engine_state.is_interactive = parsed_nu_cli_args.interactive_shell.is_some(); engine_state.is_login = parsed_nu_cli_args.login_shell.is_some(); @@ -147,7 +144,7 @@ fn main() -> Result<()> { ); start_time = std::time::Instant::now(); - acquire_terminal(engine_state.is_interactive); + acquire_terminal(parsed_nu_cli_args.commands.is_none() && script_name.is_empty()); perf( "acquire_terminal", start_time, @@ -293,6 +290,7 @@ fn main() -> Result<()> { input, ) } else { + engine_state.is_interactive = true; run_repl(&mut engine_state, parsed_nu_cli_args, entire_start_time) } } diff --git a/src/terminal.rs b/src/terminal.rs index e5eb573f1a..247cd1ea8f 100644 --- a/src/terminal.rs +++ b/src/terminal.rs @@ -1,38 +1,19 @@ #[cfg(unix)] pub(crate) fn acquire_terminal(interactive: bool) { use is_terminal::IsTerminal; - use nix::{ - errno::Errno, - sys::signal::{signal, SigHandler, Signal}, - unistd, - }; + use nix::sys::signal::{signal, SigHandler, Signal}; - if interactive && std::io::stdin().is_terminal() { - // see also: https://www.gnu.org/software/libc/manual/html_node/Initializing-the-Shell.html + if !std::io::stdin().is_terminal() { + return; + } - take_control(); + take_control(interactive); - unsafe { - // SIGINT and SIGQUIT have special handling above - signal(Signal::SIGTSTP, SigHandler::SigIgn).expect("signal ignore"); - signal(Signal::SIGTTIN, SigHandler::SigIgn).expect("signal ignore"); - signal(Signal::SIGTTOU, SigHandler::SigIgn).expect("signal ignore"); - } - - // Put ourselves in our own process group and take control of terminal, if not already - let shell_pgid = unistd::getpid(); - match unistd::setpgid(shell_pgid, shell_pgid) { - // setpgid returns EPERM if we are the session leader (e.g., as a login shell). - // The other cases that return EPERM cannot happen, since we gave our own pid. - // See: setpgid(2) - // Therefore, it is safe to ignore EPERM. - Ok(()) | Err(Errno::EPERM) => (), - Err(_) => { - eprintln!("ERROR: failed to put nushell in its own process group"); - std::process::exit(1); - } - } - let _ = unistd::tcsetpgrp(nix::libc::STDIN_FILENO, shell_pgid); + unsafe { + // SIGINT and SIGQUIT have special handling above + signal(Signal::SIGTSTP, SigHandler::SigIgn).expect("signal ignore"); + signal(Signal::SIGTTIN, SigHandler::SigIgn).expect("signal ignore"); + signal(Signal::SIGTTOU, SigHandler::SigIgn).expect("signal ignore"); } } @@ -41,7 +22,7 @@ pub(crate) fn acquire_terminal(_: bool) {} // Inspired by fish's acquire_tty_or_exit #[cfg(unix)] -fn take_control() { +fn take_control(interactive: bool) { use nix::{ errno::Errno, sys::signal::{self, SaFlags, SigAction, SigHandler, SigSet, Signal}, @@ -78,23 +59,32 @@ fn take_control() { } } + let mut success = false; for _ in 0..4096 { match unistd::tcgetpgrp(nix::libc::STDIN_FILENO) { Ok(owner_pgid) if owner_pgid == shell_pgid => { - // success - return; + success = true; + break; } Ok(owner_pgid) if owner_pgid == Pid::from_raw(0) => { // Zero basically means something like "not owned" and we can just take it let _ = unistd::tcsetpgrp(nix::libc::STDIN_FILENO, shell_pgid); } Err(Errno::ENOTTY) => { + if !interactive { + // that's fine + return; + } eprintln!("ERROR: no TTY for interactive shell"); std::process::exit(1); } _ => { // fish also has other heuristics than "too many attempts" for the orphan check, but they're optional - if signal::killpg(shell_pgid, Signal::SIGTTIN).is_err() { + if signal::killpg(Pid::from_raw(-shell_pgid.as_raw()), Signal::SIGTTIN).is_err() { + if !interactive { + // that's fine + return; + } eprintln!("ERROR: failed to SIGTTIN ourselves"); std::process::exit(1); } @@ -102,6 +92,8 @@ fn take_control() { } } - eprintln!("ERROR: failed take control of the terminal, we might be orphaned"); - std::process::exit(1); + if !success && interactive { + eprintln!("ERROR: failed take control of the terminal, we might be orphaned"); + std::process::exit(1); + } }