From 56e37d47c75d6604f60efe70e81d5ccb75956d66 Mon Sep 17 00:00:00 2001 From: Max Fierro Date: Tue, 9 Apr 2024 20:24:21 -0700 Subject: [PATCH] Fixed race condition on multithreaded test runs --- .gitignore | 2 +- src/game/test.rs | 11 ++----- src/game/zero_by/mod.rs | 1 - src/game/zero_by/states.rs | 1 - src/game/zero_by/variants.rs | 1 - src/test.rs | 62 +++++++++++++++++++++++++----------- 6 files changed, 48 insertions(+), 30 deletions(-) diff --git a/.gitignore b/.gitignore index 4a6fcf6..d3a658d 100644 --- a/.gitignore +++ b/.gitignore @@ -15,4 +15,4 @@ flamegraph.svg # Development data -/data +/dev diff --git a/src/game/test.rs b/src/game/test.rs index ea63b55..207e91e 100644 --- a/src/game/test.rs +++ b/src/game/test.rs @@ -12,9 +12,9 @@ use anyhow::Result; use petgraph::dot::{Config, Dot}; use std::fmt::Display; -use std::fs::create_dir; use std::fs::File; use std::io::Write; +use std::path::PathBuf; use std::process::{Command, Stdio}; use crate::game::mock; @@ -32,14 +32,9 @@ impl mock::Session<'_> { TestSetting::Development => (), } + let subdir = PathBuf::from(module); + let mut dir = get_directory(DevelopmentData::Visuals, subdir)?; let name = format!("{}.svg", self.name()).replace(" ", "-"); - let mut dir = get_directory(TestData::Visuals)?; - - dir.push(module); - if !dir.exists() { - create_dir(&dir) - .context("Failed to create module subdirectory.")?; - } dir.push(name); let file = File::create(dir)?; diff --git a/src/game/zero_by/mod.rs b/src/game/zero_by/mod.rs index dd03858..478ef05 100644 --- a/src/game/zero_by/mod.rs +++ b/src/game/zero_by/mod.rs @@ -10,7 +10,6 @@ //! emulate. //! //! #### Authorship -//! //! - Max Fierro, 4/6/2023 (maxfierro@berkeley.edu) use anyhow::{Context, Result}; diff --git a/src/game/zero_by/states.rs b/src/game/zero_by/states.rs index 06af900..a1af840 100644 --- a/src/game/zero_by/states.rs +++ b/src/game/zero_by/states.rs @@ -5,7 +5,6 @@ //! partially ensure compatibility with a game variant. //! //! #### Authorship -//! //! - Max Fierro, 11/2/2023 (maxfierro@berkeley.edu) use regex::Regex; diff --git a/src/game/zero_by/variants.rs b/src/game/zero_by/variants.rs index 7e63b31..b47b95d 100644 --- a/src/game/zero_by/variants.rs +++ b/src/game/zero_by/variants.rs @@ -4,7 +4,6 @@ //! into parameters that can help build a game session. //! //! #### Authorship -//! //! - Max Fierro, 11/2/2023 (maxfierro@berkeley.edu) use regex::Regex; diff --git a/src/test.rs b/src/test.rs index b9608f5..47c1d60 100644 --- a/src/test.rs +++ b/src/test.rs @@ -10,11 +10,23 @@ use anyhow::{anyhow, Context, Result}; use strum_macros::Display; -use std::{env, fs, path}; +use std::{ + env, fs, + path::{self, PathBuf}, + sync::RwLock, +}; /* CONSTANTS */ -const DATA_DIRECTORY: &str = "data"; +/// Global lock for creating development data directories. Since `cargo test` +/// executes tests in parallel, this helps prevent test flakiness by avoiding +/// race conditions when creating directories (specific files are still +/// susceptible, but they have no specific structure). +pub static DIRECTORY_LOCK: RwLock<()> = RwLock::new(()); + +/// The name of the global directory at the project root used for generated +/// development data. This directory is not shipped with release builds. +const DEV_DIRECTORY: &str = "dev"; /* DEFINITIONS */ @@ -22,7 +34,7 @@ const DATA_DIRECTORY: &str = "data"; /// purposes, which should not be distributed. #[derive(Display)] #[strum(serialize_all = "kebab-case")] -pub enum TestData { +pub enum DevelopmentData { Visuals, } @@ -55,33 +67,47 @@ pub fn test_setting() -> Result { } /// Returns a PathBuf corresponding to the correct subdirectory for storing -/// development `data`, creating it in the process if it does not exist. -pub fn get_directory(data: TestData) -> Result { +/// development `data` at a `module`-specific subdirectory, creating it in the +/// process if it does not exist. +pub fn get_directory( + data: DevelopmentData, + module: PathBuf, +) -> Result { let root = find_cargo_lock_directory() .context("Failed to find project root directory.")?; - let data_dir = root.join(DATA_DIRECTORY); - if !data_dir.exists() { - fs::create_dir(&data_dir) - .context("Failed to create data directory at project root.")?; - } - let name = format!("{}", data); - let dir = data_dir.join(name); - if !dir.exists() { - fs::create_dir(&dir) - .context("Failed to create subdirectory inside data directory.")?; + let directory = root + .join(DEV_DIRECTORY) + .join(format!("{}", data)) + .join(module); + + let guard = { + let _lock = DIRECTORY_LOCK.read().unwrap(); + directory.try_exists()? + }; + + if !guard { + // Does not completely prevent multiple threads from attempting to + // create the same directory path, but `create_dir_all` is resilient + // to this regardless. This is only necessary for preventing race + // conditions within `find_cargo_lock_directory`. + let _lock = DIRECTORY_LOCK.write().unwrap(); + fs::create_dir_all(&directory) + .context("Failed to create module subdirectory.")?; } - Ok(dir) + + Ok(directory) } /* HELPER FUNCTIONS */ /// Searches for a parent directory containing a `Cargo.lock` file. fn find_cargo_lock_directory() -> Result { + let _lock = DIRECTORY_LOCK.read().unwrap(); let mut cwd = env::current_dir()?; loop { - let lock_file = cwd.join("Cargo.lock"); - if lock_file.exists() && lock_file.is_file() { + let cargo_lock = cwd.join("Cargo.lock"); + if cargo_lock.try_exists()? && cargo_lock.is_file() { return Ok(cwd); } if let Some(parent_dir) = cwd.parent() {