From 4c1852b6181256ebd744b924cb2dd46aaa0f918f Mon Sep 17 00:00:00 2001 From: Rain Date: Mon, 7 Nov 2022 11:00:19 -0800 Subject: [PATCH] [cargo-nextest] block/unblock SIGTSTP while double-spawning This is only really useful with Rust 1.66+ (currently in beta), which has https://github.com/rust-lang/rust/pull/101077. But let's get this in so we can experiment with it. --- Cargo.lock | 13 ++++ cargo-nextest/src/double_spawn.rs | 2 + nextest-runner/Cargo.toml | 1 + nextest-runner/src/double_spawn.rs | 119 +++++++++++++++++++++++++++-- nextest-runner/src/test_command.rs | 19 ++++- workspace-hack/Cargo.toml | 8 +- 6 files changed, 147 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8ef06607e14..e3a65788bb9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1244,6 +1244,7 @@ dependencies = [ "nextest-filtering", "nextest-metadata", "nextest-workspace-hack", + "nix", "once_cell", "owo-colors", "pathdiff", @@ -1310,6 +1311,18 @@ dependencies = [ "winapi", ] +[[package]] +name = "nix" +version = "0.25.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e322c04a9e3440c327fca7b6c8a63e6890a32fa2ad689db972425f07e0d22abb" +dependencies = [ + "autocfg", + "bitflags", + "cfg-if", + "libc", +] + [[package]] name = "nom" version = "5.1.2" diff --git a/cargo-nextest/src/double_spawn.rs b/cargo-nextest/src/double_spawn.rs index b455894ad5b..8ff77c2446f 100644 --- a/cargo-nextest/src/double_spawn.rs +++ b/cargo-nextest/src/double_spawn.rs @@ -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 { // 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, diff --git a/nextest-runner/Cargo.toml b/nextest-runner/Cargo.toml index 811fb5cf8d2..72bd0d69c6f 100644 --- a/nextest-runner/Cargo.toml +++ b/nextest-runner/Cargo.toml @@ -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 = [ diff --git a/nextest-runner/src/double_spawn.rs b/nextest-runner/src/double_spawn.rs index 55caa1e0249..7f1ca89e4ae 100644 --- a/nextest-runner/src/double_spawn.rs +++ b/nextest-runner/src/double_spawn.rs @@ -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 { + 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, } - 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,6 +138,40 @@ mod imp { self.current_exe.as_deref() } } + + #[derive(Debug)] + pub(super) struct DoubleSpawnContext { + to_unblock: Option, + } + + 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))] @@ -89,9 +179,9 @@ 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() {} } diff --git a/nextest-runner/src/test_command.rs b/nextest-runner/src/test_command.rs index caa59e33fe8..c836eb15845 100644 --- a/nextest-runner/src/test_command.rs +++ b/nextest-runner/src/test_command.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, } 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 { 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 } } diff --git a/workspace-hack/Cargo.toml b/workspace-hack/Cargo.toml index 665498d9891..762e3c39655 100644 --- a/workspace-hack/Cargo.toml +++ b/workspace-hack/Cargo.toml @@ -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]