diff options
author | nome <Knut.Franke@gmx.de> | 2024-09-24 13:44:58 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-09-24 06:44:58 -0500 |
commit | a948ec6c2cd2d2486589e73e701bd2c0a91a7547 (patch) | |
tree | 2b0d39611951be8d704ee938b59280fb993e2995 | |
parent | 28a7461057456555314e65e9b10cb5b996d4f72d (diff) |
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
<!--
Don't forget to add tests that cover your changes.
Make sure you've run and fixed any issues with these commands:
- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the
tests for the standard library
> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->
# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
-rw-r--r-- | crates/nu-protocol/src/process/child.rs | 69 | ||||
-rw-r--r-- | crates/nu-protocol/src/process/mod.rs | 2 | ||||
-rw-r--r-- | crates/nu-system/src/exit_status.rs (renamed from crates/nu-protocol/src/process/exit_status.rs) | 44 | ||||
-rw-r--r-- | crates/nu-system/src/foreground.rs | 62 | ||||
-rw-r--r-- | crates/nu-system/src/lib.rs | 2 |
5 files changed, 118 insertions, 61 deletions
diff --git a/crates/nu-protocol/src/process/child.rs b/crates/nu-protocol/src/process/child.rs index 9ab92b331..930015742 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<ExitStatus, Box<ShellError>>), @@ -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<Option<ExitStatus>, ShellError> { diff --git a/crates/nu-protocol/src/process/mod.rs b/crates/nu-protocol/src/process/mod.rs index 6e10fdd62..f8a739b41 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 index 8f37902aa..8f3794c44 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 93f7dd35c..95b058c0e 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<ExitStatus> { - 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 3633d6299..d6c10a697 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}; |