In unix like system, set foreground process while running external command (#6206)

* while executing external command, make it as foreground

* remove useless file

* add comment, make var more readable

* add comment

* fmt code

* fix windows

* fix func name

* fix clippy

* fix windows clippy

* add comments, introduce `ForegroundProcess and ForegroundChild

* fix windows clippy

* fix on windows

* no need fg_process_setup module

* Revert "no need fg_process_setup module"

This reverts commit 21ee4ffbf6.

* restrict visibility for helper functions

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

@@ -13,6 +13,7 @@ use nu_protocol::{Category, Example, ListStream, PipelineData, RawStream, Span,
 use itertools::Itertools;
 
 use nu_engine::CallExt;
+use nu_system::ForegroundProcess;
 use pathdiff::diff_paths;
 use regex::Regex;
 
@@ -122,15 +123,16 @@ 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)?);
         let child;
 
         #[cfg(windows)]
         {
-            match process.spawn() {
+            match fg_process.spawn() {
                 Err(_) => {
-                    let mut process = self.create_process(&input, true, head)?;
-                    child = process.spawn();
+                    let mut fg_process =
+                        ForegroundProcess::new(self.create_process(&input, true, head)?);
+                    child = fg_process.spawn();
                 }
                 Ok(process) => {
                     child = Ok(process);
@@ -140,7 +142,7 @@ impl ExternalCommand {
 
         #[cfg(not(windows))]
         {
-            child = process.spawn()
+            child = fg_process.spawn()
         }
 
         match child {
@@ -158,7 +160,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,
@@ -199,7 +201,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"
@@ -238,7 +240,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"
@@ -276,7 +278,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

@@ -13,7 +13,7 @@ name = "ps"
 path = "src/main.rs"
 
 [dependencies]
-
+libc = "0.2"
 
 [target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies]
 procfs  = "0.13.0"
@@ -21,11 +21,9 @@ procfs  = "0.13.0"
 [target.'cfg(target_os = "macos")'.dependencies]
 libproc = "0.12.0"
 errno = "0.2"
-libc = "0.2"
 
 [target.'cfg(target_os = "windows")'.dependencies]
 winapi = { version = "0.3.9", features = ["tlhelp32", "fileapi", "handleapi", "ifdef", "ioapiset", "minwindef", "pdh", "psapi", "synchapi", "sysinfoapi", "winbase", "winerror", "winioctl", "winnt", "oleauto", "wbemcli", "rpcdce", "combaseapi", "objidl", "powerbase", "netioapi", "lmcons", "lmaccess", "lmapibuf", "memoryapi", "shellapi", "std", "securitybaseapi"] }
 chrono = "0.4"
-libc = "0.2"
 ntapi = "0.3"
 once_cell = "1.0"

crates/nu-system/src/foreground.rs

@@ -0,0 +1,111 @@
+use std::process::{Child, Command};
+
+/// A simple wrapper for `std::process::Command`
+///
+/// ## spawn behavior
+/// ### Unix
+/// When invoke `spawn`, current process will ignore `SIGTTOU`, `SIGTTIN` signal, 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, `SIGTTOU`, `SIGTTIN` signal will be reset, foreground process is back to callers' process.
+///
+/// ### 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.
+//
+// For more information, please check `child_setup_process` function in fish shell.
+// https://github.com/fish-shell/fish-shell/blob/3f90efca38079922b4b21707001d7bb9630107eb/src/postfork.cpp#L140
+#[cfg(target_family = "unix")]
+mod fg_process_setup {
+    use std::os::unix::prelude::CommandExt;
+    pub(super) fn prepare_to_foreground(external_command: &mut std::process::Command) {
+        unsafe {
+            libc::signal(libc::SIGTTOU, libc::SIG_IGN);
+            libc::signal(libc::SIGTTIN, libc::SIG_IGN);
+
+            // 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
+                libc::setpgid(0, 0);
+                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) {
+        // it's ok to use unsafe here
+        // the implementaion here is just the same as
+        // https://docs.rs/nix/latest/nix/unistd/fn.tcsetpgrp.html, which is a safe function.
+        unsafe {
+            libc::tcsetpgrp(libc::STDIN_FILENO, process.id() as i32);
+        }
+    }
+
+    /// Reset foreground to current process, and reset back `SIGTTOU`, `SIGTTIN` single handler.
+    ///
+    /// ## Safety
+    /// It can only be called when you have called `set_foreground`, or results in undefined behavior.
+    pub(super) unsafe fn reset_foreground_id() {
+        libc::tcsetpgrp(libc::STDIN_FILENO, libc::getpgrp());
+        libc::signal(libc::SIGTTOU, libc::SIG_DFL);
+        libc::signal(libc::SIGTTIN, libc::SIG_DFL);
+    }
+}
+
+// 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,3 +1,4 @@
+mod foreground;
 #[cfg(any(target_os = "android", target_os = "linux"))]
 mod linux;
 #[cfg(target_os = "macos")]
@@ -5,6 +6,7 @@ mod macos;
 #[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")]