summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authornome <Knut.Franke@gmx.de>2024-09-24 13:44:58 +0200
committerGitHub <noreply@github.com>2024-09-24 06:44:58 -0500
commita948ec6c2cd2d2486589e73e701bd2c0a91a7547 (patch)
tree2b0d39611951be8d704ee938b59280fb993e2995
parent28a7461057456555314e65e9b10cb5b996d4f72d (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.rs69
-rw-r--r--crates/nu-protocol/src/process/mod.rs2
-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.rs62
-rw-r--r--crates/nu-system/src/lib.rs2
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};