Try again: in unix like system, set foreground process while running external command (#6273)

* Revert "Fix intermittent test crash (#6268)"

This reverts commit 555d9ee763.

* make a working version again

* try second impl

* add

* fmt

* check stdin is atty before acquire stdin

* add libc

* clean comment

* fix typo

Cargo.lock

@@ -2440,6 +2440,7 @@ dependencies = [
  "bitflags",
  "cfg-if 1.0.0",
  "libc",
+ "memoffset",
 ]
 
 [[package]]
@@ -2792,10 +2793,12 @@ dependencies = [
 name = "nu-system"
 version = "0.67.1"
 dependencies = [
+ "atty",
  "chrono",
  "errno",
  "libc",
  "libproc",
+ "nix",
  "ntapi",
  "once_cell",
  "procfs",

crates/nu-command/src/system/run_external.rs

@@ -6,6 +6,7 @@ use nu_protocol::did_you_mean;
 use nu_protocol::engine::{EngineState, Stack};
 use nu_protocol::{ast::Call, engine::Command, ShellError, Signature, SyntaxShape, Value};
 use nu_protocol::{Category, Example, ListStream, PipelineData, RawStream, Span, Spanned};
+use nu_system::ForegroundProcess;
 use pathdiff::diff_paths;
 use std::collections::HashMap;
 use std::io::{BufRead, BufReader, Write};
@@ -121,7 +122,7 @@ impl ExternalCommand {
 
         let ctrlc = engine_state.ctrlc.clone();
 
-        let mut process = self.create_process(&input, false, head)?;
+        let mut fg_process = ForegroundProcess::new(self.create_process(&input, false, head)?);
         // mut is used in the windows branch only, suppress warning on other platforms
         #[allow(unused_mut)]
         let mut child;
@@ -136,8 +137,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 process.spawn() {
+            match fg_process.spawn() {
                 Err(err) => {
                     // set the default value, maybe we'll override it later
                     child = Err(err);
@@ -153,7 +153,8 @@ impl ExternalCommand {
                         .any(|&cmd| command_name_upper == cmd);
 
                     if looks_like_cmd_internal {
-                        let mut cmd_process = self.create_process(&input, true, head)?;
+                        let mut cmd_process =
+                            ForegroundProcess::new(self.create_process(&input, true, head)?);
                         child = cmd_process.spawn();
                     } else {
                         #[cfg(feature = "which-support")]
@@ -181,8 +182,10 @@ impl ExternalCommand {
                                                     item: file_name.to_string_lossy().to_string(),
                                                     span: self.name.span,
                                                 };
-                                                let mut cmd_process = new_command
-                                                    .create_process(&input, true, head)?;
+                                                let mut cmd_process = ForegroundProcess::new(
+                                                    new_command
+                                                        .create_process(&input, true, head)?,
+                                                );
                                                 child = cmd_process.spawn();
                                             }
                                         }
@@ -200,7 +203,7 @@ impl ExternalCommand {
 
         #[cfg(not(windows))]
         {
-            child = process.spawn()
+            child = fg_process.spawn()
         }
 
         match child {
@@ -252,7 +255,7 @@ impl ExternalCommand {
                     engine_state.config.use_ansi_coloring = false;
 
                     // if there is a string or a stream, that is sent to the pipe std
-                    if let Some(mut stdin_write) = child.stdin.take() {
+                    if let Some(mut stdin_write) = child.as_mut().stdin.take() {
                         std::thread::spawn(move || {
                             let input = crate::Table::run(
                                 &crate::Table,
@@ -293,7 +296,7 @@ impl ExternalCommand {
                     // and we create a ListStream that can be consumed
 
                     if redirect_stderr {
-                        let stderr = child.stderr.take().ok_or_else(|| {
+                        let stderr = child.as_mut().stderr.take().ok_or_else(|| {
                             ShellError::ExternalCommand(
                                 "Error taking stderr from external".to_string(),
                                 "Redirects need access to stderr of an external command"
@@ -332,7 +335,7 @@ impl ExternalCommand {
                     }
 
                     if redirect_stdout {
-                        let stdout = child.stdout.take().ok_or_else(|| {
+                        let stdout = child.as_mut().stdout.take().ok_or_else(|| {
                             ShellError::ExternalCommand(
                                 "Error taking stdout from external".to_string(),
                                 "Redirects need access to stdout of an external command"
@@ -370,7 +373,7 @@ impl ExternalCommand {
                         }
                     }
 
-                    match child.wait() {
+                    match child.as_mut().wait() {
                         Err(err) => Err(ShellError::ExternalCommand(
                             "External command exited with error".into(),
                             err.to_string(),

crates/nu-system/Cargo.toml

@@ -16,6 +16,10 @@ path = "src/main.rs"
 [dependencies]
 libc = "0.2"
 
+[target.'cfg(target_family = "unix")'.dependencies]
+nix = "0.24"
+atty = "0.2"
+
 [target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies]
 procfs  = "0.14.0"
 

crates/nu-system/src/foreground.rs

@@ -0,0 +1,117 @@
+use std::process::{Child, Command};
+
+/// A simple wrapper for `std::process::Command`
+///
+/// ## spawn behavior
+/// ### Unix
+/// When invoke `spawn`, current process will block `SIGTSTP`, `SIGTTOU`, `SIGTTIN`, `SIGCHLD`
+///
+/// spawned child process will get it's own process group id, and it's going to foreground(by making stdin belong's to child's process group).
+///
+/// When child is to over, unblock `SIGTSTP`, `SIGTTOU`, `SIGTTIN`, `SIGCHLD`, foreground process is back to callers' process.
+/// It bahaves something like `SignalHandler` in ion(https://gitlab.redox-os.org/redox-os/ion/-/tree/master/).
+///
+/// ### Windows
+/// It does nothing special on windows system, `spawn` is the same as [std::process::Command::spawn](std::process::Command::spawn)
+pub struct ForegroundProcess {
+    inner: Command,
+}
+
+/// A simple wrapper for `std::process::Child`
+///
+/// It can only be created by `ForegroundProcess::spawn`.
+pub struct ForegroundChild {
+    inner: Child,
+}
+
+impl ForegroundProcess {
+    pub fn new(cmd: Command) -> Self {
+        Self { inner: cmd }
+    }
+
+    pub fn spawn(&mut self) -> std::io::Result<ForegroundChild> {
+        fg_process_setup::prepare_to_foreground(&mut self.inner);
+        self.inner.spawn().map(|child| {
+            fg_process_setup::set_foreground(&child);
+            ForegroundChild { inner: child }
+        })
+    }
+}
+
+impl AsMut<Child> for ForegroundChild {
+    fn as_mut(&mut self) -> &mut Child {
+        &mut self.inner
+    }
+}
+
+impl Drop for ForegroundChild {
+    fn drop(&mut self) {
+        // It's ok to use here because we have called `set_foreground` during creation.
+        unsafe { fg_process_setup::reset_foreground_id() }
+    }
+}
+
+// It's a simpler version of fish shell's external process handling.
+#[cfg(target_family = "unix")]
+mod fg_process_setup {
+    use crate::signal::{block, unblock};
+    use nix::unistd::{self, Pid};
+    use std::os::unix::prelude::CommandExt;
+
+    pub(super) fn prepare_to_foreground(external_command: &mut std::process::Command) {
+        unsafe {
+            block();
+            // Safety:
+            // POSIX only allows async-signal-safe functions to be called.
+            // And `setpgid` is async-signal-safe function according to:
+            // https://manpages.ubuntu.com/manpages/bionic/man7/signal-safety.7.html
+            // So we're ok to invoke `libc::setpgid` inside `pre_exec`.
+            external_command.pre_exec(|| {
+                // make the command startup with new process group.
+                // The process group id must be the same as external commands' pid.
+                // Or else we'll failed to set it as foreground process.
+                // For more information, check `fork_child_for_process` function:
+                // https://github.com/fish-shell/fish-shell/blob/023042098396aa450d2c4ea1eb9341312de23126/src/exec.cpp#L398
+                if let Err(e) = unistd::setpgid(Pid::from_raw(0), Pid::from_raw(0)) {
+                    println!("ERROR: setpgid for external failed, result: {e:?}");
+                }
+                Ok(())
+            });
+        }
+    }
+
+    // If `prepare_to_foreground` function is not called, the function will fail with silence and do nothing.
+    pub(super) fn set_foreground(process: &std::process::Child) {
+        if atty::is(atty::Stream::Stdin) {
+            if let Err(e) =
+                nix::unistd::tcsetpgrp(nix::libc::STDIN_FILENO, Pid::from_raw(process.id() as i32))
+            {
+                println!("ERROR: set foreground id failed, tcsetpgrp result: {e:?}");
+            }
+        }
+    }
+
+    /// Reset foreground to current process, unblock `SIGTSTP`, `SIGTTOU`, `SIGTTIN`, `SIGCHLD`
+    ///
+    /// ## Safety
+    /// It can only be called when you have called `set_foreground`, or results in undefined behavior.
+    pub(super) unsafe fn reset_foreground_id() {
+        if atty::is(atty::Stream::Stdin) {
+            if let Err(e) = nix::unistd::tcsetpgrp(nix::libc::STDIN_FILENO, unistd::getpgrp()) {
+                println!("ERROR: reset foreground id failed, tcsetpgrp result: {e:?}");
+            }
+        }
+        unblock()
+    }
+}
+
+// TODO: investigate if we can set foreground process through windows system call.
+#[cfg(target_family = "windows")]
+mod fg_process_setup {
+
+    pub(super) fn prepare_to_foreground(_external_command: &mut std::process::Command) {}
+
+    pub(super) fn set_foreground(_process: &std::process::Child) {}
+
+    pub(super) unsafe fn reset_foreground_id() {}
+}

crates/nu-system/src/lib.rs

@@ -1,10 +1,15 @@
+mod foreground;
 #[cfg(any(target_os = "android", target_os = "linux"))]
 mod linux;
 #[cfg(target_os = "macos")]
 mod macos;
+#[cfg(target_family = "unix")]
+pub mod signal;
+
 #[cfg(target_os = "windows")]
 mod windows;
 
+pub use self::foreground::{ForegroundChild, ForegroundProcess};
 #[cfg(any(target_os = "android", target_os = "linux"))]
 pub use self::linux::*;
 #[cfg(target_os = "macos")]

crates/nu-system/src/signal.rs

@@ -0,0 +1,41 @@
+use nix::sys::signal::{self, SigHandler, Signal};
+
+/// Blocks the SIGTSTP/SIGTTOU/SIGTTIN/SIGCHLD signals so that the shell never receives
+/// them.
+pub fn block() {
+    let mut sigset = signal::SigSet::empty();
+    sigset.add(signal::Signal::SIGTSTP);
+    sigset.add(signal::Signal::SIGTTOU);
+    sigset.add(signal::Signal::SIGTTIN);
+    sigset.add(signal::Signal::SIGCHLD);
+    if let Err(e) = signal::sigprocmask(signal::SigmaskHow::SIG_BLOCK, Some(&sigset), None) {
+        println!("ERROR: Could not block the signals, error message: {e:?}");
+    }
+}
+
+/// Unblocks the SIGTSTP/SIGTTOU/SIGTTIN/SIGCHLD signals so children processes can be
+/// controlled
+/// by the shell.
+pub fn unblock() {
+    let mut sigset = signal::SigSet::empty();
+    sigset.add(signal::Signal::SIGTSTP);
+    sigset.add(signal::Signal::SIGTTOU);
+    sigset.add(signal::Signal::SIGTTIN);
+    sigset.add(signal::Signal::SIGCHLD);
+    if let Err(e) = signal::sigprocmask(signal::SigmaskHow::SIG_UNBLOCK, Some(&sigset), None) {
+        println!("ERROR: Could not unblock the signals, error message: {e:?}");
+    }
+}
+
+// It's referenced from `set_unique_pid` function in `ion`.
+pub fn set_terminal_leader() {
+    let stdin_is_a_tty = atty::is(atty::Stream::Stdin);
+    if stdin_is_a_tty {
+        // We have make sure that stdin is a tty, it's ok to ignore SIGTTOU.
+        unsafe {
+            if let Err(e) = signal::signal(Signal::SIGTTOU, SigHandler::SigIgn) {
+                println!("WARN: ignore SIGTTOU failed, error message: {e:?}");
+            }
+        }
+    }
+}

src/main.rs

@@ -134,6 +134,14 @@ fn main() -> Result<()> {
 
     let parsed_nu_cli_args = parse_commandline_args(&nushell_commandline_args, &mut engine_state);
 
+    #[cfg(target_family = "unix")]
+    {
+        // This will block SIGTSTP, SIGTTOU, SIGTTIN, and SIGCHLD, which is required
+        // for this shell to manage its own process group / children / etc.
+        nu_system::signal::block();
+        nu_system::signal::set_terminal_leader()
+    }
+
     match parsed_nu_cli_args {
         Ok(binary_args) => {
             if let Some(t) = binary_args.threads {