add comments, introduce `ForegroundProcess and ForegroundChild

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

@@ -13,9 +13,7 @@ use nu_protocol::{Category, Example, ListStream, PipelineData, RawStream, Span,
 use itertools::Itertools;
 
 use nu_engine::CallExt;
-use nu_system::external_process_setup::{
+use nu_system::ForegroundProcess;
-    prepare_to_foreground, reset_foreground_id, set_foreground,
-};
 use pathdiff::diff_paths;
 use regex::Regex;
 
@@ -125,15 +123,15 @@ 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)?);
-        prepare_to_foreground(&mut process);
         let child;
 
         #[cfg(windows)]
         {
-            match process.spawn() {
+            match fg_process.spawn() {
                 Err(_) => {
-                    let mut process = self.create_process(&input, true, head)?;
+                    let mut fg_process =
+                        ForegroundProcess::new(self.create_process(&input, true, head)?);
                     child = process.spawn();
                 }
                 Ok(process) => {
@@ -144,7 +142,7 @@ impl ExternalCommand {
 
         #[cfg(not(windows))]
         {
-            child = process.spawn()
+            child = fg_process.spawn()
         }
 
         match child {
@@ -154,7 +152,6 @@ impl ExternalCommand {
                 self.name.span,
             )),
             Ok(mut child) => {
-                set_foreground(&child);
                 if !input.is_nothing() {
                     let mut engine_state = engine_state.clone();
                     let mut stack = stack.clone();
@@ -163,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,
@@ -204,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"
@@ -243,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"
@@ -281,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(),
@@ -301,7 +298,6 @@ impl ExternalCommand {
                                     span: head,
                                 });
                             }
-                            reset_foreground_id();
                             Ok(())
                         }
                     }

crates/nu-system/src/foreground.rs

@@ -1,20 +1,76 @@
-// It's a simpler version for fish shell's external process handling.
+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")]
-pub mod external_process_setup {
+mod fg_process_setup {
     use std::os::unix::prelude::CommandExt;
     pub 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, refer to `fork_child_for_process` function:
+                // 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(())
@@ -22,28 +78,30 @@ pub mod external_process_setup {
         }
     }
 
-    /// Before call this function, `prepare_to_foreground` function must be called.  Or else
+    // If `prepare_to_foreground` function is not called, the function will fail with silence and do nothing.
-    /// it'll failed with silence.
     pub 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);
         }
     }
 
-    /// It can only be called when you have called `set_foreground`, or else
+    /// Reset foreground to current process, and reset back `SIGTTOU`, `SIGTTIN` single handler.
-    /// Something strange will happened.
+    ///
-    pub fn reset_foreground_id() {
+    /// ## Safety
-        unsafe {
+    /// It can only be called when you have called `set_foreground`, or results in undefined behavior.
+    pub 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")]
-pub mod external_process_setup {
+mod fg_process_setup {
 
     pub fn prepare_to_foreground(_external_command: &mut std::process::Command) {}
 

crates/nu-system/src/lib.rs

@@ -6,7 +6,7 @@ mod macos;
 #[cfg(target_os = "windows")]
 mod windows;
 
-pub use self::foreground::external_process_setup;
+pub use self::foreground::{ForegroundChild, ForegroundProcess};
 #[cfg(any(target_os = "android", target_os = "linux"))]
 pub use self::linux::*;
 #[cfg(target_os = "macos")]