Merge pull request #84 from delan/fix-thread-unsafety

tao-guo · web-flow · commit 171690e92158 · 2025-08-02T20:45:40.000-07:00
Fix thread unsafety due to internal use of set_var()
diff --git a/README.md b/README.md
@@ -304,11 +304,10 @@ You can use [std::env::var](https://doc.rust-lang.org/std/env/fn.var.html) to fe
 key from the current process. It will report error if the environment variable is not present, and it also
 includes other checks to avoid silent failures.
 
-To set environment variables, you can use [std::env::set_var](https://doc.rust-lang.org/std/env/fn.set_var.html).
-There are also other related APIs in the [std::env](https://doc.rust-lang.org/std/env/index.html) module.
+To set environment variables in **single-threaded programs**, you can use [std::env::set_var] and
+[std::env::remove_var]. While those functions **[must not be called]** if any other threads might be running, you can
+always set environment variables for one command at a time, by putting the assignments before the command:
 
-To set environment variables for the command only, you can put the assignments before the command.
-Like this:
 ```rust
 run_cmd!(FOO=100 /tmp/test_run_cmd_lib.sh)?;
 ```
@@ -330,9 +329,21 @@ You can use the [glob](https://github.com/rust-lang-nursery/glob) package instea
 
 #### Thread Safety
 
-This library tries very hard to not set global states, so parallel `cargo test` can be executed just fine.
-The only known APIs not supported in multi-thread environment are the
-[`tls_init!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_init.html)/[`tls_get!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_get.html)/[`tls_set!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_set.html) macros, and you should only use them for *thread local* variables.
+This library tries very hard to not set global state, so parallel `cargo test` can be executed just fine.
+That said, there are some limitations to be aware of:
+
+- [std::env::set_var] and [std::env::remove_var] **[must not be called]** in a multi-threaded program
+- [`tls_init!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_init.html),
+  [`tls_get!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_get.html), and
+  [`tls_set!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_set.html) create *thread-local* variables, which means
+  each thread will have its own independent version of the variable
+- [`set_debug`](https://docs.rs/cmd_lib/latest/cmd_lib/fn.set_debug.html) and
+  [`set_pipefail`](https://docs.rs/cmd_lib/latest/cmd_lib/fn.set_pipefail.html) are *global* and affect all threads;
+  there is currently no way to change those settings without affecting other threads
+
+[std::env::set_var]: https://doc.rust-lang.org/std/env/fn.set_var.html
+[std::env::remove_var]: https://doc.rust-lang.org/std/env/fn.remove_var.html
+[must not be called]: https://doc.rust-lang.org/nightly/edition-guide/rust-2024/newly-unsafe-functions.html#stdenvset_var-remove_var
 
 
 License: MIT OR Apache-2.0
diff --git a/src/lib.rs b/src/lib.rs
@@ -334,11 +334,10 @@
 //! key from the current process. It will report error if the environment variable is not present, and it also
 //! includes other checks to avoid silent failures.
 //!
-//! To set environment variables, you can use [std::env::set_var](https://doc.rust-lang.org/std/env/fn.set_var.html).
-//! There are also other related APIs in the [std::env](https://doc.rust-lang.org/std/env/index.html) module.
+//! To set environment variables in **single-threaded programs**, you can use [std::env::set_var] and
+//! [std::env::remove_var]. While those functions **[must not be called]** if any other threads might be running, you can
+//! always set environment variables for one command at a time, by putting the assignments before the command:
 //!
-//! To set environment variables for the command only, you can put the assignments before the command.
-//! Like this:
 //! ```no_run
 //! # use cmd_lib::run_cmd;
 //! run_cmd!(FOO=100 /tmp/test_run_cmd_lib.sh)?;
@@ -364,10 +363,21 @@
 //!
 //! ### Thread Safety
 //!
-//! This library tries very hard to not set global states, so parallel `cargo test` can be executed just fine.
-//! The only known APIs not supported in multi-thread environment are the
-//! [`tls_init!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_init.html)/[`tls_get!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_get.html)/[`tls_set!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_set.html) macros, and you should only use them for *thread local* variables.
-//!
+//! This library tries very hard to not set global state, so parallel `cargo test` can be executed just fine.
+//! That said, there are some limitations to be aware of:
+//!
+//! - [std::env::set_var] and [std::env::remove_var] **[must not be called]** in a multi-threaded program
+//! - [`tls_init!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_init.html),
+//!   [`tls_get!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_get.html), and
+//!   [`tls_set!`](https://docs.rs/cmd_lib/latest/cmd_lib/macro.tls_set.html) create *thread-local* variables, which
+//!   means each thread will have its own independent version of the variable
+//! - [`set_debug`](https://docs.rs/cmd_lib/latest/cmd_lib/fn.set_debug.html) and
+//!   [`set_pipefail`](https://docs.rs/cmd_lib/latest/cmd_lib/fn.set_pipefail.html) are *global* and affect all threads;
+//!   there is currently no way to change those settings without affecting other threads
+//!
+//! [std::env::set_var]: https://doc.rust-lang.org/std/env/fn.set_var.html
+//! [std::env::remove_var]: https://doc.rust-lang.org/std/env/fn.remove_var.html
+//! [must not be called]: https://doc.rust-lang.org/nightly/edition-guide/rust-2024/newly-unsafe-functions.html#stdenvset_var-remove_var
 
 pub use cmd_lib_macros::{
     cmd_die, main, run_cmd, run_fun, spawn, spawn_with_output, use_custom_cmd,
diff --git a/src/process.rs b/src/process.rs
@@ -14,7 +14,9 @@ use std::io::{Error, ErrorKind, Result};
 use std::mem::take;
 use std::path::{Path, PathBuf};
 use std::process::Command;
-use std::sync::Mutex;
+use std::sync::atomic::AtomicBool;
+use std::sync::atomic::Ordering::SeqCst;
+use std::sync::{LazyLock, Mutex};
 use std::thread;
 
 const CD_CMD: &str = "cd";
@@ -89,26 +91,38 @@ pub fn register_cmd(cmd: &'static str, func: FnFun) {
     CMD_MAP.lock().unwrap().insert(OsString::from(cmd), func);
 }
 
+static DEBUG_ENABLED: LazyLock<AtomicBool> =
+    LazyLock::new(|| AtomicBool::new(std::env::var("CMD_LIB_DEBUG") == Ok("1".into())));
+
+static PIPEFAIL_ENABLED: LazyLock<AtomicBool> =
+    LazyLock::new(|| AtomicBool::new(std::env::var("CMD_LIB_PIPEFAIL") != Ok("0".into())));
+
 /// Set debug mode or not, false by default.
 ///
-/// Setting environment variable CMD_LIB_DEBUG=0|1 has the same effect
+/// This is **global**, and affects all threads.
+///
+/// Setting environment variable CMD_LIB_DEBUG=0|1 has the same effect, but the environment variable is only
+/// checked once at an unspecified time, so the only reliable way to do that is when the program is first started.
 pub fn set_debug(enable: bool) {
-    std::env::set_var("CMD_LIB_DEBUG", if enable { "1" } else { "0" });
+    DEBUG_ENABLED.store(enable, SeqCst);
 }
 
 /// Set pipefail or not, true by default.
 ///
-/// Setting environment variable CMD_LIB_PIPEFAIL=0|1 has the same effect
+/// This is **global**, and affects all threads.
+///
+/// Setting environment variable CMD_LIB_DEBUG=0|1 has the same effect, but the environment variable is only
+/// checked once at an unspecified time, so the only reliable way to do that is when the program is first started.
 pub fn set_pipefail(enable: bool) {
-    std::env::set_var("CMD_LIB_PIPEFAIL", if enable { "1" } else { "0" });
+    PIPEFAIL_ENABLED.store(enable, SeqCst);
 }
 
 pub(crate) fn debug_enabled() -> bool {
-    std::env::var("CMD_LIB_DEBUG") == Ok("1".into())
+    DEBUG_ENABLED.load(SeqCst)
 }
 
 pub(crate) fn pipefail_enabled() -> bool {
-    std::env::var("CMD_LIB_PIPEFAIL") != Ok("0".into())
+    PIPEFAIL_ENABLED.load(SeqCst)
 }
 
 #[doc(hidden)]
