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/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/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/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 62b04113f4b..01ba9909aec 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -51,7 +51,6 @@ use crate::util::cache_lock::{CacheLock, CacheLockMode, CacheLocker}; use std::borrow::Cow; -use std::cell::{RefCell, RefMut}; use std::collections::hash_map::Entry::{Occupied, Vacant}; use std::collections::{HashMap, HashSet}; use std::env; @@ -63,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; @@ -74,6 +73,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 +85,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; @@ -166,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: LazyCell>, + values: OnceLock>, /// A collection of configuration options from the credentials file - credential_values: LazyCell>, + credential_values: OnceLock>, /// CLI config values, passed in via `configure`. cli_config: Option>, /// The current working directory of cargo @@ -178,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: LazyCell, + cargo_exe: OnceLock, /// The location of the rustdoc executable - rustdoc: LazyCell, + rustdoc: OnceLock, /// Whether we are printing extra verbose messages extra_verbose: bool, /// `frozen` is the same as `locked`, but additionally will not access the @@ -199,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: LazyCell>, + easy: OnceLock>, /// Cache of the `SourceId` for crates.io - crates_io_source_id: LazyCell, + 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 @@ -211,23 +210,23 @@ pub struct GlobalContext { /// Environment variable snapshot. env: Env, /// Tracks which sources have been updated to avoid multiple updates. - updated_sources: LazyCell>>, + updated_sources: Mutex>, /// Cache of credentials from configuration or credential providers. /// Maps from url to credential value. - credential_cache: LazyCell>>, + credential_cache: Mutex>, /// Cache of registry config from the `[registries]` table. - registry_config: LazyCell>>>, + registry_config: Mutex>>, /// 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: OnceLock, + future_incompat_config: OnceLock, + net_config: OnceLock, + build_config: OnceLock, + target_cfgs: OnceLock>, + doc_extern_map: OnceLock, progress_config: ProgressConfig, - env_config: LazyCell>>, + 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` @@ -245,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: LazyCell>, + 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: LazyCell>, + deferred_global_last_use: OnceLock>, } impl GlobalContext { @@ -283,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: LazyCell::new(), - credential_values: LazyCell::new(), + values: Default::default(), + credential_values: Default::default(), cli_config: None, - cargo_exe: LazyCell::new(), - rustdoc: LazyCell::new(), + cargo_exe: Default::default(), + rustdoc: Default::default(), extra_verbose: false, frozen: false, locked: false, @@ -304,28 +303,28 @@ impl GlobalContext { }, unstable_flags: CliUnstable::default(), unstable_flags_cli: None, - easy: LazyCell::new(), - crates_io_source_id: LazyCell::new(), + easy: Default::default(), + crates_io_source_id: Default::default(), 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: Default::default(), + credential_cache: Default::default(), + registry_config: Default::default(), 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: 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: LazyCell::new(), + env_config: Default::default(), 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(), + ws_roots: Default::default(), + global_cache_tracker: Default::default(), + deferred_global_last_use: Default::default(), } } @@ -408,8 +407,22 @@ 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 + /// + /// 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) { + match self.shell.try_lock() { + Ok(_) | Err(std::sync::TryLockError::Poisoned(_)) => (), + Err(std::sync::TryLockError::WouldBlock) => panic!("shell is borrowed!"), + } + } } /// Gets the path to the `rustdoc` executable. @@ -513,24 +526,20 @@ 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())) - .borrow_mut() + pub fn updated_sources(&self) -> MutexGuard<'_, HashSet> { + self.updated_sources.lock().unwrap() } /// Cached credentials from credential providers or configuration. - pub fn credential_cache(&self) -> RefMut<'_, HashMap> { - self.credential_cache - .borrow_with(|| RefCell::new(HashMap::new())) - .borrow_mut() + pub fn credential_cache(&self) -> MutexGuard<'_, HashMap> { + self.credential_cache.lock().unwrap() } /// 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())) - .borrow_mut() + pub(crate) fn registry_config( + &self, + ) -> MutexGuard<'_, HashMap>> { + self.registry_config.lock().unwrap() } /// Gets all config values from disk. @@ -550,18 +559,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.into()) { Ok(()) => Ok(()), Err(_) => bail!("could not fill values"), } @@ -730,13 +736,13 @@ 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); } } - self.get_cv_helper(key, self.values()?) + self.get_cv_helper(key, &*self.values()?) } fn get_cv_helper( @@ -1791,7 +1797,7 @@ impl GlobalContext { } } self.credential_values - .fill(credential_values) + .set(credential_values) .expect("was not filled at beginning of the function"); Ok(()) } @@ -1883,12 +1889,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)?; @@ -2099,19 +2105,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. @@ -2122,6 +2128,10 @@ impl GlobalContext { Ok(WarningHandling::default()) } } + + pub fn ws_roots(&self) -> MutexGuard<'_, HashMap> { + self.ws_roots.lock().unwrap() + } } /// Internal error for serde errors. @@ -3192,4 +3202,10 @@ mod tests { assert_eq!(http.multiplexing, result); } } + + #[test] + fn sync_context() { + fn assert_sync() {} + assert_sync::(); + } } 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(()); } 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() + } +} 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!(