Enable macOS foreground process handling (#9909)

# Description
Currently, foreground process management is disabled for macOS, since
the original code had issues (see #7068).
This PR re-enables process management on macOS in combination with the
changes from #9693.

# User-Facing Changes
Fixes hang on exit for nested nushells on macOS (issue #9859). Nushell
should now manage processes in the same way on macOS and other unix
systems.

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

@@ -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() {
+            match fg_process.spawn(engine_state.is_interactive) {
                 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();
+                        child = cmd_process.spawn(engine_state.is_interactive);
                     } else {
                         #[cfg(feature = "which-support")]
                         {
@@ -269,7 +269,8 @@ impl ExternalCommand {
                                                     cmd,
                                                     engine_state.pipeline_externals_state.clone(),
                                                 );
-                                                child = cmd_process.spawn();
+                                                child =
+                                                    cmd_process.spawn(engine_state.is_interactive);
                                             }
                                         }
                                     }
@@ -286,7 +287,7 @@ impl ExternalCommand {
 
         #[cfg(not(windows))]
         {
-            child = fg_process.spawn()
+            child = fg_process.spawn(engine_state.is_interactive)
         }
 
         match child {

crates/nu-system/src/foreground.rs

@@ -28,6 +28,7 @@ pub struct ForegroundProcess {
 pub struct ForegroundChild {
     inner: Child,
     pipeline_state: Arc<(AtomicU32, AtomicU32)>,
+    interactive: bool,
 }
 
 impl ForegroundProcess {
@@ -38,14 +39,14 @@ impl ForegroundProcess {
         }
     }
 
-    pub fn spawn(&mut self) -> std::io::Result<ForegroundChild> {
+    pub fn spawn(&mut self, interactive: bool) -> 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);
+        fg_process_setup::prepare_to_foreground(&mut self.inner, existing_pgrp, interactive);
         self.inner
             .spawn()
             .map(|child| {
-                fg_process_setup::set_foreground(&child, existing_pgrp);
+                fg_process_setup::set_foreground(&child, existing_pgrp, interactive);
                 let _ = pcnt.fetch_add(1, Ordering::SeqCst);
                 if existing_pgrp == 0 {
                     pgrp.store(child.id(), Ordering::SeqCst);
@@ -53,10 +54,11 @@ impl ForegroundProcess {
                 ForegroundChild {
                     inner: child,
                     pipeline_state: self.pipeline_state.clone(),
+                    interactive,
                 }
             })
             .map_err(|e| {
-                fg_process_setup::reset_foreground_id();
+                fg_process_setup::reset_foreground_id(interactive);
                 e
             })
     }
@@ -73,14 +75,13 @@ 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()
+            fg_process_setup::reset_foreground_id(self.interactive)
         }
     }
 }
 
 // It's a simpler version of fish shell's external process handling.
-// Note: we exclude macos because the techniques below seem to have issues in macos 13 currently.
-#[cfg(all(target_family = "unix", not(target_os = "macos")))]
+#[cfg(unix)]
 mod fg_process_setup {
     use is_terminal::IsTerminal;
     use nix::{
@@ -101,8 +102,10 @@ 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.
@@ -124,7 +127,9 @@ 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.
-                set_foreground_pid(unistd::getpid(), existing_pgrp, tty.0);
+                if interactive {
+                    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();
@@ -137,9 +142,13 @@ mod fg_process_setup {
         }
     }
 
-    pub(super) fn set_foreground(process: &std::process::Child, existing_pgrp: u32) {
+    pub(super) fn set_foreground(
+        process: &std::process::Child,
+        existing_pgrp: u32,
+        interactive: bool,
+    ) {
         // called from the parent shell process - do the stdin tty check here
-        if std::io::stdin().is_terminal() {
+        if interactive && std::io::stdin().is_terminal() {
             set_foreground_pid(
                 Pid::from_raw(process.id() as i32),
                 existing_pgrp,
@@ -163,8 +172,8 @@ mod fg_process_setup {
     }
 
     /// Reset the foreground process group to the shell
-    pub(super) fn reset_foreground_id() {
-        if std::io::stdin().is_terminal() {
+    pub(super) fn reset_foreground_id(interactive: bool) {
+        if interactive && 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:?}");
             }
@@ -172,11 +181,11 @@ mod fg_process_setup {
     }
 }
 
-#[cfg(any(not(target_family = "unix"), target_os = "macos"))]
+#[cfg(not(unix))]
 mod fg_process_setup {
-    pub(super) fn prepare_to_foreground(_: &mut std::process::Command, _: u32) {}
+    pub(super) fn prepare_to_foreground(_: &mut std::process::Command, _: u32, _: bool) {}
 
-    pub(super) fn set_foreground(_: &std::process::Child, _: u32) {}
+    pub(super) fn set_foreground(_: &std::process::Child, _: u32, _: bool) {}
 
-    pub(super) fn reset_foreground_id() {}
+    pub(super) fn reset_foreground_id(_: bool) {}
 }

src/main.rs

@@ -82,7 +82,10 @@ 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();
+    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_login = parsed_nu_cli_args.login_shell.is_some();
 
@@ -144,7 +147,7 @@ fn main() -> Result<()> {
     );
 
     start_time = std::time::Instant::now();
-    acquire_terminal(parsed_nu_cli_args.commands.is_none() && script_name.is_empty());
+    acquire_terminal(engine_state.is_interactive);
     perf(
         "acquire_terminal",
         start_time,
@@ -290,7 +293,6 @@ fn main() -> Result<()> {
             input,
         )
     } else {
-        engine_state.is_interactive = true;
         run_repl(&mut engine_state, parsed_nu_cli_args, entire_start_time)
     }
 }

src/terminal.rs

@@ -1,19 +1,39 @@
 #[cfg(unix)]
 pub(crate) fn acquire_terminal(interactive: bool) {
     use is_terminal::IsTerminal;
-    use nix::sys::signal::{signal, SigHandler, Signal};
+    use nix::{
+        errno::Errno,
+        sys::signal::{signal, SigHandler, Signal},
+        unistd,
+    };
 
-    if !std::io::stdin().is_terminal() {
-        return;
-    }
+    if interactive && std::io::stdin().is_terminal() {
+        // see also: https://www.gnu.org/software/libc/manual/html_node/Initializing-the-Shell.html
 
-    take_control(interactive);
+        take_control();
 
-    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");
+        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, 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);
+            }
+        }
+        // Set our possibly new pgid to be in control of terminal
+        let _ = unistd::tcsetpgrp(nix::libc::STDIN_FILENO, shell_pgid);
     }
 }
 
@@ -22,7 +42,7 @@ pub(crate) fn acquire_terminal(_: bool) {}
 
 // Inspired by fish's acquire_tty_or_exit
 #[cfg(unix)]
-fn take_control(interactive: bool) {
+fn take_control() {
     use nix::{
         errno::Errno,
         sys::signal::{self, SaFlags, SigAction, SigHandler, SigSet, Signal},
@@ -59,32 +79,23 @@ fn take_control(interactive: bool) {
         }
     }
 
-    let mut success = false;
     for _ in 0..4096 {
         match unistd::tcgetpgrp(nix::libc::STDIN_FILENO) {
             Ok(owner_pgid) if owner_pgid == shell_pgid => {
-                success = true;
-                break;
+                // success
+                return;
             }
             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(Pid::from_raw(-shell_pgid.as_raw()), Signal::SIGTTIN).is_err() {
-                    if !interactive {
-                        // that's fine
-                        return;
-                    }
+                if signal::killpg(shell_pgid, Signal::SIGTTIN).is_err() {
                     eprintln!("ERROR: failed to SIGTTIN ourselves");
                     std::process::exit(1);
                 }
@@ -92,8 +103,6 @@ fn take_control(interactive: bool) {
         }
     }
 
-    if !success && interactive {
-        eprintln!("ERROR: failed take control of the terminal, we might be orphaned");
-        std::process::exit(1);
-    }
+    eprintln!("ERROR: failed take control of the terminal, we might be orphaned");
+    std::process::exit(1);
 }