From bfb8479d83a923258b8402e0492b767d354494bf Mon Sep 17 00:00:00 2001 From: Matyas Susits Date: Fri, 12 Sep 2025 12:58:16 +0200 Subject: [PATCH 1/6] Abstract away an assertions details This will make it easier to change the implementation in the future. --- src/cargo/core/registry.rs | 8 +++----- src/cargo/util/context/mod.rs | 11 +++++++++++ src/cargo/util/flock.rs | 7 +++---- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index d116c1e91fe..9bc4d82025c 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -800,11 +800,9 @@ impl<'gctx> Registry for PackageRegistry<'gctx> { #[tracing::instrument(skip_all)] fn block_until_ready(&mut self) -> CargoResult<()> { - if cfg!(debug_assertions) { - // Force borrow to catch invalid borrows, regardless of which source is used and how it - // happens to behave this time - self.gctx.shell().verbosity(); - } + // Ensure `shell` is not already in use, + // regardless of which source is used and how it happens to behave this time + self.gctx.debug_assert_shell_not_borrowed(); for (source_id, source) in self.sources.sources_mut() { source .block_until_ready() diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 62b04113f4b..1a03e4b5829 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -412,6 +412,17 @@ impl GlobalContext { self.shell.borrow_mut() } + /// Assert [`Self::shell`] is not in use + /// + /// Testing might not identify bugs with two accesses to `shell` at once + /// due to conditional logic, + /// so place this outside of the conditions to catch these bugs in more situations. + pub fn debug_assert_shell_not_borrowed(&self) { + if cfg!(debug_assertions) { + self.shell().verbosity(); + } + } + /// Gets the path to the `rustdoc` executable. pub fn rustdoc(&self) -> CargoResult<&Path> { self.rustdoc diff --git a/src/cargo/util/flock.rs b/src/cargo/util/flock.rs index 9c929de995a..e82465e89a1 100644 --- a/src/cargo/util/flock.rs +++ b/src/cargo/util/flock.rs @@ -389,10 +389,9 @@ fn acquire( lock_try: &dyn Fn() -> Result<(), TryLockError>, lock_block: &dyn Fn() -> io::Result<()>, ) -> CargoResult<()> { - if cfg!(debug_assertions) { - // Force borrow to catch invalid borrows outside of contention situations - gctx.shell().verbosity(); - } + // Ensure `shell` is not already in use, + // regardless of whether we hit contention or not + gctx.debug_assert_shell_not_borrowed(); if try_acquire(path, lock_try)? { return Ok(()); } From 742a111df368f856fe18369321ba85e6b9cdf443 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 22 Sep 2025 10:13:23 -0500 Subject: [PATCH 2/6] Abstract away ws_roots --- src/cargo/core/workspace.rs | 2 +- src/cargo/util/context/mod.rs | 4 ++++ src/cargo/util/toml/mod.rs | 9 +++------ 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 1db728d81ca..096b333d184 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -2037,7 +2037,7 @@ fn find_workspace_root_with_loader( ) -> CargoResult> { // Check if there are any workspace roots that have already been found that would work { - let roots = gctx.ws_roots.borrow(); + let roots = gctx.ws_roots(); // Iterate through the manifests parent directories until we find a workspace // root. Note we skip the first item since that is just the path itself for current in manifest_path.ancestors().skip(1) { diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 1a03e4b5829..2152be49653 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -2133,6 +2133,10 @@ impl GlobalContext { Ok(WarningHandling::default()) } } + + pub fn ws_roots(&self) -> RefMut<'_, HashMap> { + self.ws_roots.borrow_mut() + } } /// Internal error for serde errors. diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index d0a7ced2eef..cf20471ca6c 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -81,8 +81,7 @@ pub fn read_manifest( to_workspace_config(&original_toml, path, is_embedded, gctx, &mut warnings)?; if let WorkspaceConfig::Root(ws_root_config) = &workspace_config { let package_root = path.parent().unwrap(); - gctx.ws_roots - .borrow_mut() + gctx.ws_roots() .insert(package_root.to_owned(), ws_root_config.clone()); } let normalized_toml = normalize_toml( @@ -996,7 +995,7 @@ fn inheritable_from_path( // Let the borrow exit scope so that it can be picked up if there is a need to // read a manifest - if let Some(ws_root) = gctx.ws_roots.borrow().get(workspace_path_root) { + if let Some(ws_root) = gctx.ws_roots().get(workspace_path_root) { return Ok(ws_root.inheritable().clone()); }; @@ -1004,9 +1003,7 @@ fn inheritable_from_path( let man = read_manifest(&workspace_path, source_id, gctx)?; match man.workspace_config() { WorkspaceConfig::Root(root) => { - gctx.ws_roots - .borrow_mut() - .insert(workspace_path, root.clone()); + gctx.ws_roots().insert(workspace_path, root.clone()); Ok(root.inheritable().clone()) } _ => bail!( From c7a1dde3fdfc63bc2b46cf70381137e104f97ff1 Mon Sep 17 00:00:00 2001 From: Matyas Susits Date: Tue, 16 Sep 2025 20:15:53 +0200 Subject: [PATCH 3/6] port from lazycell --- src/cargo/util/context/mod.rs | 94 +++++++++++++++++------------------ src/cargo/util/mod.rs | 2 + src/cargo/util/once.rs | 90 +++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 48 deletions(-) create mode 100644 src/cargo/util/once.rs diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 2152be49653..b0eeb38630e 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -51,6 +51,7 @@ use crate::util::cache_lock::{CacheLock, CacheLockMode, CacheLocker}; use std::borrow::Cow; +use std::cell::OnceCell; use std::cell::{RefCell, RefMut}; use std::collections::hash_map::Entry::{Occupied, Vacant}; use std::collections::{HashMap, HashSet}; @@ -74,6 +75,7 @@ use crate::core::{CliUnstable, Shell, SourceId, Workspace, WorkspaceRootConfig, use crate::ops::RegistryCredentialConfig; use crate::sources::CRATES_IO_INDEX; use crate::sources::CRATES_IO_REGISTRY; +use crate::util::OnceExt as _; use crate::util::errors::CargoResult; use crate::util::network::http::configure_http_handle; use crate::util::network::http::http_handle; @@ -85,7 +87,6 @@ use cargo_util::paths; use cargo_util_schemas::manifest::RegistryName; use curl::easy::Easy; use itertools::Itertools; -use lazycell::LazyCell; use serde::Deserialize; use serde::de::IntoDeserializer as _; use serde_untagged::UntaggedEnumVisitor; @@ -168,9 +169,9 @@ pub struct GlobalContext { /// Information about how to write messages to the shell shell: RefCell, /// A collection of configuration options - values: LazyCell>, + values: OnceCell>, /// A collection of configuration options from the credentials file - credential_values: LazyCell>, + credential_values: OnceCell>, /// CLI config values, passed in via `configure`. cli_config: Option>, /// The current working directory of cargo @@ -178,9 +179,9 @@ pub struct GlobalContext { /// Directory where config file searching should stop (inclusive). search_stop_path: Option, /// The location of the cargo executable (path to current process) - cargo_exe: LazyCell, + cargo_exe: OnceCell, /// The location of the rustdoc executable - rustdoc: LazyCell, + rustdoc: OnceCell, /// Whether we are printing extra verbose messages extra_verbose: bool, /// `frozen` is the same as `locked`, but additionally will not access the @@ -199,9 +200,9 @@ pub struct GlobalContext { /// Cli flags of the form "-Z something" unstable_flags_cli: Option>, /// A handle on curl easy mode for http calls - easy: LazyCell>, + easy: OnceCell>, /// Cache of the `SourceId` for crates.io - crates_io_source_id: LazyCell, + crates_io_source_id: OnceCell, /// If false, don't cache `rustc --version --verbose` invocations cache_rustc_info: bool, /// Creation time of this config, used to output the total build time @@ -211,23 +212,23 @@ pub struct GlobalContext { /// Environment variable snapshot. env: Env, /// Tracks which sources have been updated to avoid multiple updates. - updated_sources: LazyCell>>, + updated_sources: OnceCell>>, /// Cache of credentials from configuration or credential providers. /// Maps from url to credential value. - credential_cache: LazyCell>>, + credential_cache: OnceCell>>, /// Cache of registry config from the `[registries]` table. - registry_config: LazyCell>>>, + registry_config: OnceCell>>>, /// Locks on the package and index caches. package_cache_lock: CacheLocker, /// Cached configuration parsed by Cargo - http_config: LazyCell, - future_incompat_config: LazyCell, - net_config: LazyCell, - build_config: LazyCell, - target_cfgs: LazyCell>, - doc_extern_map: LazyCell, + http_config: OnceCell, + future_incompat_config: OnceCell, + net_config: OnceCell, + build_config: OnceCell, + target_cfgs: OnceCell>, + doc_extern_map: OnceCell, progress_config: ProgressConfig, - env_config: LazyCell>>, + env_config: OnceCell>>, /// This should be false if: /// - this is an artifact of the rustc distribution process for "stable" or for "beta" /// - this is an `#[test]` that does not opt in with `enable_nightly_features` @@ -247,10 +248,10 @@ pub struct GlobalContext { /// `WorkspaceRootConfigs` that have been found pub ws_roots: RefCell>, /// The global cache tracker is a database used to track disk cache usage. - global_cache_tracker: LazyCell>, + global_cache_tracker: OnceCell>, /// A cache of modifications to make to [`GlobalContext::global_cache_tracker`], /// saved to disk in a batch to improve performance. - deferred_global_last_use: LazyCell>, + deferred_global_last_use: OnceCell>, } impl GlobalContext { @@ -286,11 +287,11 @@ impl GlobalContext { shell: RefCell::new(shell), cwd, search_stop_path: None, - values: LazyCell::new(), - credential_values: LazyCell::new(), + values: OnceCell::new(), + credential_values: OnceCell::new(), cli_config: None, - cargo_exe: LazyCell::new(), - rustdoc: LazyCell::new(), + cargo_exe: OnceCell::new(), + rustdoc: OnceCell::new(), extra_verbose: false, frozen: false, locked: false, @@ -304,28 +305,28 @@ impl GlobalContext { }, unstable_flags: CliUnstable::default(), unstable_flags_cli: None, - easy: LazyCell::new(), - crates_io_source_id: LazyCell::new(), + easy: OnceCell::new(), + crates_io_source_id: OnceCell::new(), cache_rustc_info, creation_time: Instant::now(), target_dir: None, env, - updated_sources: LazyCell::new(), - credential_cache: LazyCell::new(), - registry_config: LazyCell::new(), + updated_sources: OnceCell::new(), + credential_cache: OnceCell::new(), + registry_config: OnceCell::new(), package_cache_lock: CacheLocker::new(), - http_config: LazyCell::new(), - future_incompat_config: LazyCell::new(), - net_config: LazyCell::new(), - build_config: LazyCell::new(), - target_cfgs: LazyCell::new(), - doc_extern_map: LazyCell::new(), + http_config: OnceCell::new(), + future_incompat_config: OnceCell::new(), + net_config: OnceCell::new(), + build_config: OnceCell::new(), + target_cfgs: OnceCell::new(), + doc_extern_map: OnceCell::new(), progress_config: ProgressConfig::default(), - env_config: LazyCell::new(), + env_config: OnceCell::new(), nightly_features_allowed: matches!(&*features::channel(), "nightly" | "dev"), ws_roots: RefCell::new(HashMap::new()), - global_cache_tracker: LazyCell::new(), - deferred_global_last_use: LazyCell::new(), + global_cache_tracker: OnceCell::new(), + deferred_global_last_use: OnceCell::new(), } } @@ -526,21 +527,21 @@ impl GlobalContext { /// Which package sources have been updated, used to ensure it is only done once. pub fn updated_sources(&self) -> RefMut<'_, HashSet> { self.updated_sources - .borrow_with(|| RefCell::new(HashSet::new())) + .get_or_init(|| RefCell::new(HashSet::new())) .borrow_mut() } /// Cached credentials from credential providers or configuration. pub fn credential_cache(&self) -> RefMut<'_, HashMap> { self.credential_cache - .borrow_with(|| RefCell::new(HashMap::new())) + .get_or_init(|| RefCell::new(HashMap::new())) .borrow_mut() } /// Cache of already parsed registries from the `[registries]` table. pub(crate) fn registry_config(&self) -> RefMut<'_, HashMap>> { self.registry_config - .borrow_with(|| RefCell::new(HashMap::new())) + .get_or_init(|| RefCell::new(HashMap::new())) .borrow_mut() } @@ -561,18 +562,15 @@ impl GlobalContext { /// using this if possible. pub fn values_mut(&mut self) -> CargoResult<&mut HashMap> { let _ = self.values()?; - Ok(self - .values - .borrow_mut() - .expect("already loaded config values")) + Ok(self.values.get_mut().expect("already loaded config values")) } // Note: this is used by RLS, not Cargo. pub fn set_values(&self, values: HashMap) -> CargoResult<()> { - if self.values.borrow().is_some() { + if self.values.get().is_some() { bail!("config values already found") } - match self.values.fill(values) { + match self.values.set(values) { Ok(()) => Ok(()), Err(_) => bail!("could not fill values"), } @@ -741,7 +739,7 @@ impl GlobalContext { /// This does NOT look at environment variables. See `get_cv_with_env` for /// a variant that supports environment variables. fn get_cv(&self, key: &ConfigKey) -> CargoResult> { - if let Some(vals) = self.credential_values.borrow() { + if let Some(vals) = self.credential_values.get() { let val = self.get_cv_helper(key, vals)?; if val.is_some() { return Ok(val); @@ -1802,7 +1800,7 @@ impl GlobalContext { } } self.credential_values - .fill(credential_values) + .set(credential_values) .expect("was not filled at beginning of the function"); Ok(()) } diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index a61fb5b9651..62f181f42f6 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -18,6 +18,7 @@ pub use self::into_url::IntoUrl; pub use self::into_url_with_base::IntoUrlWithBase; pub(crate) use self::io::LimitErrorReader; pub use self::lockserver::{LockServer, LockServerClient, LockServerStarted}; +pub use self::once::OnceExt; pub use self::progress::{Progress, ProgressStyle}; pub use self::queue::Queue; pub use self::rustc::Rustc; @@ -56,6 +57,7 @@ pub mod lints; mod lockserver; pub mod machine_message; pub mod network; +mod once; mod progress; mod queue; pub mod restricted_names; diff --git a/src/cargo/util/once.rs b/src/cargo/util/once.rs new file mode 100644 index 00000000000..63f0647e36a --- /dev/null +++ b/src/cargo/util/once.rs @@ -0,0 +1,90 @@ +//! Extension functions for [`std::sync::OnceLock`] / [`std::cell::OnceCell`] +//! +//! This adds polyfills for functionality in `lazycell` that is not stable within `std`. + +pub trait OnceExt { + type T; + + /// This might run `f` multiple times if different threads start initializing at once. + fn try_borrow_with(&self, f: F) -> Result<&Self::T, E> + where + F: FnOnce() -> Result; + + fn replace(&mut self, new_value: Self::T) -> Option; + + fn filled(&self) -> bool; +} + +impl OnceExt for std::sync::OnceLock { + type T = T; + + fn try_borrow_with(&self, f: F) -> Result<&T, E> + where + F: FnOnce() -> Result, + { + if let Some(value) = self.get() { + return Ok(value); + } + + // This is not how the unstable `OnceLock::get_or_try_init` works. That only starts `f` if + // no other `f` is executing and the value is not initialized. However, correctly implementing that is + // hard (one has properly handle panics in `f`) and not doable with the stable API of `OnceLock`. + let value = f()?; + // Another thread might have initialized `self` since we checked that `self.get()` returns `None`. If this is the case, `self.set()` + // returns an error. We ignore it and return the value set by the other + // thread. + let _ = self.set(value); + Ok(self.get().unwrap()) + } + + fn replace(&mut self, new_value: T) -> Option { + if let Some(value) = self.get_mut() { + Some(std::mem::replace(value, new_value)) + } else { + let result = self.set(new_value); + assert!(result.is_ok()); + None + } + } + + fn filled(&self) -> bool { + self.get().is_some() + } +} + +impl OnceExt for std::cell::OnceCell { + type T = T; + + fn try_borrow_with(&self, f: F) -> Result<&T, E> + where + F: FnOnce() -> Result, + { + if let Some(value) = self.get() { + return Ok(value); + } + + // This is not how the unstable `OnceLock::get_or_try_init` works. That only starts `f` if + // no other `f` is executing and the value is not initialized. However, correctly implementing that is + // hard (one has properly handle panics in `f`) and not doable with the stable API of `OnceLock`. + let value = f()?; + // Another thread might have initialized `self` since we checked that `self.get()` returns `None`. If this is the case, `self.set()` + // returns an error. We ignore it and return the value set by the other + // thread. + let _ = self.set(value); + Ok(self.get().unwrap()) + } + + fn replace(&mut self, new_value: T) -> Option { + if let Some(value) = self.get_mut() { + Some(std::mem::replace(value, new_value)) + } else { + let result = self.set(new_value); + assert!(result.is_ok()); + None + } + } + + fn filled(&self) -> bool { + self.get().is_some() + } +} From a8d9b7d9ae24d39ea279cc0f2dbff6ee854e5c7c Mon Sep 17 00:00:00 2001 From: Matyas Susits Date: Tue, 16 Sep 2025 20:15:53 +0200 Subject: [PATCH 4/6] make GlobalContext Sync --- src/cargo/core/shell.rs | 4 +- src/cargo/sources/git/utils.rs | 2 +- src/cargo/util/cache_lock.rs | 21 +++-- src/cargo/util/context/mod.rs | 147 +++++++++++++++++---------------- 4 files changed, 94 insertions(+), 80 deletions(-) diff --git a/src/cargo/core/shell.rs b/src/cargo/core/shell.rs index 8ac6e6fb8db..ed1bbf76c78 100644 --- a/src/cargo/core/shell.rs +++ b/src/cargo/core/shell.rs @@ -65,7 +65,7 @@ impl Shell { } /// Creates a shell from a plain writable object, with no color, and max verbosity. - pub fn from_write(out: Box) -> Shell { + pub fn from_write(out: Box) -> Shell { Shell { output: ShellOut::Write(AutoStream::never(out)), // strip all formatting on write verbosity: Verbosity::Verbose, @@ -432,7 +432,7 @@ impl Default for Shell { /// A `Write`able object, either with or without color support enum ShellOut { /// A plain write object without color support - Write(AutoStream>), + Write(AutoStream>), /// Color-enabled stdio, with information on whether color should be used Stream { stdout: AutoStream, diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index e0346aa2e84..e9224df22dc 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -1528,7 +1528,7 @@ fn github_fast_path( "https://api.github.com/repos/{}/{}/commits/{}", username, repository, github_branch_name, ); - let mut handle = gctx.http()?.borrow_mut(); + let mut handle = gctx.http()?.lock().unwrap(); debug!("attempting GitHub fast path for {}", url); handle.get(true)?; handle.url(&url)?; diff --git a/src/cargo/util/cache_lock.rs b/src/cargo/util/cache_lock.rs index afd06c998b8..068e1bb4bcd 100644 --- a/src/cargo/util/cache_lock.rs +++ b/src/cargo/util/cache_lock.rs @@ -91,8 +91,8 @@ use super::FileLock; use crate::CargoResult; use crate::GlobalContext; use anyhow::Context as _; -use std::cell::RefCell; use std::io; +use std::sync::Mutex; /// The style of lock to acquire. #[derive(Copy, Clone, Debug, PartialEq)] @@ -435,7 +435,11 @@ pub struct CacheLock<'lock> { impl Drop for CacheLock<'_> { fn drop(&mut self) { use CacheLockMode::*; - let mut state = self.locker.state.borrow_mut(); + let mut state = match self.locker.state.lock() { + Ok(result) => result, + // we should release the cache even if a thread panicked while holding a lock + Err(poison) => poison.into_inner(), + }; match self.mode { Shared => { state.mutate_lock.decrement(); @@ -472,24 +476,25 @@ pub struct CacheLocker { /// /// [`CacheLocker`] uses interior mutability because it is stuffed inside /// [`GlobalContext`], which does not allow mutation. - state: RefCell, + state: Mutex, } impl CacheLocker { /// Creates a new `CacheLocker`. pub fn new() -> CacheLocker { CacheLocker { - state: RefCell::new(CacheState { + state: CacheState { cache_lock: RecursiveLock::new(CACHE_LOCK_NAME), mutate_lock: RecursiveLock::new(MUTATE_NAME), - }), + } + .into(), } } /// Acquires a lock with the given mode, possibly blocking if another /// cargo is holding the lock. pub fn lock(&self, gctx: &GlobalContext, mode: CacheLockMode) -> CargoResult> { - let mut state = self.state.borrow_mut(); + let mut state = self.state.lock().unwrap(); let _ = state.lock(gctx, mode, Blocking)?; Ok(CacheLock { mode, locker: self }) } @@ -501,7 +506,7 @@ impl CacheLocker { gctx: &GlobalContext, mode: CacheLockMode, ) -> CargoResult>> { - let mut state = self.state.borrow_mut(); + let mut state = self.state.lock().unwrap(); if state.lock(gctx, mode, NonBlocking)? == LockAcquired { Ok(Some(CacheLock { mode, locker: self })) } else { @@ -519,7 +524,7 @@ impl CacheLocker { /// `DownloadExclusive` will return true if a `MutateExclusive` lock is /// held since they overlap. pub fn is_locked(&self, mode: CacheLockMode) -> bool { - let state = self.state.borrow(); + let state = self.state.lock().unwrap(); match ( mode, state.cache_lock.count, diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index b0eeb38630e..a1479c8c2c0 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -51,8 +51,6 @@ use crate::util::cache_lock::{CacheLock, CacheLockMode, CacheLocker}; use std::borrow::Cow; -use std::cell::OnceCell; -use std::cell::{RefCell, RefMut}; use std::collections::hash_map::Entry::{Occupied, Vacant}; use std::collections::{HashMap, HashSet}; use std::env; @@ -64,7 +62,7 @@ use std::io::prelude::*; use std::mem; use std::path::{Path, PathBuf}; use std::str::FromStr; -use std::sync::{Arc, Once}; +use std::sync::{Arc, Mutex, MutexGuard, Once, OnceLock}; use std::time::Instant; use self::ConfigValue as CV; @@ -167,11 +165,11 @@ pub struct GlobalContext { /// The location of the user's Cargo home directory. OS-dependent. home_path: Filesystem, /// Information about how to write messages to the shell - shell: RefCell, + shell: Mutex, /// A collection of configuration options - values: OnceCell>, + values: OnceLock>, /// A collection of configuration options from the credentials file - credential_values: OnceCell>, + credential_values: OnceLock>, /// CLI config values, passed in via `configure`. cli_config: Option>, /// The current working directory of cargo @@ -179,9 +177,9 @@ pub struct GlobalContext { /// Directory where config file searching should stop (inclusive). search_stop_path: Option, /// The location of the cargo executable (path to current process) - cargo_exe: OnceCell, + cargo_exe: OnceLock, /// The location of the rustdoc executable - rustdoc: OnceCell, + rustdoc: OnceLock, /// Whether we are printing extra verbose messages extra_verbose: bool, /// `frozen` is the same as `locked`, but additionally will not access the @@ -200,9 +198,9 @@ pub struct GlobalContext { /// Cli flags of the form "-Z something" unstable_flags_cli: Option>, /// A handle on curl easy mode for http calls - easy: OnceCell>, + easy: OnceLock>, /// Cache of the `SourceId` for crates.io - crates_io_source_id: OnceCell, + crates_io_source_id: OnceLock, /// If false, don't cache `rustc --version --verbose` invocations cache_rustc_info: bool, /// Creation time of this config, used to output the total build time @@ -212,23 +210,23 @@ pub struct GlobalContext { /// Environment variable snapshot. env: Env, /// Tracks which sources have been updated to avoid multiple updates. - updated_sources: OnceCell>>, + updated_sources: OnceLock>>, /// Cache of credentials from configuration or credential providers. /// Maps from url to credential value. - credential_cache: OnceCell>>, + credential_cache: OnceLock>>, /// Cache of registry config from the `[registries]` table. - registry_config: OnceCell>>>, + registry_config: OnceLock>>>, /// Locks on the package and index caches. package_cache_lock: CacheLocker, /// Cached configuration parsed by Cargo - http_config: OnceCell, - future_incompat_config: OnceCell, - net_config: OnceCell, - build_config: OnceCell, - target_cfgs: OnceCell>, - doc_extern_map: OnceCell, + http_config: OnceLock, + future_incompat_config: OnceLock, + net_config: OnceLock, + build_config: OnceLock, + target_cfgs: OnceLock>, + doc_extern_map: OnceLock, progress_config: ProgressConfig, - env_config: OnceCell>>, + env_config: OnceLock>>, /// This should be false if: /// - this is an artifact of the rustc distribution process for "stable" or for "beta" /// - this is an `#[test]` that does not opt in with `enable_nightly_features` @@ -246,12 +244,12 @@ pub struct GlobalContext { /// consider using `ConfigBuilder::enable_nightly_features` instead. pub nightly_features_allowed: bool, /// `WorkspaceRootConfigs` that have been found - pub ws_roots: RefCell>, + ws_roots: Mutex>, /// The global cache tracker is a database used to track disk cache usage. - global_cache_tracker: OnceCell>, + global_cache_tracker: OnceLock>, /// A cache of modifications to make to [`GlobalContext::global_cache_tracker`], /// saved to disk in a batch to improve performance. - deferred_global_last_use: OnceCell>, + deferred_global_last_use: OnceLock>, } impl GlobalContext { @@ -284,14 +282,14 @@ impl GlobalContext { GlobalContext { home_path: Filesystem::new(homedir), - shell: RefCell::new(shell), + shell: Mutex::new(shell), cwd, search_stop_path: None, - values: OnceCell::new(), - credential_values: OnceCell::new(), + values: Default::default(), + credential_values: Default::default(), cli_config: None, - cargo_exe: OnceCell::new(), - rustdoc: OnceCell::new(), + cargo_exe: Default::default(), + rustdoc: Default::default(), extra_verbose: false, frozen: false, locked: false, @@ -305,28 +303,28 @@ impl GlobalContext { }, unstable_flags: CliUnstable::default(), unstable_flags_cli: None, - easy: OnceCell::new(), - crates_io_source_id: OnceCell::new(), + easy: Default::default(), + crates_io_source_id: Default::default(), cache_rustc_info, creation_time: Instant::now(), target_dir: None, env, - updated_sources: OnceCell::new(), - credential_cache: OnceCell::new(), - registry_config: OnceCell::new(), + updated_sources: Default::default(), + credential_cache: Default::default(), + registry_config: Default::default(), package_cache_lock: CacheLocker::new(), - http_config: OnceCell::new(), - future_incompat_config: OnceCell::new(), - net_config: OnceCell::new(), - build_config: OnceCell::new(), - target_cfgs: OnceCell::new(), - doc_extern_map: OnceCell::new(), + http_config: Default::default(), + future_incompat_config: Default::default(), + net_config: Default::default(), + build_config: Default::default(), + target_cfgs: Default::default(), + doc_extern_map: Default::default(), progress_config: ProgressConfig::default(), - env_config: OnceCell::new(), + env_config: Default::default(), nightly_features_allowed: matches!(&*features::channel(), "nightly" | "dev"), - ws_roots: RefCell::new(HashMap::new()), - global_cache_tracker: OnceCell::new(), - deferred_global_last_use: OnceCell::new(), + ws_roots: Default::default(), + global_cache_tracker: Default::default(), + deferred_global_last_use: Default::default(), } } @@ -409,8 +407,8 @@ impl GlobalContext { } /// Gets a reference to the shell, e.g., for writing error messages. - pub fn shell(&self) -> RefMut<'_, Shell> { - self.shell.borrow_mut() + pub fn shell(&self) -> MutexGuard<'_, Shell> { + self.shell.lock().unwrap() } /// Assert [`Self::shell`] is not in use @@ -525,24 +523,29 @@ impl GlobalContext { } /// Which package sources have been updated, used to ensure it is only done once. - pub fn updated_sources(&self) -> RefMut<'_, HashSet> { + pub fn updated_sources(&self) -> MutexGuard<'_, HashSet> { self.updated_sources - .get_or_init(|| RefCell::new(HashSet::new())) - .borrow_mut() + .get_or_init(|| Default::default()) + .lock() + .unwrap() } /// Cached credentials from credential providers or configuration. - pub fn credential_cache(&self) -> RefMut<'_, HashMap> { + pub fn credential_cache(&self) -> MutexGuard<'_, HashMap> { self.credential_cache - .get_or_init(|| RefCell::new(HashMap::new())) - .borrow_mut() + .get_or_init(|| Default::default()) + .lock() + .unwrap() } /// Cache of already parsed registries from the `[registries]` table. - pub(crate) fn registry_config(&self) -> RefMut<'_, HashMap>> { + pub(crate) fn registry_config( + &self, + ) -> MutexGuard<'_, HashMap>> { self.registry_config - .get_or_init(|| RefCell::new(HashMap::new())) - .borrow_mut() + .get_or_init(|| Default::default()) + .lock() + .unwrap() } /// Gets all config values from disk. @@ -570,7 +573,7 @@ impl GlobalContext { if self.values.get().is_some() { bail!("config values already found") } - match self.values.set(values) { + match self.values.set(values.into()) { Ok(()) => Ok(()), Err(_) => bail!("could not fill values"), } @@ -745,7 +748,7 @@ impl GlobalContext { return Ok(val); } } - self.get_cv_helper(key, self.values()?) + self.get_cv_helper(key, &*self.values()?) } fn get_cv_helper( @@ -1892,12 +1895,12 @@ impl GlobalContext { self.jobserver.as_ref() } - pub fn http(&self) -> CargoResult<&RefCell> { + pub fn http(&self) -> CargoResult<&Mutex> { let http = self .easy - .try_borrow_with(|| http_handle(self).map(RefCell::new))?; + .try_borrow_with(|| http_handle(self).map(Into::into))?; { - let mut http = http.borrow_mut(); + let mut http = http.lock().unwrap(); http.reset(); let timeout = configure_http_handle(self, &mut http)?; timeout.configure(&mut http)?; @@ -2108,19 +2111,19 @@ impl GlobalContext { /// /// The package cache lock must be held to call this function (and to use /// it in general). - pub fn global_cache_tracker(&self) -> CargoResult> { + pub fn global_cache_tracker(&self) -> CargoResult> { let tracker = self.global_cache_tracker.try_borrow_with(|| { - Ok::<_, anyhow::Error>(RefCell::new(GlobalCacheTracker::new(self)?)) + Ok::<_, anyhow::Error>(Mutex::new(GlobalCacheTracker::new(self)?)) })?; - Ok(tracker.borrow_mut()) + Ok(tracker.lock().unwrap()) } /// Returns a reference to the shared [`DeferredGlobalLastUse`]. - pub fn deferred_global_last_use(&self) -> CargoResult> { - let deferred = self.deferred_global_last_use.try_borrow_with(|| { - Ok::<_, anyhow::Error>(RefCell::new(DeferredGlobalLastUse::new())) - })?; - Ok(deferred.borrow_mut()) + pub fn deferred_global_last_use(&self) -> CargoResult> { + let deferred = self + .deferred_global_last_use + .try_borrow_with(|| Ok::<_, anyhow::Error>(Mutex::new(DeferredGlobalLastUse::new())))?; + Ok(deferred.lock().unwrap()) } /// Get the global [`WarningHandling`] configuration. @@ -2132,8 +2135,8 @@ impl GlobalContext { } } - pub fn ws_roots(&self) -> RefMut<'_, HashMap> { - self.ws_roots.borrow_mut() + pub fn ws_roots(&self) -> MutexGuard<'_, HashMap> { + self.ws_roots.lock().unwrap() } } @@ -3205,4 +3208,10 @@ mod tests { assert_eq!(http.multiplexing, result); } } + + #[test] + fn sync_context() { + fn assert_sync() {} + assert_sync::(); + } } From 3fd0af4da5869a3874535c00fe2fb43a2e293c62 Mon Sep 17 00:00:00 2001 From: Matyas Susits Date: Sat, 13 Sep 2025 19:32:03 +0200 Subject: [PATCH 5/6] remove OnceLock from GlobalContext fields where the lazy loaded value is just a HashMap::defualt() --- src/cargo/util/context/mod.rs | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index a1479c8c2c0..c9f069b739f 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -210,12 +210,12 @@ pub struct GlobalContext { /// Environment variable snapshot. env: Env, /// Tracks which sources have been updated to avoid multiple updates. - updated_sources: OnceLock>>, + updated_sources: Mutex>, /// Cache of credentials from configuration or credential providers. /// Maps from url to credential value. - credential_cache: OnceLock>>, + credential_cache: Mutex>, /// Cache of registry config from the `[registries]` table. - registry_config: OnceLock>>>, + registry_config: Mutex>>, /// Locks on the package and index caches. package_cache_lock: CacheLocker, /// Cached configuration parsed by Cargo @@ -524,28 +524,19 @@ impl GlobalContext { /// Which package sources have been updated, used to ensure it is only done once. pub fn updated_sources(&self) -> MutexGuard<'_, HashSet> { - self.updated_sources - .get_or_init(|| Default::default()) - .lock() - .unwrap() + self.updated_sources.lock().unwrap() } /// Cached credentials from credential providers or configuration. pub fn credential_cache(&self) -> MutexGuard<'_, HashMap> { - self.credential_cache - .get_or_init(|| Default::default()) - .lock() - .unwrap() + self.credential_cache.lock().unwrap() } /// Cache of already parsed registries from the `[registries]` table. pub(crate) fn registry_config( &self, ) -> MutexGuard<'_, HashMap>> { - self.registry_config - .get_or_init(|| Default::default()) - .lock() - .unwrap() + self.registry_config.lock().unwrap() } /// Gets all config values from disk. From 9929c8796da2493f531c805e6e4007c7ff1d66c6 Mon Sep 17 00:00:00 2001 From: Matyas Susits Date: Fri, 12 Sep 2025 12:58:16 +0200 Subject: [PATCH 6/6] Assert, rather than hang, on shell-in-use assertions Previously, `gctx.shell().verbosity()` was used to check that `gctx.shell` is not borrowed. Since shell is now behind a `Mutex` and not a `RefCell`, this would hang waiting for the unlock instead panicking. Borrow state checking is now done using `Mutex::try_lock` in `GlobalContext::debug_assert_shell_not_borrowed` --- src/cargo/util/context/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index c9f069b739f..01ba9909aec 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -418,7 +418,10 @@ impl GlobalContext { /// so place this outside of the conditions to catch these bugs in more situations. pub fn debug_assert_shell_not_borrowed(&self) { if cfg!(debug_assertions) { - self.shell().verbosity(); + match self.shell.try_lock() { + Ok(_) | Err(std::sync::TryLockError::Poisoned(_)) => (), + Err(std::sync::TryLockError::WouldBlock) => panic!("shell is borrowed!"), + } } }