From 30540f7952296daf3fb53432788a868eeee4f504 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Sat, 14 Mar 2026 16:09:06 +0100 Subject: [PATCH 1/3] Move code from the macro to the private module That's easier to write and test and makes the api surface of the private module a bit smaller. --- src/macros.rs | 22 +---------- src/private.rs | 104 ++++++++++++++++++++++++++++++++----------------- 2 files changed, 69 insertions(+), 57 deletions(-) diff --git a/src/macros.rs b/src/macros.rs index c79296e..6dddffc 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -136,26 +136,6 @@ macro_rules! testdir { #[macro_export] macro_rules! init_testdir { () => {{ - $crate::TESTDIR.get_or_init(move || { - let parent = match $crate::private::cargo_metadata::MetadataCommand::new().exec() { - Ok(metadata) => metadata.target_directory.into(), - Err(_) => { - // In some environments cargo-metadata is not available, - // e.g. cargo-dinghy. Use the directory of test executable. - let current_exe = ::std::env::current_exe().expect("no current exe"); - current_exe - .parent() - .expect("no parent dir for current exe") - .into() - } - }; - let pkg_name = "testdir"; - let mut builder = $crate::NumberedDirBuilder::new(pkg_name.to_string()); - builder.set_parent(parent); - builder.reusefn($crate::private::reuse_cargo); - let testdir = builder.create().expect("Failed to create testdir"); - $crate::private::create_cargo_pid_file(testdir.path()); - testdir - }) + $crate::TESTDIR.get_or_init($crate::private::init_testdir); }}; } diff --git a/src/private.rs b/src/private.rs index 79be3e1..bfc6b8f 100644 --- a/src/private.rs +++ b/src/private.rs @@ -7,12 +7,12 @@ use std::ffi::OsStr; use std::fs; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::sync::LazyLock; use sysinfo::Pid; -pub use cargo_metadata; +use crate::{NumberedDir, NumberedDirBuilder}; /// The filename in which we store the Cargo PID: `cargo-pid`. const CARGO_PID_FILE_NAME: &str = "cargo-pid"; @@ -32,6 +32,71 @@ const CARGO_NAME: &str = "cargo.exe"; #[cfg(target_family = "windows")] const NEXTEST_NAME: &str = "cargo-nextest.exe"; +/// Implementation of `crate::macros::init_testdir`. +pub fn init_testdir() -> NumberedDir { + let parent = cargo_target_dir(); + let pkg_name = "testdir"; + let mut builder = NumberedDirBuilder::new(pkg_name.to_string()); + builder.set_parent(parent); + builder.reusefn(reuse_cargo); + let testdir = builder.create().expect("Failed to create testdir"); + create_cargo_pid_file(testdir.path()); + testdir +} + +/// Returns the cargo target directory or a best guess. +/// +/// This aims to return the cargo target directory. Though in some environments like +/// cargo-dinghy cargo_metadata is not available. In those cases the directory of the test +/// executable is used, which usually is somewhere in the target directory. +fn cargo_target_dir() -> PathBuf { + match cargo_metadata::MetadataCommand::new().exec() { + Ok(metadata) => metadata.target_directory.into(), + Err(_) => { + // In some environments cargo-metadata is not available, + // e.g. cargo-dinghy. Use the directory of test executable. + let current_exe = ::std::env::current_exe().expect("no current exe"); + current_exe + .parent() + .expect("no parent dir for current exe") + .into() + } + } +} + +/// Determines if a [`NumberedDir`] was created by the same cargo parent process. +/// +/// Commands like `cargo test` run various tests in sub-processes (unittests, doctests, +/// integration tests). All of those subprocesses should re-use the same [`NumberedDir`]. +/// This function figures out whether the given directory is the correct one or not. +/// +/// [`NumberedDir`]: crate::NumberedDir +pub(crate) fn reuse_cargo(dir: &Path) -> bool { + let file_name = dir.join(CARGO_PID_FILE_NAME); + if let Ok(content) = fs::read_to_string(file_name) { + if let Ok(read_cargo_pid) = content.parse::() { + if let Some(cargo_pid) = *CARGO_PID { + return read_cargo_pid == cargo_pid; + } + } + } + false +} + +/// Creates a file storing the Cargo PID if not yet present. +/// +/// # Panics +/// +/// If the PID file could not be created or written. +pub(crate) fn create_cargo_pid_file(dir: &Path) { + if let Some(cargo_pid) = *CARGO_PID { + let file_name = dir.join(CARGO_PID_FILE_NAME); + if !file_name.exists() { + fs::write(&file_name, cargo_pid.to_string()).expect("Failed to write Cargo PID"); + } + } +} + /// Returns the process ID of our parent Cargo process. /// /// If our parent process is not Cargo, `None` is returned. @@ -78,39 +143,6 @@ fn cargo_pid() -> Option { } } -/// Determines if a [`NumberedDir`] was created by the same cargo parent process. -/// -/// Commands like `cargo test` run various tests in sub-processes (unittests, doctests, -/// integration tests). All of those subprocesses should re-use the same [`NumberedDir`]. -/// This function figures out whether the given directory is the correct one or not. -/// -/// [`NumberedDir`]: crate::NumberedDir -pub fn reuse_cargo(dir: &Path) -> bool { - let file_name = dir.join(CARGO_PID_FILE_NAME); - if let Ok(content) = fs::read_to_string(file_name) { - if let Ok(read_cargo_pid) = content.parse::() { - if let Some(cargo_pid) = *CARGO_PID { - return read_cargo_pid == cargo_pid; - } - } - } - false -} - -/// Creates a file storing the Cargo PID if not yet present. -/// -/// # Panics -/// -/// If the PID file could not be created or written. -pub fn create_cargo_pid_file(dir: &Path) { - if let Some(cargo_pid) = *CARGO_PID { - let file_name = dir.join(CARGO_PID_FILE_NAME); - if !file_name.exists() { - fs::write(&file_name, cargo_pid.to_string()).expect("Failed to write Cargo PID"); - } - } -} - /// Extracts the name of the currently executing test. pub fn extract_test_name(module_path: &str) -> String { let mut name = std::thread::current() @@ -127,7 +159,7 @@ pub fn extract_test_name(module_path: &str) -> String { } /// Extracts the name of the currently executing tests using [`backtrace`]. -pub fn extract_test_name_from_backtrace(module_path: &str) -> String { +fn extract_test_name_from_backtrace(module_path: &str) -> String { for symbol in backtrace::Backtrace::new() .frames() .iter() From f348f762b6bd4bfd59b08467af3022f774f07d42 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Sat, 14 Mar 2026 17:52:44 +0100 Subject: [PATCH 2/3] Wait for a cargo-pid file to appear if none exists When multiple processes are racing to create the next testdir, as is the case with cargo-nextest, a process might encounter a a numbered dir with no cargo-pid file present. Previously it would assume this directory can not be used and create the next one. Which results in cargo-nextest creating directories per thread, often deleting some earlier testdirs that are being used by the same testrun. By waiting a little for the pidfile to appear we can avoid this and do correctly reuse the directory. If two processes create the same numbered dir concurrently this will be fine, since only one will create the cargo-pid file or they will create the same cargo-pid file. Fixes #17. --- src/private.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/private.rs b/src/private.rs index bfc6b8f..340e7f5 100644 --- a/src/private.rs +++ b/src/private.rs @@ -9,6 +9,7 @@ use std::ffi::OsStr; use std::fs; use std::path::{Path, PathBuf}; use std::sync::LazyLock; +use std::time::{Duration, Instant}; use sysinfo::Pid; @@ -73,13 +74,21 @@ fn cargo_target_dir() -> PathBuf { /// [`NumberedDir`]: crate::NumberedDir pub(crate) fn reuse_cargo(dir: &Path) -> bool { let file_name = dir.join(CARGO_PID_FILE_NAME); - if let Ok(content) = fs::read_to_string(file_name) { - if let Ok(read_cargo_pid) = content.parse::() { - if let Some(cargo_pid) = *CARGO_PID { - return read_cargo_pid == cargo_pid; - } + let start = Instant::now(); + while start.elapsed() <= Duration::from_millis(500) { + if let Some(read_cargo_pid) = fs::read_to_string(&file_name) + .ok() + .and_then(|content| content.parse::().ok()) + { + return Some(read_cargo_pid) == *CARGO_PID; + } else { + // Wen we encounter a directory that has no pidfile we assume some other process + // just created the directory and is about to write the pdifile. So we wait a + // little in the hope the pidfile appears. + std::thread::sleep(Duration::from_millis(5)); } } + // Give up, we'll create a new directory ourselves. false } From 8435461c833d354b8558465b6be7b99c087c7d94 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Sat, 14 Mar 2026 19:17:27 +0100 Subject: [PATCH 3/3] Protect against different runs racing testdir creation If multiple test runs are started at virutally the same time they could accidentally start using the same testdir. This protects against this by making sure to not overwrite a pidfile and check if the correct pid is inside. If it is not, then it tries again. --- src/lib.rs | 11 +++++++++-- src/private.rs | 43 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 4bece74..06d3f86 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -104,8 +104,15 @@ where let test_dir = TESTDIR.get_or_init(|| { let mut builder = NumberedDirBuilder::new(String::from("init_testdir-not-called")); builder.reusefn(private::reuse_cargo); - let testdir = builder.create().expect("Failed to create testdir"); - private::create_cargo_pid_file(testdir.path()); + let mut testdir = builder.create().expect("Failed to create testdir"); + let mut count = 0; + while private::create_cargo_pid_file(testdir.path()).is_err() { + count += 1; + if count > 20 { + break; + } + testdir = builder.create().expect("Failed to create testdir"); + } testdir }); func(test_dir) diff --git a/src/private.rs b/src/private.rs index 340e7f5..a679af4 100644 --- a/src/private.rs +++ b/src/private.rs @@ -6,11 +6,12 @@ //! stability and this will violate semvers. use std::ffi::OsStr; -use std::fs; +use std::fs::{self, File}; use std::path::{Path, PathBuf}; use std::sync::LazyLock; use std::time::{Duration, Instant}; +use anyhow::Result; use sysinfo::Pid; use crate::{NumberedDir, NumberedDirBuilder}; @@ -40,8 +41,15 @@ pub fn init_testdir() -> NumberedDir { let mut builder = NumberedDirBuilder::new(pkg_name.to_string()); builder.set_parent(parent); builder.reusefn(reuse_cargo); - let testdir = builder.create().expect("Failed to create testdir"); - create_cargo_pid_file(testdir.path()); + let mut testdir = builder.create().expect("Failed to create testdir"); + let mut count = 0; + while create_cargo_pid_file(testdir.path()).is_err() { + count += 1; + if count > 20 { + break; + } + testdir = builder.create().expect("Failed to create testdir"); + } testdir } @@ -94,14 +102,31 @@ pub(crate) fn reuse_cargo(dir: &Path) -> bool { /// Creates a file storing the Cargo PID if not yet present. /// +/// # Returns +/// +/// An error return indicates that the pid file was created by another process that was not +/// part of our testrun. So this numbered dir should not be used. +/// /// # Panics /// -/// If the PID file could not be created or written. -pub(crate) fn create_cargo_pid_file(dir: &Path) { - if let Some(cargo_pid) = *CARGO_PID { - let file_name = dir.join(CARGO_PID_FILE_NAME); - if !file_name.exists() { - fs::write(&file_name, cargo_pid.to_string()).expect("Failed to write Cargo PID"); +/// If the PID file could not be created, written or read. +pub(crate) fn create_cargo_pid_file(dir: &Path) -> Result<()> { + let cargo_pid = CARGO_PID + .map(|pid| pid.to_string()) + .unwrap_or("failed to get cargo PID".to_string()); + let file_name = dir.join(CARGO_PID_FILE_NAME); + match File::create_new(&file_name) { + Ok(_) => { + fs::write(&file_name, cargo_pid).expect("Failed to write cargo PID"); + Ok(()) + } + Err(_) => { + let contents = fs::read_to_string(&file_name).expect("Failed to read cargo-pid"); + if cargo_pid == contents { + Ok(()) + } else { + Err(anyhow::Error::msg("cargo PID does not match")) + } } } }