From a948ec6c2cd2d2486589e73e701bd2c0a91a7547 Mon Sep 17 00:00:00 2001 From: nome Date: Tue, 24 Sep 2024 13:44:58 +0200 Subject: [PATCH] Fix handling of stopped TUI applications on unix (#13741) # Description Instead of handling a foreground process being stopped in any way, we were simply ignoring SIGTSTP (which the terminal usually sends to the foreground process group when Ctrl-Z is pressed), and propagating this to our foreground children. This works for most processes, but it generally fails for applications which put the terminal in raw mode[1] and implement their own suspension mechanism (typically TUI apps like helix[2], neovim[3], top[4] or amp[5]). In these cases, triggering suspension within the app results in the terminal getting blocked, since the application is waiting for a SIGCONT, while nushell is waiting for it to exit. Fix this by unblocking SIGTSTP for our foreground children (neovim, helix and probably others send this to themselves while trying to suspend), and displaying the following message whenever one of them gets stopped: nushell currently does not support background jobs press any key to continue Pressing any key will then send SIGCONT to the child's process group and resume waiting. This fix is probably going to be superseded by a proper background job implementation (#247) at some point, but for now it's better than completely blocking the terminal. [1] https://docs.rs/crossterm/latest/crossterm/terminal/index.html#raw-mode [2] https://helix-editor.com/ [3] https://neovim.io/ [4] https://man7.org/linux/man-pages/man1/top.1.html [5] https://amp.rs/ - fixes #1038 - fixes #7335 - fixes #10335 # User-Facing Changes While any foreground process is running, Ctrl-Z is no longer ignored. Instead, the message described above is displayed, and nushell waits for a key to be pressed. # Tests + Formatting # After Submitting --- crates/nu-protocol/src/process/child.rs | 69 +++++++++++++++---- crates/nu-protocol/src/process/mod.rs | 2 - .../process => nu-system/src}/exit_status.rs | 44 ------------ crates/nu-system/src/foreground.rs | 62 ++++++++++++++++- crates/nu-system/src/lib.rs | 2 + 5 files changed, 118 insertions(+), 61 deletions(-) rename crates/{nu-protocol/src/process => nu-system/src}/exit_status.rs (54%) diff --git a/crates/nu-protocol/src/process/child.rs b/crates/nu-protocol/src/process/child.rs index 9ab92b331c..930015742b 100644 --- a/crates/nu-protocol/src/process/child.rs +++ b/crates/nu-protocol/src/process/child.rs @@ -1,7 +1,5 @@ -use crate::{ - byte_stream::convert_file, process::ExitStatus, ErrSpan, IntoSpanned, ShellError, Span, -}; -use nu_system::ForegroundChild; +use crate::{byte_stream::convert_file, ErrSpan, IntoSpanned, ShellError, Span}; +use nu_system::{ExitStatus, ForegroundChild}; use os_pipe::PipeReader; use std::{ fmt::Debug, @@ -10,6 +8,49 @@ use std::{ thread, }; +fn check_ok(status: ExitStatus, ignore_error: bool, span: Span) -> Result<(), ShellError> { + match status { + ExitStatus::Exited(exit_code) => { + if ignore_error { + Ok(()) + } else if let Ok(exit_code) = exit_code.try_into() { + Err(ShellError::NonZeroExitCode { exit_code, span }) + } else { + Ok(()) + } + } + #[cfg(unix)] + ExitStatus::Signaled { + signal, + core_dumped, + } => { + use nix::sys::signal::Signal; + + let sig = Signal::try_from(signal); + + if sig == Ok(Signal::SIGPIPE) || (ignore_error && !core_dumped) { + // Processes often exit with SIGPIPE, but this is not an error condition. + Ok(()) + } else { + let signal_name = sig.map(Signal::as_str).unwrap_or("unknown signal").into(); + Err(if core_dumped { + ShellError::CoreDumped { + signal_name, + signal, + span, + } + } else { + ShellError::TerminatedBySignal { + signal_name, + signal, + span, + } + }) + } + } + } +} + #[derive(Debug)] enum ExitStatusFuture { Finished(Result>), @@ -29,7 +70,7 @@ impl ExitStatusFuture { core_dumped: true, .. }, )) => { - status.check_ok(false, span)?; + check_ok(status, false, span)?; Ok(status) } Ok(Ok(status)) => Ok(status), @@ -138,7 +179,7 @@ impl ChildProcess { thread::Builder::new() .name("exit status waiter".into()) - .spawn(move || exit_status_sender.send(child.wait().map(Into::into))) + .spawn(move || exit_status_sender.send(child.wait())) .err_span(span)?; Ok(Self::from_raw(stdout, stderr, Some(exit_status), span)) @@ -185,9 +226,11 @@ impl ChildProcess { Vec::new() }; - self.exit_status - .wait(self.span)? - .check_ok(self.ignore_error, self.span)?; + check_ok( + self.exit_status.wait(self.span)?, + self.ignore_error, + self.span, + )?; Ok(bytes) } @@ -228,9 +271,11 @@ impl ChildProcess { consume_pipe(stderr).err_span(self.span)?; } - self.exit_status - .wait(self.span)? - .check_ok(self.ignore_error, self.span) + check_ok( + self.exit_status.wait(self.span)?, + self.ignore_error, + self.span, + ) } pub fn try_wait(&mut self) -> Result, ShellError> { diff --git a/crates/nu-protocol/src/process/mod.rs b/crates/nu-protocol/src/process/mod.rs index 6e10fdd620..f8a739b41b 100644 --- a/crates/nu-protocol/src/process/mod.rs +++ b/crates/nu-protocol/src/process/mod.rs @@ -1,6 +1,4 @@ //! Handling of external subprocesses mod child; -mod exit_status; pub use child::*; -pub use exit_status::ExitStatus; diff --git a/crates/nu-protocol/src/process/exit_status.rs b/crates/nu-system/src/exit_status.rs similarity index 54% rename from crates/nu-protocol/src/process/exit_status.rs rename to crates/nu-system/src/exit_status.rs index 8f37902aae..8f3794c44f 100644 --- a/crates/nu-protocol/src/process/exit_status.rs +++ b/crates/nu-system/src/exit_status.rs @@ -1,4 +1,3 @@ -use crate::{ShellError, Span}; use std::process; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -19,49 +18,6 @@ impl ExitStatus { ExitStatus::Signaled { signal, .. } => -signal, } } - - pub fn check_ok(self, ignore_error: bool, span: Span) -> Result<(), ShellError> { - match self { - ExitStatus::Exited(exit_code) => { - if ignore_error { - Ok(()) - } else if let Ok(exit_code) = exit_code.try_into() { - Err(ShellError::NonZeroExitCode { exit_code, span }) - } else { - Ok(()) - } - } - #[cfg(unix)] - ExitStatus::Signaled { - signal, - core_dumped, - } => { - use nix::sys::signal::Signal; - - let sig = Signal::try_from(signal); - - if sig == Ok(Signal::SIGPIPE) || (ignore_error && !core_dumped) { - // Processes often exit with SIGPIPE, but this is not an error condition. - Ok(()) - } else { - let signal_name = sig.map(Signal::as_str).unwrap_or("unknown signal").into(); - Err(if core_dumped { - ShellError::CoreDumped { - signal_name, - signal, - span, - } - } else { - ShellError::TerminatedBySignal { - signal_name, - signal, - span, - } - }) - } - } - } - } } #[cfg(unix)] diff --git a/crates/nu-system/src/foreground.rs b/crates/nu-system/src/foreground.rs index 93f7dd35c0..95b058c0e2 100644 --- a/crates/nu-system/src/foreground.rs +++ b/crates/nu-system/src/foreground.rs @@ -1,15 +1,22 @@ +#[cfg(unix)] +use std::io::prelude::*; use std::{ io, - process::{Child, Command, ExitStatus}, + process::{Child, Command}, sync::{atomic::AtomicU32, Arc}, }; +use crate::ExitStatus; + #[cfg(unix)] use std::{io::IsTerminal, sync::atomic::Ordering}; #[cfg(unix)] pub use foreground_pgroup::stdin_fd; +#[cfg(unix)] +use nix::{sys::signal, sys::wait, unistd::Pid}; + /// A simple wrapper for [`std::process::Child`] /// /// It can only be created by [`ForegroundChild::spawn`]. @@ -73,7 +80,52 @@ impl ForegroundChild { } pub fn wait(&mut self) -> io::Result { - self.as_mut().wait() + #[cfg(unix)] + { + // the child may be stopped multiple times, we loop until it exits + loop { + let child_pid = Pid::from_raw(self.inner.id() as i32); + let status = wait::waitpid(child_pid, Some(wait::WaitPidFlag::WUNTRACED)); + match status { + Err(e) => { + drop(self.inner.stdin.take()); + return Err(e.into()); + } + Ok(wait::WaitStatus::Exited(_, status)) => { + drop(self.inner.stdin.take()); + return Ok(ExitStatus::Exited(status)); + } + Ok(wait::WaitStatus::Signaled(_, signal, core_dumped)) => { + drop(self.inner.stdin.take()); + return Ok(ExitStatus::Signaled { + signal: signal as i32, + core_dumped, + }); + } + Ok(wait::WaitStatus::Stopped(_, _)) => { + println!("nushell currently does not support background jobs"); + // acquire terminal in order to be able to read from stdin + foreground_pgroup::reset(); + let mut stdin = io::stdin(); + let mut stdout = io::stdout(); + write!(stdout, "press any key to continue")?; + stdout.flush()?; + stdin.read_exact(&mut [0u8])?; + // bring child's pg back into foreground and continue it + if let Some(state) = self.pipeline_state.as_ref() { + let existing_pgrp = state.0.load(Ordering::SeqCst); + foreground_pgroup::set(&self.inner, existing_pgrp); + } + signal::killpg(child_pid, signal::SIGCONT)?; + } + Ok(_) => { + // keep waiting + } + }; + } + } + #[cfg(not(unix))] + self.as_mut().wait().map(Into::into) } } @@ -275,9 +327,13 @@ mod foreground_pgroup { // SIGINT has special handling let _ = sigaction(Signal::SIGQUIT, &default); // We don't support background jobs, so keep some signals blocked for now - // let _ = sigaction(Signal::SIGTSTP, &default); // let _ = sigaction(Signal::SIGTTIN, &default); // let _ = sigaction(Signal::SIGTTOU, &default); + // We do need to reset SIGTSTP though, since some TUI + // applications implement their own Ctrl-Z handling, and + // ForegroundChild::wait() needs to be able to react to the + // child being stopped. + let _ = sigaction(Signal::SIGTSTP, &default); let _ = sigaction(Signal::SIGTERM, &default); Ok(()) diff --git a/crates/nu-system/src/lib.rs b/crates/nu-system/src/lib.rs index 3633d62990..d6c10a697f 100644 --- a/crates/nu-system/src/lib.rs +++ b/crates/nu-system/src/lib.rs @@ -1,4 +1,5 @@ #![doc = include_str!("../README.md")] +mod exit_status; mod foreground; #[cfg(target_os = "freebsd")] @@ -13,6 +14,7 @@ pub mod os_info; #[cfg(target_os = "windows")] mod windows; +pub use self::exit_status::ExitStatus; #[cfg(unix)] pub use self::foreground::stdin_fd; pub use self::foreground::{ForegroundChild, ForegroundGuard};