[cargo-nextest] block/unblock SIGTSTP while double-spawning

This is only really useful with Rust 1.66+ (currently in beta), which
has rust-lang/rust#101077. But let's get this in
so we can experiment with it.

Author: sunshowers

Cargo.lock

@@ -4,6 +4,7 @@
 use crate::{output::Color, ExpectedError, Result};
 use camino::Utf8PathBuf;
 use clap::Args;
+use nextest_runner::double_spawn::double_spawn_child_init;
 use std::os::unix::process::CommandExt;
 
 #[derive(Debug, Args)]
@@ -19,6 +20,7 @@ impl DoubleSpawnOpts {
     pub(crate) fn exec(self) -> Result<i32> {
         // This double-spawned process should never use coloring.
         Color::Never.init();
+        double_spawn_child_init();
         let args = shell_words::split(&self.args).map_err(|err| {
             ExpectedError::DoubleSpawnParseArgsError {
                 args: self.args,

cargo-nextest/src/double_spawn.rs

@@ -86,6 +86,7 @@ nextest-workspace-hack = { version = "0.1", path = "../workspace-hack" }
 
 [target.'cfg(unix)'.dependencies]
 libc = "0.2.137"
+nix = { version = "0.25.0", default-features = false, features = ["signal"] }
 
 [target.'cfg(windows)'.dependencies]
 windows = { version = "0.42.0", features = [

nextest-runner/Cargo.toml

@@ -5,8 +5,28 @@
 //!
 //! Nextest has experimental support on Unix for spawning test processes twice, to enable better
 //! isolation and solve some thorny issues.
+//!
+//! ## Issues this currently solves
+//!
+//! ### `posix_spawn` SIGTSTP race
+//!
+//! It's been empirically observed that if nextest receives a `SIGTSTP` (Ctrl-Z) while it's running,
+//! it can get completely stuck sometimes. This is due to a race between the child being spawned and it
+//! receiving a `SIGTSTP` signal.
+//!
+//! For more details, see [this
+//! message](https://sourceware.org/pipermail/libc-help/2022-August/006263.html) on the glibc-help
+//! mailing list.
+//!
+//! To solve this issue, we do the following:
+//!
+//! 1. In the main nextest runner process, using `DoubleSpawnContext`, block `SIGTSTP` in the
+//!    current thread (using `pthread_sigmask`) before spawning the stub child cargo-nextest
+//!    process.
+//! 2. In the stub child process, unblock `SIGTSTP`.
+//!
+//! With this approach, the race condition between posix_spawn and `SIGTSTP` no longer exists.
 
-use self::imp::DoubleSpawnInfoImp;
 use std::path::Path;
 
 /// Information about double-spawning processes. This determines whether a process will be
@@ -15,7 +35,7 @@ use std::path::Path;
 /// This is used by the main nextest process.
 #[derive(Clone, Debug)]
 pub struct DoubleSpawnInfo {
-    inner: DoubleSpawnInfoImp,
+    inner: imp::DoubleSpawnInfo,
 }
 
 impl DoubleSpawnInfo {
@@ -27,39 +47,75 @@ impl DoubleSpawnInfo {
     /// This is super experimental, and should be used with caution.
     pub fn enabled() -> Self {
         Self {
-            inner: DoubleSpawnInfoImp::enabled(),
+            inner: imp::DoubleSpawnInfo::enabled(),
         }
     }
 
     /// This returns a `DoubleSpawnInfo` which disables double-spawning.
     pub fn disabled() -> Self {
         Self {
-            inner: DoubleSpawnInfoImp::disabled(),
+            inner: imp::DoubleSpawnInfo::disabled(),
         }
     }
+
     /// Returns the current executable, if one is available.
     ///
     /// If `None`, double-spawning is not used.
     pub fn current_exe(&self) -> Option<&Path> {
         self.inner.current_exe()
     }
+
+    /// Returns a context that is meant to be obtained before spawning processes and dropped afterwards.
+    pub fn spawn_context(&self) -> Option<DoubleSpawnContext> {
+        self.current_exe().map(|_| DoubleSpawnContext::new())
+    }
+}
+
+/// Context to be used before spawning processes and dropped afterwards.
+///
+/// Returned by [`DoubleSpawnInfo::spawn_context`].
+#[derive(Debug)]
+pub struct DoubleSpawnContext {
+    // Only used for the Drop impl.
+    #[allow(dead_code)]
+    inner: imp::DoubleSpawnContext,
+}
+
+impl DoubleSpawnContext {
+    #[inline]
+    fn new() -> Self {
+        Self {
+            inner: imp::DoubleSpawnContext::new(),
+        }
+    }
+
+    /// Close the double-spawn context, dropping any changes that needed to be done to it.
+    pub fn finish(self) {}
+}
+
+/// Initialization for the double-spawn child.
+pub fn double_spawn_child_init() {
+    imp::double_spawn_child_init()
 }
 
 #[cfg(unix)]
 mod imp {
+    use nix::sys::signal::{SigSet, Signal};
+
     use super::*;
     use std::path::PathBuf;
 
     #[derive(Clone, Debug)]
-    pub(super) struct DoubleSpawnInfoImp {
+    pub(super) struct DoubleSpawnInfo {
         current_exe: Option<PathBuf>,
     }
 
-    impl DoubleSpawnInfoImp {
+    impl DoubleSpawnInfo {
         #[inline]
         pub(super) fn enabled() -> Self {
             // Attempt to obtain the current exe, and warn if it couldn't be found.
             // TODO: maybe add an option to fail?
+            // TODO: Always use /proc/self/exe directly on Linux, just make sure it's always accessible
             let current_exe = std::env::current_exe().map_or_else(
                 |error| {
                     log::warn!(
@@ -82,16 +138,50 @@ mod imp {
             self.current_exe.as_deref()
         }
     }
+
+    #[derive(Debug)]
+    pub(super) struct DoubleSpawnContext {
+        to_unblock: Option<SigSet>,
+    }
+
+    impl DoubleSpawnContext {
+        #[inline]
+        pub(super) fn new() -> Self {
+            // Block SIGTSTP, unblocking it in the child process. This avoids a complex race
+            // condition.
+            let mut sigset = SigSet::empty();
+            sigset.add(Signal::SIGTSTP);
+            let to_unblock = sigset.thread_block().ok().map(|()| sigset);
+            Self { to_unblock }
+        }
+    }
+
+    impl Drop for DoubleSpawnContext {
+        fn drop(&mut self) {
+            if let Some(sigset) = &self.to_unblock {
+                _ = sigset.thread_unblock();
+            }
+        }
+    }
+
+    #[inline]
+    pub(super) fn double_spawn_child_init() {
+        let mut sigset = SigSet::empty();
+        sigset.add(Signal::SIGTSTP);
+        if sigset.thread_unblock().is_err() {
+            log::warn!("[double-spawn] unable to unblock SIGTSTP in child");
+        }
+    }
 }
 
 #[cfg(not(unix))]
 mod imp {
     use super::*;
 
     #[derive(Clone, Debug)]
-    pub(super) struct DoubleSpawnInfoImp {}
+    pub(super) struct DoubleSpawnInfo {}
 
-    impl DoubleSpawnInfoImp {
+    impl DoubleSpawnInfo {
         #[inline]
         pub(super) fn enabled() -> Self {
             Self {}
@@ -107,4 +197,17 @@ mod imp {
             None
         }
     }
+
+    #[derive(Debug)]
+    pub(super) struct DoubleSpawnContext {}
+
+    impl DoubleSpawnContext {
+        #[inline]
+        pub(super) fn new() -> Self {
+            Self {}
+        }
+    }
+
+    #[inline]
+    pub(super) fn double_spawn_child_init() {}
 }

nextest-runner/src/double_spawn.rs

@@ -2,7 +2,9 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
 use crate::{
-    cargo_config::EnvironmentMap, double_spawn::DoubleSpawnInfo, helpers::dylib_path_envvar,
+    cargo_config::EnvironmentMap,
+    double_spawn::{DoubleSpawnContext, DoubleSpawnInfo},
+    helpers::dylib_path_envvar,
     target_runner::TargetRunner,
 };
 use camino::Utf8PathBuf;
@@ -25,6 +27,8 @@ pub(crate) struct LocalExecuteContext<'a> {
 pub(crate) struct TestCommand {
     /// The command to be run.
     command: std::process::Command,
+    /// Double-spawn context.
+    double_spawn: Option<DoubleSpawnContext>,
 }
 
 impl TestCommand {
@@ -164,7 +168,12 @@ impl TestCommand {
             cmd.env(format!("NEXTEST_BIN_EXE_{}", name), path);
         }
 
-        Self { command: cmd }
+        let double_spawn = ctx.double_spawn.spawn_context();
+
+        Self {
+            command: cmd,
+            double_spawn,
+        }
     }
 
     #[inline]
@@ -174,6 +183,10 @@ impl TestCommand {
 
     pub(crate) fn spawn(self) -> std::io::Result<tokio::process::Child> {
         let mut command = tokio::process::Command::from(self.command);
-        command.spawn()
+        let res = command.spawn();
+        if let Some(ctx) = self.double_spawn {
+            ctx.finish();
+        }
+        res
     }
 }

nextest-runner/src/test_command.rs

@@ -45,26 +45,26 @@ syn = { version = "1.0.103", features = ["clone-impls", "derive", "extra-traits"
 futures-core = { version = "0.3.25", features = ["alloc", "std"] }
 futures-sink = { version = "0.3.25" }
 indexmap = { version = "1.9.1", default-features = false, features = ["std"] }
-libc = { version = "0.2.137", features = ["std"] }
+libc = { version = "0.2.137", features = ["extra_traits", "std"] }
 once_cell = { version = "1.16.0", features = ["alloc", "race", "std"] }
 tokio = { version = "1.21.2", features = ["bytes", "io-util", "libc", "macros", "memchr", "mio", "net", "num_cpus", "process", "rt", "rt-multi-thread", "signal", "signal-hook-registry", "socket2", "sync", "time", "tokio-macros"] }
 
 [target.x86_64-unknown-linux-gnu.build-dependencies]
 getrandom = { version = "0.2.8", default-features = false, features = ["std"] }
-libc = { version = "0.2.137", features = ["std"] }
+libc = { version = "0.2.137", features = ["extra_traits", "std"] }
 once_cell = { version = "1.16.0", features = ["alloc", "race", "std"] }
 
 [target.x86_64-apple-darwin.dependencies]
 futures-core = { version = "0.3.25", features = ["alloc", "std"] }
 futures-sink = { version = "0.3.25" }
 indexmap = { version = "1.9.1", default-features = false, features = ["std"] }
-libc = { version = "0.2.137", features = ["std"] }
+libc = { version = "0.2.137", features = ["extra_traits", "std"] }
 once_cell = { version = "1.16.0", features = ["alloc", "race", "std"] }
 tokio = { version = "1.21.2", features = ["bytes", "io-util", "libc", "macros", "memchr", "mio", "net", "num_cpus", "process", "rt", "rt-multi-thread", "signal", "signal-hook-registry", "socket2", "sync", "time", "tokio-macros"] }
 
 [target.x86_64-apple-darwin.build-dependencies]
 getrandom = { version = "0.2.8", default-features = false, features = ["std"] }
-libc = { version = "0.2.137", features = ["std"] }
+libc = { version = "0.2.137", features = ["extra_traits", "std"] }
 once_cell = { version = "1.16.0", features = ["alloc", "race", "std"] }
 
 [target.x86_64-pc-windows-msvc.dependencies]