Revert 9693 to prevent CPU hangs (#9893)

# 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
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

# 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 -A
clippy::needless_collect -A clippy::result_large_err` to check that
you're using the standard code style
- `cargo test --workspace` to check that all tests pass
- `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` 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:
JT 2023-08-02 11:24:28 +12:00 committed by GitHub
parent 778a00efa1
commit d25df9c00b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 47 additions and 68 deletions

View file

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

View file

@ -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<ForegroundChild> {
pub fn spawn(&mut self) -> std::io::Result<ForegroundChild> {
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() {}
}

View file

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

View file

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