extern/nushell
1
0
Fork 1
mirror of https://github.com/nushell/nushell.git synced 2024-11-25 01:43:47 +01:00
Code Issues Packages Projects Releases Wiki Activity

Fix handling of stopped TUI applications on unix (#13741)

Browse Source 
# 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.
-->
This commit is contained in:
nome 2024-09-24 13:44:58 +02:00 committed by GitHub
parent 28a7461057
commit a948ec6c2c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 118 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

69
crates/nu-protocol/src/process/child.rs
View File

@ -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> {

2
crates/nu-protocol/src/process/mod.rs
View File

@ -1,6 +1,4 @@
//! Handling of external subprocesses
mod child;
mod exit_status;
pub use child::*;
pub use exit_status::ExitStatus;

44
crates/nu-protocol/src/process/exit_status.rs → crates/nu-system/src/exit_status.rs
View File

@ -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)]

62
crates/nu-system/src/foreground.rs
View File

@ -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(())

2
crates/nu-system/src/lib.rs
View File

@ -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};