From 83fff2d1b1c2267dff5dbb42172bce0872d64223 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 6 Sep 2023 19:30:08 -0700 Subject: [PATCH 01/10] Add a test helper for retrying a function multiple times. --- crates/cargo-test-support/src/lib.rs | 30 ++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index 1a874272071..ffaee9ea5aa 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -1470,3 +1470,33 @@ pub fn symlink_supported() -> bool { pub fn no_such_file_err_msg() -> String { std::io::Error::from_raw_os_error(2).to_string() } + +/// Helper to retry a function `n` times. +/// +/// The function should return `Some` when it is ready. +pub fn retry(n: u32, mut f: F) -> R +where + F: FnMut() -> Option, +{ + let mut count = 0; + let start = std::time::Instant::now(); + loop { + if let Some(r) = f() { + return r; + } + count += 1; + if count > n { + panic!( + "test did not finish within {n} attempts ({:?} total)", + start.elapsed() + ); + } + sleep_ms(100); + } +} + +#[test] +#[should_panic(expected = "test did not finish")] +fn retry_fails() { + retry(2, || None::<()>); +} From b85ef38187000f0d65325696a4ef535e0c93f0d8 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 6 Sep 2023 19:30:45 -0700 Subject: [PATCH 02/10] Add test helpers for waiting for an operation with a timeout. --- crates/cargo-test-support/src/lib.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index ffaee9ea5aa..ec74ce0b206 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -13,6 +13,7 @@ use std::path::{Path, PathBuf}; use std::process::{Command, Output}; use std::str; use std::sync::OnceLock; +use std::thread::JoinHandle; use std::time::{self, Duration}; use anyhow::{bail, Result}; @@ -1500,3 +1501,20 @@ where fn retry_fails() { retry(2, || None::<()>); } + +/// Helper that waits for a thread to finish, up to `n` tenths of a second. +pub fn thread_wait_timeout(n: u32, thread: JoinHandle) -> T { + retry(n, || thread.is_finished().then_some(())); + thread.join().unwrap() +} + +/// Helper that runs some function, and waits up to `n` tenths of a second for +/// it to finish. +pub fn threaded_timeout(n: u32, f: F) -> R +where + F: FnOnce() -> R + Send + 'static, + R: Send + 'static, +{ + let thread = std::thread::spawn(|| f()); + thread_wait_timeout(n, thread) +} From b37c8f35c01f640ea1a03b8d1fae02f8f7ac5f43 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 6 Sep 2023 19:36:08 -0700 Subject: [PATCH 03/10] Add ConfigBuilder::root This adds the `root` method to set the root directory to be able to override paths::root(). --- tests/testsuite/config.rs | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 7078fc445fb..9e77048c3f3 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -21,6 +21,7 @@ pub struct ConfigBuilder { unstable: Vec, config_args: Vec, cwd: Option, + root: Option, enable_nightly_features: bool, } @@ -30,6 +31,7 @@ impl ConfigBuilder { env: HashMap::new(), unstable: Vec::new(), config_args: Vec::new(), + root: None, cwd: None, enable_nightly_features: false, } @@ -60,8 +62,28 @@ impl ConfigBuilder { } /// Sets the current working directory where config files will be loaded. + /// + /// Default is the root from [`ConfigBuilder::root`] or [`paths::root`]. pub fn cwd(&mut self, path: impl AsRef) -> &mut Self { - self.cwd = Some(paths::root().join(path.as_ref())); + let path = path.as_ref(); + let cwd = self + .root + .as_ref() + .map_or_else(|| paths::root().join(path), |r| r.join(path)); + self.cwd = Some(cwd); + self + } + + /// Sets the test root directory. + /// + /// This generally should not be necessary. It is only useful if you want + /// to create a `Config` from within a thread. Since Cargo's testsuite + /// uses thread-local storage, this can be used to avoid accessing that + /// thread-local storage. + /// + /// Default is [`paths::root`]. + pub fn root(&mut self, path: impl Into) -> &mut Self { + self.root = Some(path.into()); self } @@ -72,14 +94,15 @@ impl ConfigBuilder { /// Creates the `Config`, returning a Result. pub fn build_err(&self) -> CargoResult { - let output = Box::new(fs::File::create(paths::root().join("shell.out")).unwrap()); + let root = self.root.clone().unwrap_or_else(|| paths::root()); + let output = Box::new(fs::File::create(root.join("shell.out")).unwrap()); let shell = Shell::from_write(output); - let cwd = self.cwd.clone().unwrap_or_else(|| paths::root()); - let homedir = paths::home(); + let cwd = self.cwd.clone().unwrap_or_else(|| root.clone()); + let homedir = root.join("home").join(".cargo"); let mut config = Config::new(shell, cwd, homedir); config.nightly_features_allowed = self.enable_nightly_features || !self.unstable.is_empty(); config.set_env(self.env.clone()); - config.set_search_stop_path(paths::root()); + config.set_search_stop_path(&root); config.configure( 0, false, From b4982adfd996303f1574126e5be4c8d033df3be1 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 6 Sep 2023 21:01:18 -0700 Subject: [PATCH 04/10] Add a new package cache locking system. This introduces a new `CacheLocker` which manages locks on the package cache. Instead of either being "locked" or "not locked", the new locker supports multiple modes: - Shared lock: Cargo can read from the package sources, along with any other cargos reading at the same time. - Download exclusive lock: Only one cargo can perform downloads. Download locks do not interfere with Shared locks, since it is expected that downloading does not modify existing files (only adds new ones). - Mutate exclusive lock: Only one cargo can have this lock, and it also prevents shared locks. This is so that the cargo can modify the package cache (such as deleting files) without breaking concurrent processes. --- crates/cargo-test-support/src/compare.rs | 1 + crates/xtask-bump-check/src/xtask.rs | 3 +- src/cargo/core/compiler/context/mod.rs | 8 + src/cargo/core/compiler/future_incompat.rs | 6 +- src/cargo/core/package.rs | 11 +- src/cargo/ops/cargo_add/mod.rs | 5 +- src/cargo/ops/cargo_generate_lockfile.rs | 5 +- src/cargo/ops/cargo_package.rs | 3 +- .../ops/common_for_install_and_uninstall.rs | 5 +- src/cargo/ops/registry/mod.rs | 3 +- src/cargo/ops/registry/publish.rs | 3 +- src/cargo/ops/resolve.rs | 5 +- src/cargo/sources/git/source.rs | 5 +- src/cargo/sources/registry/download.rs | 7 +- src/cargo/sources/registry/http_remote.rs | 5 +- src/cargo/sources/registry/index.rs | 3 +- src/cargo/sources/registry/mod.rs | 5 +- src/cargo/sources/registry/remote.rs | 15 +- src/cargo/util/cache_lock.rs | 463 ++++++++++++++++++ src/cargo/util/config/mod.rs | 119 ++--- src/cargo/util/flock.rs | 223 +++++---- src/cargo/util/mod.rs | 1 + tests/testsuite/cache_lock.rs | 302 ++++++++++++ tests/testsuite/main.rs | 1 + tests/testsuite/search.rs | 5 +- 25 files changed, 1007 insertions(+), 205 deletions(-) create mode 100644 src/cargo/util/cache_lock.rs create mode 100644 tests/testsuite/cache_lock.rs diff --git a/crates/cargo-test-support/src/compare.rs b/crates/cargo-test-support/src/compare.rs index 09e3a5a0ca5..154938ad57b 100644 --- a/crates/cargo-test-support/src/compare.rs +++ b/crates/cargo-test-support/src/compare.rs @@ -236,6 +236,7 @@ fn substitute_macros(input: &str) -> String { ("[SKIPPING]", " Skipping"), ("[WAITING]", " Waiting"), ("[PUBLISHED]", " Published"), + ("[BLOCKING]", " Blocking"), ]; let mut result = input.to_owned(); for &(pat, subst) in ¯os { diff --git a/crates/xtask-bump-check/src/xtask.rs b/crates/xtask-bump-check/src/xtask.rs index 4bf3f03d5af..e1698e22877 100644 --- a/crates/xtask-bump-check/src/xtask.rs +++ b/crates/xtask-bump-check/src/xtask.rs @@ -22,6 +22,7 @@ use cargo::core::Registry; use cargo::core::SourceId; use cargo::core::Workspace; use cargo::sources::source::QueryKind; +use cargo::util::cache_lock::CacheLockMode; use cargo::util::command_prelude::*; use cargo::util::ToSemver; use cargo::CargoResult; @@ -361,7 +362,7 @@ fn check_crates_io<'a>( ) -> CargoResult<()> { let source_id = SourceId::crates_io(config)?; let mut registry = PackageRegistry::new(config)?; - let _lock = config.acquire_package_cache_lock()?; + let _lock = config.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; registry.lock_patches(); config.shell().status( STATUS, diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index 010fe2793dd..cfbfccb3045 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/src/cargo/core/compiler/context/mod.rs @@ -7,6 +7,7 @@ use std::sync::{Arc, Mutex}; use crate::core::compiler::compilation::{self, UnitOutput}; use crate::core::compiler::{self, artifact, Unit}; use crate::core::PackageId; +use crate::util::cache_lock::CacheLockMode; use crate::util::errors::CargoResult; use crate::util::profile; use anyhow::{bail, Context as _}; @@ -132,6 +133,13 @@ impl<'a, 'cfg> Context<'a, 'cfg> { /// /// [`ops::cargo_compile`]: ../../../ops/cargo_compile/index.html pub fn compile(mut self, exec: &Arc) -> CargoResult> { + // A shared lock is held during the duration of the build since rustc + // needs to read from the `src` cache, and we don't want other + // commands modifying the `src` cache while it is running. + let _lock = self + .bcx + .config + .acquire_package_cache_lock(CacheLockMode::Shared)?; let mut queue = JobQueue::new(self.bcx); let mut plan = BuildPlan::new(); let build_plan = self.bcx.build_config.build_plan; diff --git a/src/cargo/core/compiler/future_incompat.rs b/src/cargo/core/compiler/future_incompat.rs index 907a0f97d63..27645a85cb4 100644 --- a/src/cargo/core/compiler/future_incompat.rs +++ b/src/cargo/core/compiler/future_incompat.rs @@ -37,6 +37,7 @@ use crate::core::compiler::BuildContext; use crate::core::{Dependency, PackageId, Workspace}; use crate::sources::source::QueryKind; use crate::sources::SourceConfigMap; +use crate::util::cache_lock::CacheLockMode; use crate::util::{iter_join, CargoResult}; use anyhow::{bail, format_err, Context}; use serde::{Deserialize, Serialize}; @@ -297,7 +298,10 @@ fn render_report(per_package_reports: &[FutureIncompatReportPackage]) -> BTreeMa /// This is best-effort - if an error occurs, `None` will be returned. fn get_updates(ws: &Workspace<'_>, package_ids: &BTreeSet) -> Option { // This in general ignores all errors since this is opportunistic. - let _lock = ws.config().acquire_package_cache_lock().ok()?; + let _lock = ws + .config() + .acquire_package_cache_lock(CacheLockMode::DownloadExclusive) + .ok()?; // Create a set of updated registry sources. let map = SourceConfigMap::new(ws.config()).ok()?; let mut package_ids: BTreeSet<_> = package_ids diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 76f6c405bc3..274798474ac 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -24,7 +24,7 @@ use crate::core::resolver::{HasDevUnits, Resolve}; use crate::core::{Dependency, Manifest, PackageId, SourceId, Target}; use crate::core::{Summary, Workspace}; use crate::sources::source::{MaybePackage, SourceMap}; -use crate::util::config::PackageCacheLock; +use crate::util::cache_lock::{CacheLock, CacheLockMode}; use crate::util::errors::{CargoResult, HttpNotSuccessful}; use crate::util::interning::InternedString; use crate::util::network::http::http_handle_and_timeout; @@ -367,7 +367,7 @@ pub struct Downloads<'a, 'cfg> { next_speed_check_bytes_threshold: Cell, /// Global filesystem lock to ensure only one Cargo is downloading at a /// time. - _lock: PackageCacheLock<'cfg>, + _lock: CacheLock<'cfg>, } struct Download<'cfg> { @@ -465,7 +465,9 @@ impl<'cfg> PackageSet<'cfg> { timeout, next_speed_check: Cell::new(Instant::now()), next_speed_check_bytes_threshold: Cell::new(0), - _lock: self.config.acquire_package_cache_lock()?, + _lock: self + .config + .acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?, }) } @@ -478,6 +480,9 @@ impl<'cfg> PackageSet<'cfg> { pub fn get_many(&self, ids: impl IntoIterator) -> CargoResult> { let mut pkgs = Vec::new(); + let _lock = self + .config + .acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; let mut downloads = self.enable_download()?; for id in ids { pkgs.extend(downloads.start(id)?); diff --git a/src/cargo/ops/cargo_add/mod.rs b/src/cargo/ops/cargo_add/mod.rs index 968d6068f6a..a8a8b76fa54 100644 --- a/src/cargo/ops/cargo_add/mod.rs +++ b/src/cargo/ops/cargo_add/mod.rs @@ -23,6 +23,7 @@ use crate::core::Shell; use crate::core::Summary; use crate::core::Workspace; use crate::sources::source::QueryKind; +use crate::util::cache_lock::CacheLockMode; use crate::util::style; use crate::util::toml_mut::dependency::Dependency; use crate::util::toml_mut::dependency::GitSource; @@ -77,7 +78,9 @@ pub fn add(workspace: &Workspace<'_>, options: &AddOptions<'_>) -> CargoResult<( let mut registry = PackageRegistry::new(options.config)?; let deps = { - let _lock = options.config.acquire_package_cache_lock()?; + let _lock = options + .config + .acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; registry.lock_patches(); options .dependencies diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index a83a92ccccf..832261a4184 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -3,6 +3,7 @@ use crate::core::resolver::features::{CliFeatures, HasDevUnits}; use crate::core::{PackageId, PackageIdSpec}; use crate::core::{Resolve, SourceId, Workspace}; use crate::ops; +use crate::util::cache_lock::CacheLockMode; use crate::util::config::Config; use crate::util::style; use crate::util::CargoResult; @@ -48,7 +49,9 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes // Updates often require a lot of modifications to the registry, so ensure // that we're synchronized against other Cargos. - let _lock = ws.config().acquire_package_cache_lock()?; + let _lock = ws + .config() + .acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; let max_rust_version = ws.rust_version(); diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index b6423aa9684..179673b473b 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -13,6 +13,7 @@ use crate::core::{registry::PackageRegistry, resolver::HasDevUnits}; use crate::core::{Feature, Shell, Verbosity, Workspace}; use crate::core::{Package, PackageId, PackageSet, Resolve, SourceId}; use crate::sources::PathSource; +use crate::util::cache_lock::CacheLockMode; use crate::util::config::JobsConfig; use crate::util::errors::CargoResult; use crate::util::toml::TomlManifest; @@ -806,7 +807,7 @@ pub fn check_yanked( ) -> CargoResult<()> { // Checking the yanked status involves taking a look at the registry and // maybe updating files, so be sure to lock it here. - let _lock = config.acquire_package_cache_lock()?; + let _lock = config.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; let mut sources = pkg_set.sources_mut(); let mut pending: Vec = resolve.iter().collect(); diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index 0934cbd2943..a0f0c4afa9a 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -17,6 +17,7 @@ use crate::ops::{self, CompileFilter, CompileOptions}; use crate::sources::source::QueryKind; use crate::sources::source::Source; use crate::sources::PathSource; +use crate::util::cache_lock::CacheLockMode; use crate::util::errors::CargoResult; use crate::util::Config; use crate::util::{FileLock, Filesystem}; @@ -536,7 +537,7 @@ where // This operation may involve updating some sources or making a few queries // which may involve frobbing caches, as a result make sure we synchronize // with other global Cargos - let _lock = config.acquire_package_cache_lock()?; + let _lock = config.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; if needs_update { source.invalidate_cache(); @@ -604,7 +605,7 @@ where // This operation may involve updating some sources or making a few queries // which may involve frobbing caches, as a result make sure we synchronize // with other global Cargos - let _lock = config.acquire_package_cache_lock()?; + let _lock = config.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; source.invalidate_cache(); diff --git a/src/cargo/ops/registry/mod.rs b/src/cargo/ops/registry/mod.rs index c82230a1694..3fa8fd29162 100644 --- a/src/cargo/ops/registry/mod.rs +++ b/src/cargo/ops/registry/mod.rs @@ -22,6 +22,7 @@ use crate::core::SourceId; use crate::sources::source::Source; use crate::sources::{RegistrySource, SourceConfigMap}; use crate::util::auth; +use crate::util::cache_lock::CacheLockMode; use crate::util::config::{Config, PathAndArgs}; use crate::util::errors::CargoResult; use crate::util::network::http::http_handle; @@ -131,7 +132,7 @@ fn registry( } let cfg = { - let _lock = config.acquire_package_cache_lock()?; + let _lock = config.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; let mut src = RegistrySource::remote(source_ids.replacement, &HashSet::new(), config)?; // Only update the index if `force_update` is set. if force_update { diff --git a/src/cargo/ops/registry/publish.rs b/src/cargo/ops/registry/publish.rs index a88a0a30f0a..5009c0b67cc 100644 --- a/src/cargo/ops/registry/publish.rs +++ b/src/cargo/ops/registry/publish.rs @@ -30,6 +30,7 @@ use crate::sources::source::QueryKind; use crate::sources::SourceConfigMap; use crate::sources::CRATES_IO_REGISTRY; use crate::util::auth; +use crate::util::cache_lock::CacheLockMode; use crate::util::config::JobsConfig; use crate::util::Progress; use crate::util::ProgressStyle; @@ -233,7 +234,7 @@ fn wait_for_publish( progress.tick_now(0, max, "")?; let is_available = loop { { - let _lock = config.acquire_package_cache_lock()?; + let _lock = config.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; // Force re-fetching the source // // As pulling from a git source is expensive, we track when we've done it within the diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 053098d5559..36fc00b7fea 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -68,6 +68,7 @@ use crate::core::Feature; use crate::core::{GitReference, PackageId, PackageIdSpec, PackageSet, SourceId, Workspace}; use crate::ops; use crate::sources::PathSource; +use crate::util::cache_lock::CacheLockMode; use crate::util::errors::CargoResult; use crate::util::RustVersion; use crate::util::{profile, CanonicalUrl}; @@ -289,7 +290,9 @@ pub fn resolve_with_previous<'cfg>( ) -> CargoResult { // We only want one Cargo at a time resolving a crate graph since this can // involve a lot of frobbing of the global caches. - let _lock = ws.config().acquire_package_cache_lock()?; + let _lock = ws + .config() + .acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; // Here we place an artificial limitation that all non-registry sources // cannot be locked at more than one revision. This means that if a Git diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index 7af342f4505..f006a591cce 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -8,6 +8,7 @@ use crate::sources::source::MaybePackage; use crate::sources::source::QueryKind; use crate::sources::source::Source; use crate::sources::PathSource; +use crate::util::cache_lock::CacheLockMode; use crate::util::errors::CargoResult; use crate::util::hex::short_hash; use crate::util::Config; @@ -212,7 +213,9 @@ impl<'cfg> Source for GitSource<'cfg> { // Ignore errors creating it, in case this is a read-only filesystem: // perhaps the later operations can succeed anyhow. let _ = git_fs.create_dir(); - let git_path = self.config.assert_package_cache_locked(&git_fs); + let git_path = self + .config + .assert_package_cache_locked(CacheLockMode::DownloadExclusive, &git_fs); // Before getting a checkout, make sure that `/git` is // marked as excluded from indexing and backups. Older versions of Cargo diff --git a/src/cargo/sources/registry/download.rs b/src/cargo/sources/registry/download.rs index 9bf838625c1..7864328354f 100644 --- a/src/cargo/sources/registry/download.rs +++ b/src/cargo/sources/registry/download.rs @@ -12,6 +12,7 @@ use crate::core::PackageId; use crate::sources::registry::MaybeLock; use crate::sources::registry::RegistryConfig; use crate::util::auth; +use crate::util::cache_lock::CacheLockMode; use crate::util::errors::CargoResult; use crate::util::{Config, Filesystem}; use std::fmt::Write as FmtWrite; @@ -38,7 +39,7 @@ pub(super) fn download( registry_config: RegistryConfig, ) -> CargoResult { let path = cache_path.join(&pkg.tarball_name()); - let path = config.assert_package_cache_locked(&path); + let path = config.assert_package_cache_locked(CacheLockMode::DownloadExclusive, &path); // Attempt to open a read-only copy first to avoid an exclusive write // lock and also work with read-only filesystems. Note that we check the @@ -117,7 +118,7 @@ pub(super) fn finish_download( cache_path.create_dir()?; let path = cache_path.join(&pkg.tarball_name()); - let path = config.assert_package_cache_locked(&path); + let path = config.assert_package_cache_locked(CacheLockMode::DownloadExclusive, &path); let mut dst = OpenOptions::new() .create(true) .read(true) @@ -144,7 +145,7 @@ pub(super) fn is_crate_downloaded( pkg: PackageId, ) -> bool { let path = cache_path.join(pkg.tarball_name()); - let path = config.assert_package_cache_locked(&path); + let path = config.assert_package_cache_locked(CacheLockMode::DownloadExclusive, &path); if let Ok(meta) = fs::metadata(path) { return meta.len() > 0; } diff --git a/src/cargo/sources/registry/http_remote.rs b/src/cargo/sources/registry/http_remote.rs index 9fe76333b19..3d31110c38e 100644 --- a/src/cargo/sources/registry/http_remote.rs +++ b/src/cargo/sources/registry/http_remote.rs @@ -4,6 +4,7 @@ use crate::core::{PackageId, SourceId}; use crate::sources::registry::download; use crate::sources::registry::MaybeLock; use crate::sources::registry::{LoadResponse, RegistryConfig, RegistryData}; +use crate::util::cache_lock::CacheLockMode; use crate::util::errors::{CargoResult, HttpNotSuccessful}; use crate::util::network::http::http_handle; use crate::util::network::retry::{Retry, RetryResult}; @@ -461,7 +462,8 @@ impl<'cfg> RegistryData for HttpRegistry<'cfg> { } fn assert_index_locked<'a>(&self, path: &'a Filesystem) -> &'a Path { - self.config.assert_package_cache_locked(path) + self.config + .assert_package_cache_locked(CacheLockMode::DownloadExclusive, path) } fn is_updated(&self) -> bool { @@ -744,6 +746,7 @@ impl<'cfg> RegistryData for HttpRegistry<'cfg> { Poll::Ready(cfg) => break cfg.to_owned(), } }; + download::download( &self.cache_path, &self.config, diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index ca1cf40693f..7a9ba0e15cb 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -89,6 +89,7 @@ use crate::core::dependency::{Artifact, DepKind}; use crate::core::Dependency; use crate::core::{PackageId, SourceId, Summary}; use crate::sources::registry::{LoadResponse, RegistryData}; +use crate::util::cache_lock::CacheLockMode; use crate::util::interning::InternedString; use crate::util::IntoUrl; use crate::util::{internal, CargoResult, Config, Filesystem, OptVersionReq, RustVersion}; @@ -823,7 +824,7 @@ impl Summaries { // something in case of error. if paths::create_dir_all(cache_path.parent().unwrap()).is_ok() { let path = Filesystem::new(cache_path.clone()); - config.assert_package_cache_locked(&path); + config.assert_package_cache_locked(CacheLockMode::DownloadExclusive, &path); if let Err(e) = fs::write(cache_path, &cache_bytes) { tracing::info!("failed to write cache: {}", e); } diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index fb8f7981788..bc343fb7b5d 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -206,6 +206,7 @@ use crate::sources::source::MaybePackage; use crate::sources::source::QueryKind; use crate::sources::source::Source; use crate::sources::PathSource; +use crate::util::cache_lock::CacheLockMode; use crate::util::hex; use crate::util::interning::InternedString; use crate::util::network::PollExt; @@ -581,7 +582,9 @@ impl<'cfg> RegistrySource<'cfg> { let package_dir = format!("{}-{}", pkg.name(), pkg.version()); let dst = self.src_path.join(&package_dir); let path = dst.join(PACKAGE_SOURCE_LOCK); - let path = self.config.assert_package_cache_locked(&path); + let path = self + .config + .assert_package_cache_locked(CacheLockMode::DownloadExclusive, &path); let unpack_dir = path.parent().unwrap(); match fs::read_to_string(path) { Ok(ok) => match serde_json::from_str::(&ok) { diff --git a/src/cargo/sources/registry/remote.rs b/src/cargo/sources/registry/remote.rs index 39eb14dc083..ba171eac3d4 100644 --- a/src/cargo/sources/registry/remote.rs +++ b/src/cargo/sources/registry/remote.rs @@ -6,6 +6,7 @@ use crate::sources::git::fetch::RemoteKind; use crate::sources::registry::download; use crate::sources::registry::MaybeLock; use crate::sources::registry::{LoadResponse, RegistryConfig, RegistryData}; +use crate::util::cache_lock::CacheLockMode; use crate::util::errors::CargoResult; use crate::util::interning::InternedString; use crate::util::{Config, Filesystem}; @@ -104,7 +105,9 @@ impl<'cfg> RemoteRegistry<'cfg> { fn repo(&self) -> CargoResult<&git2::Repository> { self.repo.try_borrow_with(|| { trace!("acquiring registry index lock"); - let path = self.config.assert_package_cache_locked(&self.index_path); + let path = self + .config + .assert_package_cache_locked(CacheLockMode::DownloadExclusive, &self.index_path); match git2::Repository::open(&path) { Ok(repo) => Ok(repo), @@ -216,7 +219,8 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { } fn assert_index_locked<'a>(&self, path: &'a Filesystem) -> &'a Path { - self.config.assert_package_cache_locked(path) + self.config + .assert_package_cache_locked(CacheLockMode::DownloadExclusive, path) } /// Read the general concept for `load()` on [`RegistryData::load`]. @@ -302,7 +306,8 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { fn config(&mut self) -> Poll>> { debug!("loading config"); self.prepare()?; - self.config.assert_package_cache_locked(&self.index_path); + self.config + .assert_package_cache_locked(CacheLockMode::DownloadExclusive, &self.index_path); match ready!(self.load(Path::new(""), Path::new(RegistryConfig::NAME), None)?) { LoadResponse::Data { raw_data, .. } => { trace!("config loaded"); @@ -346,7 +351,9 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { self.head.set(None); *self.tree.borrow_mut() = None; self.current_sha.set(None); - let _path = self.config.assert_package_cache_locked(&self.index_path); + let _path = self + .config + .assert_package_cache_locked(CacheLockMode::DownloadExclusive, &self.index_path); if !self.quiet { self.config .shell() diff --git a/src/cargo/util/cache_lock.rs b/src/cargo/util/cache_lock.rs new file mode 100644 index 00000000000..f283df3bc66 --- /dev/null +++ b/src/cargo/util/cache_lock.rs @@ -0,0 +1,463 @@ +//! Support for locking the package and index caches. +//! +//! This implements locking on the package and index caches (source files, +//! `.crate` files, and index caches) to coordinate when multiple cargos are +//! running at the same time. There are three styles of locks: +//! +//! * [`CacheLockMode::DownloadExclusive`] -- This is an exclusive lock +//! acquired while downloading packages and doing resolution. +//! * [`CacheLockMode::Shared`] -- This is a shared lock acquired while a +//! build is running. In other words, whenever cargo just needs to read from +//! the cache, it should hold this lock. This is here to ensure that no +//! cargos are trying to read the source caches when cache garbage +//! collection runs. +//! * [`CacheLockMode::MutateExclusive`] -- This is an exclusive lock acquired +//! whenever needing to modify existing source files (for example, with +//! cache garbage collection). This is acquired to make sure that no other +//! cargo is reading from the cache. +//! +//! Importantly, a `DownloadExclusive` lock does *not* interfere with a +//! `Shared` lock. The download process generally does not modify source +//! files, so other cargos should be able to safely proceed in reading source +//! files[^1]. +//! +//! This is implemented by two separate lock files, the "download" one and the +//! "mutate" one. The `MutateExclusive` lock acquired both the "mutate" and +//! "download" locks. The `Shared` lock acquires the "mutate" lock in share +//! mode. +//! +//! An important rule is that `MutateExclusive` acquires the locks in the +//! order "mutate" first and then the "download". That helps prevent +//! deadlocks. It is not allowed for a cargo to first acquire a +//! `DownloadExclusive` lock and then a `Shared` lock because that would open +//! it up for deadlock. +//! +//! Another rule is that there should be only one [`CacheLocker`] per process +//! to uphold the ordering rules. You could in theory have multiple if you +//! could ensure that other threads would make progress and drop a lock, but +//! cargo is not architected that way. +//! +//! It is safe to recursively acquire a lock as many times as you want. +//! +//! ## Interaction with older cargos +//! +//! Before version 1.74, cargo only acquired the `DownloadExclusive` lock when +//! downloading and doing resolution. Newer cargos that acquire +//! `MutateExclusive` should still correctly block when an old cargo is +//! downloading (because it also acquires `DownloadExclusive`), but they do +//! not properly coordinate when an old cargo is in the build phase (because +//! it holds no locks). This isn't expected to be much of a problem because +//! the intended use of mutating the cache is only to delete old contents +//! which aren't currently being used. It is possible for there to be a +//! conflict, particularly if the user manually deletes the entire cache, but +//! it is not expected for this scenario to happen too often, and the only +//! consequence is that one side or the other encounters an error and needs to +//! retry. +//! +//! [^1]: A minor caveat is that downloads will delete an existing `src` +//! directory if it was extracted via an old cargo. See +//! [`crate::sources::registry::RegistrySource::unpack_package`]. This +//! should probably be fixed, but is unlikely to be a problem if the user is +//! only using versions of cargo with the same deletion logic. + +use super::FileLock; +use crate::CargoResult; +use crate::Config; +use anyhow::Context; +use std::cell::RefCell; +use std::io; + +/// The style of lock to acquire. +#[derive(Copy, Clone, Debug)] +pub enum CacheLockMode { + /// A `Shared` lock allows multiple cargos to read from the source files. + /// + /// If another cargo has a `MutateExclusive` lock, then an attempt to get + /// a `Shared` will block. + /// + /// If another cargo has a `DownloadExclusive` lock, then the both can + /// operate concurrently under the assumption that downloading does not + /// modify existing source files. + Shared, + /// A `DownloadExclusive` lock ensures that only one cargo is downloading + /// new packages. + /// + /// If another cargo has a `MutateExclusive` lock, then an attempt to get + /// a `DownloadExclusive` lock will block. + /// + /// If another cargo has a `Shared` lock, then both can operate + /// concurrently. + DownloadExclusive, + /// A `MutateExclusive` lock ensures no other cargo is reading or writing + /// from the package caches. + /// + /// This is used for things like garbage collection to avoid modifying + /// caches while other cargos are running. + MutateExclusive, +} + +/// A locker that can be used to acquire locks. +#[derive(Debug)] +pub struct CacheLocker { + state: RefCell, +} + +/// The state of the [`CacheLocker`]. +/// +/// [`CacheLocker`] uses interior mutability because it is stuffed inside the +/// global `Config`, which does not allow mutation. +/// +/// An important note is that locks can be `None` even when they are held. +/// This can happen on things like old NFS mounts where locking isn't +/// supported. We otherwise pretend we have a lock via the lock counts. See +/// [`FileLock`] for more detail on that. +#[derive(Debug, Default)] +struct CacheState { + cache_lock: Option, + cache_lock_count: u32, + mutate_lock: Option, + mutate_lock_count: u32, + /// Indicator of whether or not `mutate_lock` is currently a shared lock + /// or an exclusive one. + mutate_is_exclusive: bool, +} + +/// A held lock guard. +/// +/// When this is dropped, the lock will be released. +#[must_use] +pub struct CacheLock<'lock> { + mode: CacheLockMode, + locker: &'lock CacheLocker, +} + +/// The filename for the [`CacheLockMode::DownloadExclusive`] lock. +const CACHE_LOCK_NAME: &str = ".package-cache"; +/// The filename for the [`CacheLockMode::MutateExclusive`] and +/// [`CacheLockMode::Shared`] lock. +const MUTATE_NAME: &str = ".package-cache-mutate"; + +// Descriptions that are displayed in the "Blocking" message shown to the user. +const SHARED_DESCR: &str = "shared package cache"; +const DOWNLOAD_EXCLUSIVE_DESCR: &str = "package cache"; +const MUTATE_EXCLUSIVE_DESCR: &str = "package cache mutation"; + +/// Macro to unlock a previously acquired lock. +macro_rules! decrement { + ($state:expr, $count:ident, $lock:ident) => { + let new_cnt = $state.$count.checked_sub(1).unwrap(); + $state.$count = new_cnt; + if new_cnt == 0 { + // This will drop, releasing the lock. + $state.$lock = None; + } + }; +} + +/// Macro for acquiring a shared lock (can block). +macro_rules! do_shared_lock { + ($self: ident, $config: expr, $to_set: ident, $lock_name: expr, $descr: expr) => { + $self.$to_set = match $config + .home() + .open_shared_create($lock_name, $config, $descr) + { + Ok(lock) => Some(lock), + Err(e) => { + // There is no error here because locking is mostly a + // best-effort attempt. If cargo home is read-only, we don't + // want to fail just because we couldn't create the lock file. + tracing::warn!("failed to acquire cache lock {}: {e:?}", $lock_name); + None + } + }; + }; +} + +/// Macro for acquiring a shared lock without blocking. +macro_rules! do_try_shared_lock { + ($self: ident, $config: expr, $to_set: ident, $lock_name: expr, $unused: expr) => { + $self.$to_set = match $config.home().try_open_shared_create($lock_name) { + Ok(Some(lock)) => Some(lock), + Ok(None) => { + return Ok(false); + } + Err(e) => { + tracing::warn!("failed to acquire cache lock {}: {e:?}", $lock_name); + None + } + }; + }; +} + +/// Macro for acquiring an exclusive lock (can block). +macro_rules! do_exclusive_lock { + ($self: ident, $config: expr, $to_set: ident, $lock_name: expr, $descr: expr) => { + match $config.home().open_rw($lock_name, $config, $descr) { + Ok(lock) => { + $self.$to_set = Some(lock); + Ok(()) + } + Err(e) => { + if maybe_readonly(&e) { + // This is a best-effort attempt to at least try to + // acquire some sort of lock. This can help in the + // situation where this cargo only has read-only access, + // but maybe some other cargo has read-write. This will at + // least attempt to coordinate with it. + // + // We don't want to fail on a read-only mount because + // cargo grabs an exclusive lock in situations where it + // may only be reading from the package cache. In that + // case, cargo isn't writing anything, and we don't want + // to fail on that. + do_shared_lock!($self, $config, $to_set, $lock_name, $descr); + Ok(()) + } else { + Err(e).with_context(|| "failed to acquire package cache lock") + } + } + } + }; +} + +/// Macro for acquiring an exclusive lock without blocking. +macro_rules! do_try_exclusive_lock { + ($self: ident, $config: expr, $to_set: ident, $lock_name: expr, $unused: expr) => { + match $config.home().try_open_rw($lock_name) { + Ok(Some(lock)) => { + $self.$to_set = Some(lock); + Ok(()) + } + Ok(None) => return Ok(false), + Err(e) => { + if maybe_readonly(&e) { + do_try_shared_lock!($self, $config, $to_set, $lock_name, $unused); + Ok(()) + } else { + Err(e).with_context(|| "failed to acquire package cache lock") + } + } + } + }; +} + +/// Macro to help with acquiring a lock. +/// +/// `shared` and `exclusive` are inputs to the macro so that either the +/// blocking or non-blocking implementations can be called based on what the +/// caller wants. +macro_rules! do_lock { + ($self: ident, $config: expr, $mode: expr, $shared: ident, $exclusive: ident) => { + use CacheLockMode::*; + match ( + $mode, + &$self.cache_lock_count, + &$self.mutate_lock_count, + $self.mutate_is_exclusive, + ) { + (Shared, 0, 0, _) => { + // Shared lock, no locks currently held. + $shared!($self, $config, mutate_lock, MUTATE_NAME, SHARED_DESCR); + $self.mutate_lock_count += 1; + $self.mutate_is_exclusive = false; + } + (DownloadExclusive, 0, _, _) => { + // DownloadExclusive lock, no DownloadExclusive lock currently held. + $exclusive!( + $self, + $config, + cache_lock, + CACHE_LOCK_NAME, + DOWNLOAD_EXCLUSIVE_DESCR + )?; + $self.cache_lock_count += 1; + } + (MutateExclusive, 0, 0, _) => { + // MutateExclusive lock, no locks currently held. + $exclusive!( + $self, + $config, + mutate_lock, + MUTATE_NAME, + MUTATE_EXCLUSIVE_DESCR + )?; + $self.mutate_lock_count += 1; + $self.mutate_is_exclusive = true; + + // Part of the contract of MutateExclusive is that it doesn't + // allow any processes to have a lock on the package cache, so + // this acquires both locks. + if let Err(e) = $exclusive!( + $self, + $config, + cache_lock, + CACHE_LOCK_NAME, + DOWNLOAD_EXCLUSIVE_DESCR + ) { + decrement!($self, mutate_lock_count, mutate_lock); + return Err(e); + } + $self.cache_lock_count += 1; + } + (Shared, 1.., 0, _) => { + // Shared lock, when a DownloadExclusive is held. + // + // This isn't supported because it could cause a deadlock. If + // one cargo is attempting to acquire a MutateExclusive lock, + // and acquires the mutate lock, but is blocked on the + // download lock, and the cargo that holds the download lock + // attempts to get a shared lock, they would end up blocking + // each other. + panic!("shared lock while holding download lock is not allowed"); + } + (MutateExclusive, _, 1.., false) => { + // MutateExclusive lock, when a Shared lock is held. + // + // Lock upgrades are dicey. It might be possible to support + // this but would take a bit of work, and so far it isn't + // needed. + panic!("lock upgrade from Shared to MutateExclusive not supported"); + } + (Shared, _, 1.., _) => { + // Shared lock, when a Shared or MutateExclusive lock is held. + // + // MutateExclusive is more restrictive than Shared, so no need + // to do anything. + $self.mutate_lock_count = $self.mutate_lock_count.checked_add(1).unwrap(); + } + (DownloadExclusive, 1.., _, _) => { + // DownloadExclusive lock, when another DownloadExclusive is held. + $self.cache_lock_count = $self.cache_lock_count.checked_add(1).unwrap(); + } + (MutateExclusive, _, 1.., true) => { + // MutateExclusive lock, when another MutateExclusive is held. + $self.cache_lock_count += 1; + $self.mutate_lock_count += 1; + } + (MutateExclusive, 1.., 0, _) => { + // MutateExclusive lock, when only a DownloadExclusive is held. + $exclusive!( + $self, + $config, + mutate_lock, + MUTATE_NAME, + MUTATE_EXCLUSIVE_DESCR + )?; + // Both of these need to be incremented to match the behavior + // in the drop impl. + $self.cache_lock_count += 1; + $self.mutate_lock_count += 1; + $self.mutate_is_exclusive = true; + } + } + }; +} + +impl CacheLocker { + /// Creates a new `CacheLocker`. + pub fn new() -> CacheLocker { + CacheLocker { + state: RefCell::new(CacheState::default()), + } + } + + /// Acquires a lock with the given mode, possibly blocking if another + /// cargo is holding the lock. + pub fn lock(&self, config: &Config, mode: CacheLockMode) -> CargoResult> { + let mut state = self.state.borrow_mut(); + state.lock(config, mode)?; + Ok(CacheLock { mode, locker: self }) + } + + /// Acquires a lock with the given mode, returning `None` if another cargo + /// is holding the lock. + pub fn try_lock( + &self, + config: &Config, + mode: CacheLockMode, + ) -> CargoResult>> { + let mut state = self.state.borrow_mut(); + if state.try_lock(config, mode)? { + Ok(Some(CacheLock { mode, locker: self })) + } else { + Ok(None) + } + } + + /// Returns whether or not a lock is held for the given mode in this locker. + /// + /// This does not tell you whether or not it is locked in some other + /// locker (such as in another process). + /// + /// Note that `Shared` will return true if a `MutateExclusive` lock is + /// held, since `MutateExclusive` is just an upgraded `Shared`. Likewise, + /// `DownlaodExclusive` 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(); + match ( + mode, + state.cache_lock_count, + state.mutate_lock_count, + state.mutate_is_exclusive, + ) { + (CacheLockMode::Shared, _, 1.., _) => true, + (CacheLockMode::MutateExclusive, _, 1.., true) => true, + (CacheLockMode::DownloadExclusive, 1.., _, _) => true, + _ => false, + } + } +} + +impl CacheState { + fn lock(&mut self, config: &Config, mode: CacheLockMode) -> CargoResult<()> { + do_lock!(self, config, mode, do_shared_lock, do_exclusive_lock); + Ok(()) + } + + fn try_lock(&mut self, config: &Config, mode: CacheLockMode) -> CargoResult { + do_lock!( + self, + config, + mode, + do_try_shared_lock, + do_try_exclusive_lock + ); + Ok(true) + } +} + +/// Returns whether or not the error appears to be from a read-only filesystem. +fn maybe_readonly(err: &anyhow::Error) -> bool { + err.chain().any(|err| { + if let Some(io) = err.downcast_ref::() { + if io.kind() == io::ErrorKind::PermissionDenied { + return true; + } + + #[cfg(unix)] + return io.raw_os_error() == Some(libc::EROFS); + } + + false + }) +} + +impl Drop for CacheLock<'_> { + fn drop(&mut self) { + use CacheLockMode::*; + let mut state = self.locker.state.borrow_mut(); + match self.mode { + Shared => { + decrement!(state, mutate_lock_count, mutate_lock); + } + DownloadExclusive => { + decrement!(state, cache_lock_count, cache_lock); + } + MutateExclusive => { + decrement!(state, cache_lock_count, cache_lock); + decrement!(state, mutate_lock_count, mutate_lock); + } + } + } +} diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index b87f98afdf6..53a0ba38b27 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -49,6 +49,7 @@ //! translate from `ConfigValue` and environment variables to the caller's //! desired type. +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}; @@ -58,7 +59,7 @@ use std::ffi::{OsStr, OsString}; use std::fmt; use std::fs::{self, File}; use std::io::prelude::*; -use std::io::{self, SeekFrom}; +use std::io::SeekFrom; use std::mem; use std::path::{Path, PathBuf}; use std::str::FromStr; @@ -78,7 +79,7 @@ use crate::util::network::http::http_handle; use crate::util::toml as cargo_toml; use crate::util::{internal, CanonicalUrl}; use crate::util::{try_canonicalize, validate_package_name}; -use crate::util::{FileLock, Filesystem, IntoUrl, IntoUrlWithBase, Rustc}; +use crate::util::{Filesystem, IntoUrl, IntoUrlWithBase, Rustc}; use anyhow::{anyhow, bail, format_err, Context as _}; use cargo_credential::Secret; use cargo_util::paths; @@ -215,9 +216,8 @@ pub struct Config { credential_cache: LazyCell>>, /// Cache of registry config from from the `[registries]` table. registry_config: LazyCell>>>, - /// Lock, if held, of the global package cache along with the number of - /// acquisitions so far. - package_cache_lock: RefCell, usize)>>, + /// Locks on the package and index caches. + package_cache_lock: CacheLocker, /// Cached configuration parsed by Cargo http_config: LazyCell, future_incompat_config: LazyCell, @@ -307,7 +307,7 @@ impl Config { updated_sources: LazyCell::new(), credential_cache: LazyCell::new(), registry_config: LazyCell::new(), - package_cache_lock: RefCell::new(None), + package_cache_lock: CacheLocker::new(), http_config: LazyCell::new(), future_incompat_config: LazyCell::new(), net_config: LazyCell::new(), @@ -1876,10 +1876,20 @@ impl Config { T::deserialize(d).map_err(|e| e.into()) } - pub fn assert_package_cache_locked<'a>(&self, f: &'a Filesystem) -> &'a Path { + /// Obtain a [`Path`] from a [`Filesystem`], verifying that the + /// appropriate lock is already currently held. + /// + /// Locks are usually acquired via [`Config::acquire_package_cache_lock`] + /// or [`Config::try_acquire_package_cache_lock`]. + #[track_caller] + pub fn assert_package_cache_locked<'a>( + &self, + mode: CacheLockMode, + f: &'a Filesystem, + ) -> &'a Path { let ret = f.as_path_unlocked(); assert!( - self.package_cache_lock.borrow().is_some(), + self.package_cache_lock.is_locked(mode), "package cache lock is not currently held, Cargo forgot to call \ `acquire_package_cache_lock` before we got to this stack frame", ); @@ -1887,72 +1897,26 @@ impl Config { ret } - /// Acquires an exclusive lock on the global "package cache" + /// Acquires a lock on the global "package cache", blocking if another + /// cargo holds the lock. /// - /// This lock is global per-process and can be acquired recursively. An RAII - /// structure is returned to release the lock, and if this process - /// abnormally terminates the lock is also released. - pub fn acquire_package_cache_lock(&self) -> CargoResult> { - let mut slot = self.package_cache_lock.borrow_mut(); - match *slot { - // We've already acquired the lock in this process, so simply bump - // the count and continue. - Some((_, ref mut cnt)) => { - *cnt += 1; - } - None => { - let path = ".package-cache"; - let desc = "package cache"; - - // First, attempt to open an exclusive lock which is in general - // the purpose of this lock! - // - // If that fails because of a readonly filesystem or a - // permission error, though, then we don't really want to fail - // just because of this. All files that this lock protects are - // in subfolders, so they're assumed by Cargo to also be - // readonly or have invalid permissions for us to write to. If - // that's the case, then we don't really need to grab a lock in - // the first place here. - // - // Despite this we attempt to grab a readonly lock. This means - // that if our read-only folder is shared read-write with - // someone else on the system we should synchronize with them, - // but if we can't even do that then we did our best and we just - // keep on chugging elsewhere. - match self.home_path.open_rw(path, self, desc) { - Ok(lock) => *slot = Some((Some(lock), 1)), - Err(e) => { - if maybe_readonly(&e) { - let lock = self.home_path.open_ro(path, self, desc).ok(); - *slot = Some((lock, 1)); - return Ok(PackageCacheLock(self)); - } - - Err(e).with_context(|| "failed to acquire package cache lock")?; - } - } - } - } - return Ok(PackageCacheLock(self)); - - fn maybe_readonly(err: &anyhow::Error) -> bool { - err.chain().any(|err| { - if let Some(io) = err.downcast_ref::() { - if io.kind() == io::ErrorKind::PermissionDenied { - return true; - } - - #[cfg(unix)] - return io.raw_os_error() == Some(libc::EROFS); - } - - false - }) - } + /// See [`crate::util::cache_lock`] for an in-depth discussion of locking + /// and lock modes. + pub fn acquire_package_cache_lock(&self, mode: CacheLockMode) -> CargoResult> { + self.package_cache_lock.lock(self, mode) } - pub fn release_package_cache_lock(&self) {} + /// Acquires a lock on the global "package cache", returning `None` if + /// another cargo holds the lock. + /// + /// See [`crate::util::cache_lock`] for an in-depth discussion of locking + /// and lock modes. + pub fn try_acquire_package_cache_lock( + &self, + mode: CacheLockMode, + ) -> CargoResult>> { + self.package_cache_lock.try_lock(self, mode) + } } /// Internal error for serde errors. @@ -2390,19 +2354,6 @@ pub fn save_credentials( } } -pub struct PackageCacheLock<'a>(&'a Config); - -impl Drop for PackageCacheLock<'_> { - fn drop(&mut self) { - let mut slot = self.0.package_cache_lock.borrow_mut(); - let (_, cnt) = slot.as_mut().unwrap(); - *cnt -= 1; - if *cnt == 0 { - *slot = None; - } - } -} - #[derive(Debug, Default, Deserialize, PartialEq)] #[serde(rename_all = "kebab-case")] pub struct CargoHttpConfig { diff --git a/src/cargo/util/flock.rs b/src/cargo/util/flock.rs index aa056c96579..7ae58f13276 100644 --- a/src/cargo/util/flock.rs +++ b/src/cargo/util/flock.rs @@ -14,14 +14,6 @@ use sys::*; pub struct FileLock { f: Option, path: PathBuf, - state: State, -} - -#[derive(PartialEq, Debug)] -enum State { - Unlocked, - Shared, - Exclusive, } impl FileLock { @@ -35,13 +27,11 @@ impl FileLock { /// Note that special care must be taken to ensure that the path is not /// referenced outside the lifetime of this lock. pub fn path(&self) -> &Path { - assert_ne!(self.state, State::Unlocked); &self.path } /// Returns the parent path containing this file pub fn parent(&self) -> &Path { - assert_ne!(self.state, State::Unlocked); self.path.parent().unwrap() } @@ -91,9 +81,9 @@ impl Write for FileLock { impl Drop for FileLock { fn drop(&mut self) { - if self.state != State::Unlocked { - if let Some(f) = self.f.take() { - let _ = unlock(&f); + if let Some(f) = self.f.take() { + if let Err(e) = unlock(&f) { + tracing::warn!("failed to release lock: {e:?}"); } } } @@ -171,13 +161,25 @@ impl Filesystem { where P: AsRef, { - self.open( - path.as_ref(), - OpenOptions::new().read(true).write(true).create(true), - State::Exclusive, - config, - msg, - ) + let mut opts = OpenOptions::new(); + opts.read(true).write(true).create(true); + let (path, f) = self.open(path.as_ref(), &opts, true)?; + self.lock(path, f, config, msg, true) + } + + /// A non-blocking version of [`Filesystem::open_rw`]. + /// + /// Returns `None` if the operation would block due to another process + /// holding the lock. + pub fn try_open_rw>(&self, path: P) -> CargoResult> { + let mut opts = OpenOptions::new(); + opts.read(true).write(true).create(true); + let (path, f) = self.open(path.as_ref(), &opts, true)?; + if try_acquire(&path, &|| try_lock_exclusive(&f))? { + Ok(Some(FileLock { f: Some(f), path })) + } else { + Ok(None) + } } /// Opens shared access to a file, returning the locked version of a file. @@ -193,32 +195,50 @@ impl Filesystem { where P: AsRef, { - self.open( - path.as_ref(), - OpenOptions::new().read(true), - State::Shared, - config, - msg, - ) + let (path, f) = self.open(path.as_ref(), &OpenOptions::new().read(true), false)?; + self.lock(path, f, config, msg, false) } - fn open( + /// Opens shared access to a file, returning the locked version of a file. + /// + /// Compared to [`Filesystem::open_ro`], this will create the file (and + /// any directories in the parent) if the file does not already exist. + pub fn open_shared_create>( &self, - path: &Path, - opts: &OpenOptions, - state: State, + path: P, config: &Config, msg: &str, ) -> CargoResult { - let path = self.root.join(path); + let mut opts = OpenOptions::new(); + opts.read(true).write(true).create(true); + let (path, f) = self.open(path.as_ref(), &opts, true)?; + self.lock(path, f, config, msg, false) + } - // If we want an exclusive lock then if we fail because of NotFound it's - // likely because an intermediate directory didn't exist, so try to - // create the directory and then continue. + /// A non-blocking version of [`Filesystem::open_shared_create`]. + /// + /// Returns `None` if the operation would block due to another process + /// holding the lock. + pub fn try_open_shared_create>(&self, path: P) -> CargoResult> { + let mut opts = OpenOptions::new(); + opts.read(true).write(true).create(true); + let (path, f) = self.open(path.as_ref(), &opts, true)?; + if try_acquire(&path, &|| try_lock_shared(&f))? { + Ok(Some(FileLock { f: Some(f), path })) + } else { + Ok(None) + } + } + + fn open(&self, path: &Path, opts: &OpenOptions, create: bool) -> CargoResult<(PathBuf, File)> { + let path = self.root.join(path); let f = opts .open(&path) .or_else(|e| { - if e.kind() == io::ErrorKind::NotFound && state == State::Exclusive { + // If we were requested to create this file, and there was a + // NotFound error, then that was likely due to missing + // intermediate directories. Try creating them and try again. + if e.kind() == io::ErrorKind::NotFound && create { paths::create_dir_all(path.parent().unwrap())?; Ok(opts.open(&path)?) } else { @@ -226,24 +246,27 @@ impl Filesystem { } }) .with_context(|| format!("failed to open: {}", path.display()))?; - match state { - State::Exclusive => { - acquire(config, msg, &path, &|| try_lock_exclusive(&f), &|| { - lock_exclusive(&f) - })?; - } - State::Shared => { - acquire(config, msg, &path, &|| try_lock_shared(&f), &|| { - lock_shared(&f) - })?; - } - State::Unlocked => {} + Ok((path, f)) + } + + fn lock( + &self, + path: PathBuf, + f: File, + config: &Config, + msg: &str, + exclusive: bool, + ) -> CargoResult { + if exclusive { + acquire(config, msg, &path, &|| try_lock_exclusive(&f), &|| { + lock_exclusive(&f) + })?; + } else { + acquire(config, msg, &path, &|| try_lock_shared(&f), &|| { + lock_shared(&f) + })?; } - Ok(FileLock { - f: Some(f), - path, - state, - }) + Ok(FileLock { f: Some(f), path }) } } @@ -259,28 +282,7 @@ impl PartialEq for Path { } } -/// Acquires a lock on a file in a "nice" manner. -/// -/// Almost all long-running blocking actions in Cargo have a status message -/// associated with them as we're not sure how long they'll take. Whenever a -/// conflicted file lock happens, this is the case (we're not sure when the lock -/// will be released). -/// -/// This function will acquire the lock on a `path`, printing out a nice message -/// to the console if we have to wait for it. It will first attempt to use `try` -/// to acquire a lock on the crate, and in the case of contention it will emit a -/// status message based on `msg` to `config`'s shell, and then use `block` to -/// block waiting to acquire a lock. -/// -/// Returns an error if the lock could not be acquired or if any error other -/// than a contention error happens. -fn acquire( - config: &Config, - msg: &str, - path: &Path, - lock_try: &dyn Fn() -> io::Result<()>, - lock_block: &dyn Fn() -> io::Result<()>, -) -> CargoResult<()> { +fn try_acquire(path: &Path, lock_try: &dyn Fn() -> io::Result<()>) -> CargoResult { // File locking on Unix is currently implemented via `flock`, which is known // to be broken on NFS. We could in theory just ignore errors that happen on // NFS, but apparently the failure mode [1] for `flock` on NFS is **blocking @@ -292,16 +294,17 @@ fn acquire( // // [1]: https://github.com/rust-lang/cargo/issues/2615 if is_on_nfs_mount(path) { - return Ok(()); + tracing::debug!("{path:?} appears to be an NFS mount, not trying to lock"); + return Ok(true); } match lock_try() { - Ok(()) => return Ok(()), + Ok(()) => return Ok(true), // In addition to ignoring NFS which is commonly not working we also // just ignore locking on filesystems that look like they don't // implement file locking. - Err(e) if error_unsupported(&e) => return Ok(()), + Err(e) if error_unsupported(&e) => return Ok(true), Err(e) => { if !error_contended(&e) { @@ -311,36 +314,64 @@ fn acquire( } } } + Ok(false) +} + +/// Acquires a lock on a file in a "nice" manner. +/// +/// Almost all long-running blocking actions in Cargo have a status message +/// associated with them as we're not sure how long they'll take. Whenever a +/// conflicted file lock happens, this is the case (we're not sure when the lock +/// will be released). +/// +/// This function will acquire the lock on a `path`, printing out a nice message +/// to the console if we have to wait for it. It will first attempt to use `try` +/// to acquire a lock on the crate, and in the case of contention it will emit a +/// status message based on `msg` to `config`'s shell, and then use `block` to +/// block waiting to acquire a lock. +/// +/// Returns an error if the lock could not be acquired or if any error other +/// than a contention error happens. +fn acquire( + config: &Config, + msg: &str, + path: &Path, + lock_try: &dyn Fn() -> io::Result<()>, + lock_block: &dyn Fn() -> io::Result<()>, +) -> CargoResult<()> { + if try_acquire(path, lock_try)? { + return Ok(()); + } let msg = format!("waiting for file lock on {}", msg); config .shell() .status_with_color("Blocking", &msg, &style::NOTE)?; lock_block().with_context(|| format!("failed to lock file: {}", path.display()))?; - return Ok(()); + Ok(()) +} - #[cfg(all(target_os = "linux", not(target_env = "musl")))] - fn is_on_nfs_mount(path: &Path) -> bool { - use std::ffi::CString; - use std::mem; - use std::os::unix::prelude::*; +#[cfg(all(target_os = "linux", not(target_env = "musl")))] +fn is_on_nfs_mount(path: &Path) -> bool { + use std::ffi::CString; + use std::mem; + use std::os::unix::prelude::*; - let Ok(path) = CString::new(path.as_os_str().as_bytes()) else { - return false; - }; + let Ok(path) = CString::new(path.as_os_str().as_bytes()) else { + return false; + }; - unsafe { - let mut buf: libc::statfs = mem::zeroed(); - let r = libc::statfs(path.as_ptr(), &mut buf); + unsafe { + let mut buf: libc::statfs = mem::zeroed(); + let r = libc::statfs(path.as_ptr(), &mut buf); - r == 0 && buf.f_type as u32 == libc::NFS_SUPER_MAGIC as u32 - } + r == 0 && buf.f_type as u32 == libc::NFS_SUPER_MAGIC as u32 } +} - #[cfg(any(not(target_os = "linux"), target_env = "musl"))] - fn is_on_nfs_mount(_path: &Path) -> bool { - false - } +#[cfg(any(not(target_os = "linux"), target_env = "musl"))] +fn is_on_nfs_mount(_path: &Path) -> bool { + false } #[cfg(unix)] diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index b4d14f038fc..e73dc42be1f 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -31,6 +31,7 @@ pub use self::workspace::{ }; pub mod auth; +pub mod cache_lock; mod canonical_url; pub mod command_prelude; pub mod config; diff --git a/tests/testsuite/cache_lock.rs b/tests/testsuite/cache_lock.rs new file mode 100644 index 00000000000..c5eaeb2d65b --- /dev/null +++ b/tests/testsuite/cache_lock.rs @@ -0,0 +1,302 @@ +//! Tests for `CacheLock`. + +use crate::config::ConfigBuilder; +use cargo::util::cache_lock::{CacheLockMode, CacheLocker}; +use cargo_test_support::paths::{self, CargoPathExt}; +use cargo_test_support::{retry, thread_wait_timeout, threaded_timeout}; +use std::thread::JoinHandle; + +/// Helper to verify that it is OK to acquire the given lock (it shouldn't block). +fn verify_lock_is_ok(mode: CacheLockMode) { + let root = paths::root(); + threaded_timeout(10, move || { + let config = ConfigBuilder::new().root(root).build(); + let locker = CacheLocker::new(); + // This would block if it is held. + let _lock = locker.lock(&config, mode).unwrap(); + assert!(locker.is_locked(mode)); + }); +} + +/// Helper to acquire two locks from the same locker. +fn a_b_nested(a: CacheLockMode, b: CacheLockMode) { + let config = ConfigBuilder::new().build(); + let locker = CacheLocker::new(); + let lock1 = locker.lock(&config, a).unwrap(); + assert!(locker.is_locked(a)); + let lock2 = locker.lock(&config, b).unwrap(); + assert!(locker.is_locked(b)); + drop(lock2); + drop(lock1); + // Verify locks were unlocked. + verify_lock_is_ok(CacheLockMode::Shared); + verify_lock_is_ok(CacheLockMode::DownloadExclusive); + verify_lock_is_ok(CacheLockMode::MutateExclusive); +} + +/// Helper to acquire two locks from separate lockers, verifying that they +/// don't block each other. +fn a_then_b_separate_not_blocked(a: CacheLockMode, b: CacheLockMode, verify: CacheLockMode) { + let config = ConfigBuilder::new().build(); + let locker1 = CacheLocker::new(); + let lock1 = locker1.lock(&config, a).unwrap(); + assert!(locker1.is_locked(a)); + let locker2 = CacheLocker::new(); + let lock2 = locker2.lock(&config, b).unwrap(); + assert!(locker2.is_locked(b)); + let thread = verify_lock_would_block(verify); + // Unblock the thread. + drop(lock1); + drop(lock2); + // Verify the thread is unblocked. + thread_wait_timeout::<()>(100, thread); +} + +/// Helper to acquire two locks from separate lockers, verifying that the +/// second one blocks. +fn a_then_b_separate_blocked(a: CacheLockMode, b: CacheLockMode) { + let config = ConfigBuilder::new().build(); + let locker = CacheLocker::new(); + let lock = locker.lock(&config, a).unwrap(); + assert!(locker.is_locked(a)); + let thread = verify_lock_would_block(b); + // Unblock the thread. + drop(lock); + // Verify the thread is unblocked. + thread_wait_timeout::<()>(100, thread); +} + +/// Helper to verify that acquiring the given mode would block. +/// +/// Always call `thread_wait_timeout` on the result. +#[must_use] +fn verify_lock_would_block(mode: CacheLockMode) -> JoinHandle<()> { + let root = paths::root(); + // Spawn a thread that will block on the lock. + let thread = std::thread::spawn(move || { + let config = ConfigBuilder::new().root(root).build(); + let locker2 = CacheLocker::new(); + let lock2 = locker2.lock(&config, mode).unwrap(); + assert!(locker2.is_locked(mode)); + drop(lock2); + }); + // Verify that it blocked. + retry(100, || { + if let Ok(s) = std::fs::read_to_string(paths::root().join("shell.out")) { + if s.trim().starts_with("Blocking waiting for file lock on") { + return Some(()); + } else { + eprintln!("unexpected output: {s}"); + // Try again, it might have been partially written. + } + } + None + }); + thread +} + +#[test] +fn new_is_unlocked() { + let locker = CacheLocker::new(); + assert!(!locker.is_locked(CacheLockMode::Shared)); + assert!(!locker.is_locked(CacheLockMode::DownloadExclusive)); + assert!(!locker.is_locked(CacheLockMode::MutateExclusive)); +} + +#[cargo_test] +fn multiple_shared() { + // Test that two nested shared locks from the same locker are safe to acquire. + a_b_nested(CacheLockMode::Shared, CacheLockMode::Shared); +} + +#[cargo_test] +fn multiple_shared_separate() { + // Test that two independent shared locks are safe to acquire at the same time. + a_then_b_separate_not_blocked( + CacheLockMode::Shared, + CacheLockMode::Shared, + CacheLockMode::MutateExclusive, + ); +} + +#[cargo_test] +fn multiple_download() { + // That that two nested download locks from the same locker are safe to acquire. + a_b_nested( + CacheLockMode::DownloadExclusive, + CacheLockMode::DownloadExclusive, + ); +} + +#[cargo_test] +fn multiple_mutate() { + // That that two nested mutate locks from the same locker are safe to acquire. + a_b_nested( + CacheLockMode::MutateExclusive, + CacheLockMode::MutateExclusive, + ); +} + +#[cargo_test] +#[should_panic(expected = "lock is not allowed")] +fn download_then_shared() { + // This sequence is not supported. + a_b_nested(CacheLockMode::DownloadExclusive, CacheLockMode::Shared); +} + +#[cargo_test] +#[should_panic(expected = "Shared to MutateExclusive not supported")] +fn shared_then_mutate() { + // This sequence is not supported. + a_b_nested(CacheLockMode::Shared, CacheLockMode::MutateExclusive); +} + +#[cargo_test] +fn shared_then_download() { + a_b_nested(CacheLockMode::Shared, CacheLockMode::DownloadExclusive); + // Verify drop actually unlocked. + verify_lock_is_ok(CacheLockMode::DownloadExclusive); + verify_lock_is_ok(CacheLockMode::MutateExclusive); +} + +#[cargo_test] +fn mutate_then_shared() { + a_b_nested(CacheLockMode::MutateExclusive, CacheLockMode::Shared); + // Verify drop actually unlocked. + verify_lock_is_ok(CacheLockMode::MutateExclusive); +} + +#[cargo_test] +fn download_then_mutate() { + a_b_nested( + CacheLockMode::DownloadExclusive, + CacheLockMode::MutateExclusive, + ); + // Verify drop actually unlocked. + verify_lock_is_ok(CacheLockMode::DownloadExclusive); + verify_lock_is_ok(CacheLockMode::MutateExclusive); +} + +#[cargo_test] +fn mutate_then_download() { + a_b_nested( + CacheLockMode::MutateExclusive, + CacheLockMode::DownloadExclusive, + ); + // Verify drop actually unlocked. + verify_lock_is_ok(CacheLockMode::MutateExclusive); + verify_lock_is_ok(CacheLockMode::DownloadExclusive); +} + +#[cargo_test] +fn readonly() { + // In a permission denied situation, it should still allow a lock. It just + // silently behaves as-if it was locked. + let cargo_home = paths::home().join(".cargo"); + std::fs::create_dir_all(&cargo_home).unwrap(); + let mut perms = std::fs::metadata(&cargo_home).unwrap().permissions(); + perms.set_readonly(true); + std::fs::set_permissions(&cargo_home, perms).unwrap(); + let config = ConfigBuilder::new().build(); + let locker = CacheLocker::new(); + for mode in [ + CacheLockMode::Shared, + CacheLockMode::DownloadExclusive, + CacheLockMode::MutateExclusive, + ] { + let _lock = locker.lock(&config, mode).unwrap(); + } +} + +#[cargo_test] +fn download_then_shared_separate() { + a_then_b_separate_not_blocked( + CacheLockMode::DownloadExclusive, + CacheLockMode::Shared, + CacheLockMode::MutateExclusive, + ); +} + +#[cargo_test] +fn shared_then_download_separate() { + a_then_b_separate_not_blocked( + CacheLockMode::Shared, + CacheLockMode::DownloadExclusive, + CacheLockMode::MutateExclusive, + ); +} + +#[cargo_test] +fn multiple_download_separate() { + // Test that with two independent download locks, the second blocks until + // the first is released. + a_then_b_separate_blocked( + CacheLockMode::DownloadExclusive, + CacheLockMode::DownloadExclusive, + ); +} + +#[cargo_test] +fn multiple_mutate_separate() { + // Test that with two independent mutate locks, the second blocks until + // the first is released. + a_then_b_separate_blocked( + CacheLockMode::MutateExclusive, + CacheLockMode::MutateExclusive, + ); +} + +#[cargo_test] +fn shared_then_mutate_separate() { + a_then_b_separate_blocked(CacheLockMode::Shared, CacheLockMode::MutateExclusive); +} + +#[cargo_test] +fn download_then_mutate_separate() { + a_then_b_separate_blocked( + CacheLockMode::DownloadExclusive, + CacheLockMode::MutateExclusive, + ); +} + +#[cargo_test] +fn mutate_then_download_separate() { + a_then_b_separate_blocked( + CacheLockMode::MutateExclusive, + CacheLockMode::DownloadExclusive, + ); +} + +#[cargo_test] +fn mutate_then_shared_separate() { + a_then_b_separate_blocked(CacheLockMode::MutateExclusive, CacheLockMode::Shared); +} + +#[cargo_test(ignore_windows = "no method to prevent creating or locking a file")] +fn mutate_err_is_atomic() { + // Verifies that when getting a mutate lock, that if the first lock + // succeeds, but the second one fails, that the first lock is released. + let config = ConfigBuilder::new().build(); + let locker = CacheLocker::new(); + let cargo_home = config.home().as_path_unlocked(); + let cache_path = cargo_home.join(".package-cache"); + // This is a hacky way to force an error acquiring the download lock. By + // making it a directory, it is unable to open it. + // TODO: Unfortunately this doesn't work on Windows. I don't have any + // ideas on how to simulate an error on Windows. + cache_path.mkdir_p(); + match locker.lock(&config, CacheLockMode::MutateExclusive) { + Ok(_) => panic!("did not expect lock to succeed"), + Err(e) => { + let msg = format!("{e:?}"); + assert!(msg.contains("failed to open:"), "{msg}"); + } + } + assert!(!locker.is_locked(CacheLockMode::MutateExclusive)); + assert!(!locker.is_locked(CacheLockMode::DownloadExclusive)); + assert!(!locker.is_locked(CacheLockMode::Shared)); + cache_path.rm_rf(); + verify_lock_is_ok(CacheLockMode::DownloadExclusive); + verify_lock_is_ok(CacheLockMode::Shared); + verify_lock_is_ok(CacheLockMode::MutateExclusive); +} diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 8279f58180f..d31255d0ae7 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -17,6 +17,7 @@ mod build_plan; mod build_script; mod build_script_env; mod build_script_extra_link_arg; +mod cache_lock; mod cache_messages; mod cargo; mod cargo_add; diff --git a/tests/testsuite/search.rs b/tests/testsuite/search.rs index 4c3155c8fb2..c76397ac709 100644 --- a/tests/testsuite/search.rs +++ b/tests/testsuite/search.rs @@ -1,5 +1,6 @@ //! Tests for the `cargo search` command. +use cargo::util::cache_lock::CacheLockMode; use cargo_test_support::cargo_process; use cargo_test_support::paths; use cargo_test_support::registry::{RegistryBuilder, Response}; @@ -100,7 +101,9 @@ fn not_update() { paths::root(), paths::home().join(".cargo"), ); - let lock = cfg.acquire_package_cache_lock().unwrap(); + let lock = cfg + .acquire_package_cache_lock(CacheLockMode::DownloadExclusive) + .unwrap(); let mut regsrc = RegistrySource::remote(sid, &HashSet::new(), &cfg).unwrap(); regsrc.invalidate_cache(); regsrc.block_until_ready().unwrap(); From 907bc0f777a6bdfc2eb0d225924afe7134934224 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 19 Sep 2023 19:17:56 -0700 Subject: [PATCH 05/10] Rearrange `CacheLockMode` order. --- src/cargo/util/cache_lock.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/cargo/util/cache_lock.rs b/src/cargo/util/cache_lock.rs index f283df3bc66..fc379ae2426 100644 --- a/src/cargo/util/cache_lock.rs +++ b/src/cargo/util/cache_lock.rs @@ -70,15 +70,6 @@ use std::io; /// The style of lock to acquire. #[derive(Copy, Clone, Debug)] pub enum CacheLockMode { - /// A `Shared` lock allows multiple cargos to read from the source files. - /// - /// If another cargo has a `MutateExclusive` lock, then an attempt to get - /// a `Shared` will block. - /// - /// If another cargo has a `DownloadExclusive` lock, then the both can - /// operate concurrently under the assumption that downloading does not - /// modify existing source files. - Shared, /// A `DownloadExclusive` lock ensures that only one cargo is downloading /// new packages. /// @@ -88,6 +79,15 @@ pub enum CacheLockMode { /// If another cargo has a `Shared` lock, then both can operate /// concurrently. DownloadExclusive, + /// A `Shared` lock allows multiple cargos to read from the source files. + /// + /// If another cargo has a `MutateExclusive` lock, then an attempt to get + /// a `Shared` will block. + /// + /// If another cargo has a `DownloadExclusive` lock, then the both can + /// operate concurrently under the assumption that downloading does not + /// modify existing source files. + Shared, /// A `MutateExclusive` lock ensures no other cargo is reading or writing /// from the package caches. /// From 1f0e483c669608c046e72c362ecfda11c04ca0cd Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 19 Sep 2023 19:18:26 -0700 Subject: [PATCH 06/10] Inline `lock` method. --- src/cargo/util/flock.rs | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/src/cargo/util/flock.rs b/src/cargo/util/flock.rs index 7ae58f13276..a1dc1c37e42 100644 --- a/src/cargo/util/flock.rs +++ b/src/cargo/util/flock.rs @@ -164,7 +164,10 @@ impl Filesystem { let mut opts = OpenOptions::new(); opts.read(true).write(true).create(true); let (path, f) = self.open(path.as_ref(), &opts, true)?; - self.lock(path, f, config, msg, true) + acquire(config, msg, &path, &|| try_lock_exclusive(&f), &|| { + lock_exclusive(&f) + })?; + Ok(FileLock { f: Some(f), path }) } /// A non-blocking version of [`Filesystem::open_rw`]. @@ -196,7 +199,10 @@ impl Filesystem { P: AsRef, { let (path, f) = self.open(path.as_ref(), &OpenOptions::new().read(true), false)?; - self.lock(path, f, config, msg, false) + acquire(config, msg, &path, &|| try_lock_shared(&f), &|| { + lock_shared(&f) + })?; + Ok(FileLock { f: Some(f), path }) } /// Opens shared access to a file, returning the locked version of a file. @@ -212,7 +218,10 @@ impl Filesystem { let mut opts = OpenOptions::new(); opts.read(true).write(true).create(true); let (path, f) = self.open(path.as_ref(), &opts, true)?; - self.lock(path, f, config, msg, false) + acquire(config, msg, &path, &|| try_lock_shared(&f), &|| { + lock_shared(&f) + })?; + Ok(FileLock { f: Some(f), path }) } /// A non-blocking version of [`Filesystem::open_shared_create`]. @@ -248,26 +257,6 @@ impl Filesystem { .with_context(|| format!("failed to open: {}", path.display()))?; Ok((path, f)) } - - fn lock( - &self, - path: PathBuf, - f: File, - config: &Config, - msg: &str, - exclusive: bool, - ) -> CargoResult { - if exclusive { - acquire(config, msg, &path, &|| try_lock_exclusive(&f), &|| { - lock_exclusive(&f) - })?; - } else { - acquire(config, msg, &path, &|| try_lock_shared(&f), &|| { - lock_shared(&f) - })?; - } - Ok(FileLock { f: Some(f), path }) - } } impl PartialEq for Filesystem { From fd6185193aced70248bd79308009971a6f4528f5 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 23 Sep 2023 14:21:57 -0700 Subject: [PATCH 07/10] Rewrite the cache locker to use functions instead of macros. --- src/cargo/util/cache_lock.rs | 539 ++++++++++++++++++---------------- tests/testsuite/cache_lock.rs | 6 +- 2 files changed, 293 insertions(+), 252 deletions(-) diff --git a/src/cargo/util/cache_lock.rs b/src/cargo/util/cache_lock.rs index fc379ae2426..2226ae5c569 100644 --- a/src/cargo/util/cache_lock.rs +++ b/src/cargo/util/cache_lock.rs @@ -68,10 +68,10 @@ use std::cell::RefCell; use std::io; /// The style of lock to acquire. -#[derive(Copy, Clone, Debug)] +#[derive(Copy, Clone, Debug, PartialEq)] pub enum CacheLockMode { - /// A `DownloadExclusive` lock ensures that only one cargo is downloading - /// new packages. + /// A `DownloadExclusive` lock ensures that only one cargo is doing + /// resolution and downloading new packages. /// /// If another cargo has a `MutateExclusive` lock, then an attempt to get /// a `DownloadExclusive` lock will block. @@ -99,265 +99,259 @@ pub enum CacheLockMode { /// A locker that can be used to acquire locks. #[derive(Debug)] pub struct CacheLocker { + /// The state of the locker. + /// + /// [`CacheLocker`] uses interior mutability because it is stuffed inside + /// the global `Config`, which does not allow mutation. state: RefCell, } -/// The state of the [`CacheLocker`]. -/// -/// [`CacheLocker`] uses interior mutability because it is stuffed inside the -/// global `Config`, which does not allow mutation. -/// -/// An important note is that locks can be `None` even when they are held. -/// This can happen on things like old NFS mounts where locking isn't -/// supported. We otherwise pretend we have a lock via the lock counts. See -/// [`FileLock`] for more detail on that. -#[derive(Debug, Default)] -struct CacheState { - cache_lock: Option, - cache_lock_count: u32, - mutate_lock: Option, - mutate_lock_count: u32, - /// Indicator of whether or not `mutate_lock` is currently a shared lock - /// or an exclusive one. - mutate_is_exclusive: bool, +/// Whether or not a lock attempt should block. +#[derive(Copy, Clone)] +enum BlockingMode { + Blocking, + NonBlocking, } -/// A held lock guard. -/// -/// When this is dropped, the lock will be released. +use BlockingMode::*; + +/// Whether or not a lock attempt blocked or succeeded. +#[derive(PartialEq, Copy, Clone)] #[must_use] -pub struct CacheLock<'lock> { - mode: CacheLockMode, - locker: &'lock CacheLocker, +enum LockingResult { + LockAcquired, + WouldBlock, } -/// The filename for the [`CacheLockMode::DownloadExclusive`] lock. -const CACHE_LOCK_NAME: &str = ".package-cache"; -/// The filename for the [`CacheLockMode::MutateExclusive`] and -/// [`CacheLockMode::Shared`] lock. -const MUTATE_NAME: &str = ".package-cache-mutate"; +use LockingResult::*; -// Descriptions that are displayed in the "Blocking" message shown to the user. -const SHARED_DESCR: &str = "shared package cache"; -const DOWNLOAD_EXCLUSIVE_DESCR: &str = "package cache"; -const MUTATE_EXCLUSIVE_DESCR: &str = "package cache mutation"; +/// A file lock, with a counter to assist with recursive locking. +#[derive(Debug)] +struct RecursiveLock { + /// The file lock. + /// + /// An important note is that locks can be `None` even when they are held. + /// This can happen on things like old NFS mounts where locking isn't + /// supported. We otherwise pretend we have a lock via the lock count. See + /// [`FileLock`] for more detail on that. + lock: Option, + /// Number locks held, to support recursive locking. + count: u32, + /// If this is `true`, it is an exclusive lock, otherwise it is shared. + is_exclusive: bool, + /// The filename of the lock. + filename: &'static str, +} + +impl RecursiveLock { + fn new(filename: &'static str) -> RecursiveLock { + RecursiveLock { + lock: None, + count: 0, + is_exclusive: false, + filename, + } + } -/// Macro to unlock a previously acquired lock. -macro_rules! decrement { - ($state:expr, $count:ident, $lock:ident) => { - let new_cnt = $state.$count.checked_sub(1).unwrap(); - $state.$count = new_cnt; + /// Low-level lock count increment routine. + fn increment(&mut self) { + self.count = self.count.checked_add(1).unwrap(); + } + + /// Unlocks a previously acquired lock. + fn decrement(&mut self) { + let new_cnt = self.count.checked_sub(1).unwrap(); + self.count = new_cnt; if new_cnt == 0 { // This will drop, releasing the lock. - $state.$lock = None; + self.lock = None; } - }; -} - -/// Macro for acquiring a shared lock (can block). -macro_rules! do_shared_lock { - ($self: ident, $config: expr, $to_set: ident, $lock_name: expr, $descr: expr) => { - $self.$to_set = match $config - .home() - .open_shared_create($lock_name, $config, $descr) - { - Ok(lock) => Some(lock), - Err(e) => { - // There is no error here because locking is mostly a - // best-effort attempt. If cargo home is read-only, we don't - // want to fail just because we couldn't create the lock file. - tracing::warn!("failed to acquire cache lock {}: {e:?}", $lock_name); - None - } - }; - }; -} + } -/// Macro for acquiring a shared lock without blocking. -macro_rules! do_try_shared_lock { - ($self: ident, $config: expr, $to_set: ident, $lock_name: expr, $unused: expr) => { - $self.$to_set = match $config.home().try_open_shared_create($lock_name) { - Ok(Some(lock)) => Some(lock), - Ok(None) => { - return Ok(false); - } - Err(e) => { - tracing::warn!("failed to acquire cache lock {}: {e:?}", $lock_name); - None + /// Acquires a shared lock. + fn lock_shared( + &mut self, + config: &Config, + description: &'static str, + blocking: BlockingMode, + ) -> LockingResult { + match blocking { + Blocking => { + self.lock_shared_blocking(config, description); + LockAcquired } - }; - }; -} + NonBlocking => self.lock_shared_nonblocking(config), + } + } -/// Macro for acquiring an exclusive lock (can block). -macro_rules! do_exclusive_lock { - ($self: ident, $config: expr, $to_set: ident, $lock_name: expr, $descr: expr) => { - match $config.home().open_rw($lock_name, $config, $descr) { - Ok(lock) => { - $self.$to_set = Some(lock); - Ok(()) - } - Err(e) => { - if maybe_readonly(&e) { - // This is a best-effort attempt to at least try to - // acquire some sort of lock. This can help in the - // situation where this cargo only has read-only access, - // but maybe some other cargo has read-write. This will at - // least attempt to coordinate with it. - // - // We don't want to fail on a read-only mount because - // cargo grabs an exclusive lock in situations where it - // may only be reading from the package cache. In that - // case, cargo isn't writing anything, and we don't want - // to fail on that. - do_shared_lock!($self, $config, $to_set, $lock_name, $descr); - Ok(()) - } else { - Err(e).with_context(|| "failed to acquire package cache lock") + /// Acquires a shared lock, blocking if held by another locker. + fn lock_shared_blocking(&mut self, config: &Config, description: &'static str) { + if self.count == 0 { + self.is_exclusive = false; + self.lock = match config + .home() + .open_shared_create(self.filename, config, description) + { + Ok(lock) => Some(lock), + Err(e) => { + // There is no error here because locking is mostly a + // best-effort attempt. If cargo home is read-only, we don't + // want to fail just because we couldn't create the lock file. + tracing::warn!("failed to acquire cache lock {}: {e:?}", self.filename); + None } - } + }; } - }; -} + self.increment(); + } -/// Macro for acquiring an exclusive lock without blocking. -macro_rules! do_try_exclusive_lock { - ($self: ident, $config: expr, $to_set: ident, $lock_name: expr, $unused: expr) => { - match $config.home().try_open_rw($lock_name) { - Ok(Some(lock)) => { - $self.$to_set = Some(lock); - Ok(()) - } - Ok(None) => return Ok(false), - Err(e) => { - if maybe_readonly(&e) { - do_try_shared_lock!($self, $config, $to_set, $lock_name, $unused); - Ok(()) - } else { - Err(e).with_context(|| "failed to acquire package cache lock") + /// Acquires a shared lock, returns [`WouldBlock`] if held by another locker. + fn lock_shared_nonblocking(&mut self, config: &Config) -> LockingResult { + if self.count == 0 { + self.is_exclusive = false; + self.lock = match config.home().try_open_shared_create(self.filename) { + Ok(Some(lock)) => Some(lock), + Ok(None) => { + return WouldBlock; } - } + Err(e) => { + // Pretend that the lock was acquired (see lock_shared_blocking). + tracing::warn!("failed to acquire cache lock {}: {e:?}", self.filename); + None + } + }; } - }; -} + self.increment(); + LockAcquired + } -/// Macro to help with acquiring a lock. -/// -/// `shared` and `exclusive` are inputs to the macro so that either the -/// blocking or non-blocking implementations can be called based on what the -/// caller wants. -macro_rules! do_lock { - ($self: ident, $config: expr, $mode: expr, $shared: ident, $exclusive: ident) => { - use CacheLockMode::*; - match ( - $mode, - &$self.cache_lock_count, - &$self.mutate_lock_count, - $self.mutate_is_exclusive, - ) { - (Shared, 0, 0, _) => { - // Shared lock, no locks currently held. - $shared!($self, $config, mutate_lock, MUTATE_NAME, SHARED_DESCR); - $self.mutate_lock_count += 1; - $self.mutate_is_exclusive = false; - } - (DownloadExclusive, 0, _, _) => { - // DownloadExclusive lock, no DownloadExclusive lock currently held. - $exclusive!( - $self, - $config, - cache_lock, - CACHE_LOCK_NAME, - DOWNLOAD_EXCLUSIVE_DESCR - )?; - $self.cache_lock_count += 1; + /// Acquires an exclusive lock. + fn lock_exclusive( + &mut self, + config: &Config, + description: &'static str, + blocking: BlockingMode, + ) -> CargoResult { + if self.count > 0 && !self.is_exclusive { + // Lock upgrades are dicey. It might be possible to support + // this but would take a bit of work, and so far it isn't + // needed. + panic!("lock upgrade from shared to exclusive not supported"); + } + match blocking { + Blocking => { + self.lock_exclusive_blocking(config, description)?; + Ok(LockAcquired) } - (MutateExclusive, 0, 0, _) => { - // MutateExclusive lock, no locks currently held. - $exclusive!( - $self, - $config, - mutate_lock, - MUTATE_NAME, - MUTATE_EXCLUSIVE_DESCR - )?; - $self.mutate_lock_count += 1; - $self.mutate_is_exclusive = true; + NonBlocking => self.lock_exclusive_nonblocking(config), + } + } - // Part of the contract of MutateExclusive is that it doesn't - // allow any processes to have a lock on the package cache, so - // this acquires both locks. - if let Err(e) = $exclusive!( - $self, - $config, - cache_lock, - CACHE_LOCK_NAME, - DOWNLOAD_EXCLUSIVE_DESCR - ) { - decrement!($self, mutate_lock_count, mutate_lock); - return Err(e); + /// Acquires an exclusive lock, blocking if held by another locker. + fn lock_exclusive_blocking( + &mut self, + config: &Config, + description: &'static str, + ) -> CargoResult<()> { + if self.count == 0 { + self.is_exclusive = true; + match config.home().open_rw(self.filename, config, description) { + Ok(lock) => self.lock = Some(lock), + Err(e) => { + if maybe_readonly(&e) { + // This is a best-effort attempt to at least try to + // acquire some sort of lock. This can help in the + // situation where this cargo only has read-only access, + // but maybe some other cargo has read-write. This will at + // least attempt to coordinate with it. + // + // We don't want to fail on a read-only mount because + // cargo grabs an exclusive lock in situations where it + // may only be reading from the package cache. In that + // case, cargo isn't writing anything, and we don't want + // to fail on that. + self.lock_shared_blocking(config, description); + // This has to pretend it is exclusive for recursive locks to work. + self.is_exclusive = true; + return Ok(()); + } else { + return Err(e).with_context(|| "failed to acquire package cache lock"); + } } - $self.cache_lock_count += 1; - } - (Shared, 1.., 0, _) => { - // Shared lock, when a DownloadExclusive is held. - // - // This isn't supported because it could cause a deadlock. If - // one cargo is attempting to acquire a MutateExclusive lock, - // and acquires the mutate lock, but is blocked on the - // download lock, and the cargo that holds the download lock - // attempts to get a shared lock, they would end up blocking - // each other. - panic!("shared lock while holding download lock is not allowed"); - } - (MutateExclusive, _, 1.., false) => { - // MutateExclusive lock, when a Shared lock is held. - // - // Lock upgrades are dicey. It might be possible to support - // this but would take a bit of work, and so far it isn't - // needed. - panic!("lock upgrade from Shared to MutateExclusive not supported"); - } - (Shared, _, 1.., _) => { - // Shared lock, when a Shared or MutateExclusive lock is held. - // - // MutateExclusive is more restrictive than Shared, so no need - // to do anything. - $self.mutate_lock_count = $self.mutate_lock_count.checked_add(1).unwrap(); } - (DownloadExclusive, 1.., _, _) => { - // DownloadExclusive lock, when another DownloadExclusive is held. - $self.cache_lock_count = $self.cache_lock_count.checked_add(1).unwrap(); - } - (MutateExclusive, _, 1.., true) => { - // MutateExclusive lock, when another MutateExclusive is held. - $self.cache_lock_count += 1; - $self.mutate_lock_count += 1; - } - (MutateExclusive, 1.., 0, _) => { - // MutateExclusive lock, when only a DownloadExclusive is held. - $exclusive!( - $self, - $config, - mutate_lock, - MUTATE_NAME, - MUTATE_EXCLUSIVE_DESCR - )?; - // Both of these need to be incremented to match the behavior - // in the drop impl. - $self.cache_lock_count += 1; - $self.mutate_lock_count += 1; - $self.mutate_is_exclusive = true; + } + self.increment(); + Ok(()) + } + + /// Acquires an exclusive lock, returns [`WouldBlock`] if held by another locker. + fn lock_exclusive_nonblocking(&mut self, config: &Config) -> CargoResult { + if self.count == 0 { + self.is_exclusive = true; + match config.home().try_open_rw(self.filename) { + Ok(Some(lock)) => self.lock = Some(lock), + Ok(None) => return Ok(WouldBlock), + Err(e) => { + if maybe_readonly(&e) { + let result = self.lock_shared_nonblocking(config); + // This has to pretend it is exclusive for recursive locks to work. + self.is_exclusive = true; + return Ok(result); + } else { + return Err(e).with_context(|| "failed to acquire package cache lock"); + } + } } } - }; + self.increment(); + Ok(LockAcquired) + } +} + +/// The state of the [`CacheLocker`]. +#[derive(Debug)] +struct CacheState { + /// The cache lock guards the package cache used for download and + /// resolution (append operations that should not interfere with reading + /// from existing src files). + cache_lock: RecursiveLock, + /// The mutate lock is used to either guard the entire package cache for + /// destructive modifications (in exclusive mode), or for reading the + /// package cache src files (in shared mode). + /// + /// Note that [`CacheLockMode::MutateExclusive`] holds both + /// [`CacheState::mutate_lock`] and [`CacheState::cache_lock`]. + mutate_lock: RecursiveLock, } +/// A held lock guard. +/// +/// When this is dropped, the lock will be released. +#[must_use] +pub struct CacheLock<'lock> { + mode: CacheLockMode, + locker: &'lock CacheLocker, +} + +/// The filename for the [`CacheLockMode::DownloadExclusive`] lock. +const CACHE_LOCK_NAME: &str = ".package-cache"; +/// The filename for the [`CacheLockMode::MutateExclusive`] and +/// [`CacheLockMode::Shared`] lock. +const MUTATE_NAME: &str = ".package-cache-mutate"; + +// Descriptions that are displayed in the "Blocking" message shown to the user. +const SHARED_DESCR: &str = "shared package cache"; +const DOWNLOAD_EXCLUSIVE_DESCR: &str = "package cache"; +const MUTATE_EXCLUSIVE_DESCR: &str = "package cache mutation"; + impl CacheLocker { /// Creates a new `CacheLocker`. pub fn new() -> CacheLocker { CacheLocker { - state: RefCell::new(CacheState::default()), + state: RefCell::new(CacheState { + cache_lock: RecursiveLock::new(CACHE_LOCK_NAME), + mutate_lock: RecursiveLock::new(MUTATE_NAME), + }), } } @@ -365,7 +359,7 @@ impl CacheLocker { /// cargo is holding the lock. pub fn lock(&self, config: &Config, mode: CacheLockMode) -> CargoResult> { let mut state = self.state.borrow_mut(); - state.lock(config, mode)?; + let _ = state.lock(config, mode, Blocking)?; Ok(CacheLock { mode, locker: self }) } @@ -377,7 +371,7 @@ impl CacheLocker { mode: CacheLockMode, ) -> CargoResult>> { let mut state = self.state.borrow_mut(); - if state.try_lock(config, mode)? { + if state.lock(config, mode, NonBlocking)? == LockAcquired { Ok(Some(CacheLock { mode, locker: self })) } else { Ok(None) @@ -397,9 +391,9 @@ impl CacheLocker { let state = self.state.borrow(); match ( mode, - state.cache_lock_count, - state.mutate_lock_count, - state.mutate_is_exclusive, + state.cache_lock.count, + state.mutate_lock.count, + state.mutate_lock.is_exclusive, ) { (CacheLockMode::Shared, _, 1.., _) => true, (CacheLockMode::MutateExclusive, _, 1.., true) => true, @@ -410,20 +404,65 @@ impl CacheLocker { } impl CacheState { - fn lock(&mut self, config: &Config, mode: CacheLockMode) -> CargoResult<()> { - do_lock!(self, config, mode, do_shared_lock, do_exclusive_lock); - Ok(()) - } + fn lock( + &mut self, + config: &Config, + mode: CacheLockMode, + blocking: BlockingMode, + ) -> CargoResult { + use CacheLockMode::*; + if mode == Shared && self.cache_lock.count > 0 && self.mutate_lock.count == 0 { + // Shared lock, when a DownloadExclusive is held. + // + // This isn't supported because it could cause a deadlock. If + // one cargo is attempting to acquire a MutateExclusive lock, + // and acquires the mutate lock, but is blocked on the + // download lock, and the cargo that holds the download lock + // attempts to get a shared lock, they would end up blocking + // each other. + panic!("shared lock while holding download lock is not allowed"); + } + match mode { + Shared => { + if self.mutate_lock.lock_shared(config, SHARED_DESCR, blocking) == WouldBlock { + return Ok(WouldBlock); + } + } + DownloadExclusive => { + if self + .cache_lock + .lock_exclusive(config, DOWNLOAD_EXCLUSIVE_DESCR, blocking)? + == WouldBlock + { + return Ok(WouldBlock); + } + } + MutateExclusive => { + if self + .mutate_lock + .lock_exclusive(config, MUTATE_EXCLUSIVE_DESCR, blocking)? + == WouldBlock + { + return Ok(WouldBlock); + } - fn try_lock(&mut self, config: &Config, mode: CacheLockMode) -> CargoResult { - do_lock!( - self, - config, - mode, - do_try_shared_lock, - do_try_exclusive_lock - ); - Ok(true) + // Part of the contract of MutateExclusive is that it doesn't + // allow any processes to have a lock on the package cache, so + // this acquires both locks. + match self + .cache_lock + .lock_exclusive(config, DOWNLOAD_EXCLUSIVE_DESCR, blocking) + { + Ok(LockAcquired) => {} + Ok(WouldBlock) => return Ok(WouldBlock), + Err(e) => { + self.mutate_lock.decrement(); + return Err(e); + } + } + } + } + Ok(LockAcquired) } } @@ -449,14 +488,14 @@ impl Drop for CacheLock<'_> { let mut state = self.locker.state.borrow_mut(); match self.mode { Shared => { - decrement!(state, mutate_lock_count, mutate_lock); + state.mutate_lock.decrement(); } DownloadExclusive => { - decrement!(state, cache_lock_count, cache_lock); + state.cache_lock.decrement(); } MutateExclusive => { - decrement!(state, cache_lock_count, cache_lock); - decrement!(state, mutate_lock_count, mutate_lock); + state.cache_lock.decrement(); + state.mutate_lock.decrement(); } } } diff --git a/tests/testsuite/cache_lock.rs b/tests/testsuite/cache_lock.rs index c5eaeb2d65b..f5b681f3a21 100644 --- a/tests/testsuite/cache_lock.rs +++ b/tests/testsuite/cache_lock.rs @@ -145,7 +145,7 @@ fn download_then_shared() { } #[cargo_test] -#[should_panic(expected = "Shared to MutateExclusive not supported")] +#[should_panic(expected = "lock upgrade from shared to exclusive not supported")] fn shared_then_mutate() { // This sequence is not supported. a_b_nested(CacheLockMode::Shared, CacheLockMode::MutateExclusive); @@ -204,7 +204,9 @@ fn readonly() { CacheLockMode::DownloadExclusive, CacheLockMode::MutateExclusive, ] { - let _lock = locker.lock(&config, mode).unwrap(); + let _lock1 = locker.lock(&config, mode).unwrap(); + // Make sure it can recursively acquire the lock, too. + let _lock2 = locker.lock(&config, mode).unwrap(); } } From df2e2a3ae6795782089ab2c9bbca8f2dc443c42a Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 8 Oct 2023 17:46:39 -0700 Subject: [PATCH 08/10] Rename some `Filesystem` methods for clarity and consistency. This tries to align the `Filesystem` method names so they have the same form, and also so they spell out exactly what they do. (An alternative would be to have a builder, similar to `OpenOptions`, but that seems a bit overkill here.) This also extends the docs to try to make it clearer how this works. --- src/cargo/core/compiler/future_incompat.rs | 4 +- src/cargo/core/compiler/layout.rs | 2 +- src/cargo/ops/cargo_package.rs | 2 +- .../ops/common_for_install_and_uninstall.rs | 6 +- src/cargo/ops/lockfile.rs | 6 +- src/cargo/util/cache_lock.rs | 36 ++++---- src/cargo/util/config/mod.rs | 2 +- src/cargo/util/flock.rs | 91 +++++++++++++++---- 8 files changed, 107 insertions(+), 42 deletions(-) diff --git a/src/cargo/core/compiler/future_incompat.rs b/src/cargo/core/compiler/future_incompat.rs index 27645a85cb4..af44940bd37 100644 --- a/src/cargo/core/compiler/future_incompat.rs +++ b/src/cargo/core/compiler/future_incompat.rs @@ -167,7 +167,7 @@ impl OnDiskReports { let on_disk = serde_json::to_vec(&self).unwrap(); if let Err(e) = ws .target_dir() - .open_rw( + .open_rw_exclusive_create( FUTURE_INCOMPAT_FILE, ws.config(), "Future incompatibility report", @@ -191,7 +191,7 @@ impl OnDiskReports { /// Loads the on-disk reports. pub fn load(ws: &Workspace<'_>) -> CargoResult { - let report_file = match ws.target_dir().open_ro( + let report_file = match ws.target_dir().open_ro_shared( FUTURE_INCOMPAT_FILE, ws.config(), "Future incompatible report", diff --git a/src/cargo/core/compiler/layout.rs b/src/cargo/core/compiler/layout.rs index d92adffebb3..57b65907ca2 100644 --- a/src/cargo/core/compiler/layout.rs +++ b/src/cargo/core/compiler/layout.rs @@ -166,7 +166,7 @@ impl Layout { // For now we don't do any more finer-grained locking on the artifact // directory, so just lock the entire thing for the duration of this // compile. - let lock = dest.open_rw(".cargo-lock", ws.config(), "build directory")?; + let lock = dest.open_rw_exclusive_create(".cargo-lock", ws.config(), "build directory")?; let root = root.into_path_unlocked(); let dest = dest.into_path_unlocked(); let deps = dest.join("deps"); diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 179673b473b..1a291045b9a 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -133,7 +133,7 @@ pub fn package_one( let dir = ws.target_dir().join("package"); let mut dst = { let tmp = format!(".{}", filename); - dir.open_rw(&tmp, config, "package scratch space")? + dir.open_rw_exclusive_create(&tmp, config, "package scratch space")? }; // Package up and test a temporary tarball and only move it to the final diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index a0f0c4afa9a..45c74ab3c11 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -98,8 +98,10 @@ pub struct CrateListingV1 { impl InstallTracker { /// Create an InstallTracker from information on disk. pub fn load(config: &Config, root: &Filesystem) -> CargoResult { - let v1_lock = root.open_rw(Path::new(".crates.toml"), config, "crate metadata")?; - let v2_lock = root.open_rw(Path::new(".crates2.json"), config, "crate metadata")?; + let v1_lock = + root.open_rw_exclusive_create(Path::new(".crates.toml"), config, "crate metadata")?; + let v2_lock = + root.open_rw_exclusive_create(Path::new(".crates2.json"), config, "crate metadata")?; let v1 = (|| -> CargoResult<_> { let mut contents = String::new(); diff --git a/src/cargo/ops/lockfile.rs b/src/cargo/ops/lockfile.rs index 26456e5609d..aa05efd3559 100644 --- a/src/cargo/ops/lockfile.rs +++ b/src/cargo/ops/lockfile.rs @@ -12,7 +12,7 @@ pub fn load_pkg_lockfile(ws: &Workspace<'_>) -> CargoResult> { return Ok(None); } - let mut f = lock_root.open_ro("Cargo.lock", ws.config(), "Cargo.lock file")?; + let mut f = lock_root.open_ro_shared("Cargo.lock", ws.config(), "Cargo.lock file")?; let mut s = String::new(); f.read_to_string(&mut s) @@ -79,7 +79,7 @@ pub fn write_pkg_lockfile(ws: &Workspace<'_>, resolve: &mut Resolve) -> CargoRes // Ok, if that didn't work just write it out lock_root - .open_rw("Cargo.lock", ws.config(), "Cargo.lock file") + .open_rw_exclusive_create("Cargo.lock", ws.config(), "Cargo.lock file") .and_then(|mut f| { f.file().set_len(0)?; f.write_all(out.as_bytes())?; @@ -100,7 +100,7 @@ fn resolve_to_string_orig( ) -> (Option, String, Filesystem) { // Load the original lock file if it exists. let lock_root = lock_root(ws); - let orig = lock_root.open_ro("Cargo.lock", ws.config(), "Cargo.lock file"); + let orig = lock_root.open_ro_shared("Cargo.lock", ws.config(), "Cargo.lock file"); let orig = orig.and_then(|mut f| { let mut s = String::new(); f.read_to_string(&mut s)?; diff --git a/src/cargo/util/cache_lock.rs b/src/cargo/util/cache_lock.rs index 2226ae5c569..2fd16fcd5e2 100644 --- a/src/cargo/util/cache_lock.rs +++ b/src/cargo/util/cache_lock.rs @@ -188,19 +188,20 @@ impl RecursiveLock { fn lock_shared_blocking(&mut self, config: &Config, description: &'static str) { if self.count == 0 { self.is_exclusive = false; - self.lock = match config - .home() - .open_shared_create(self.filename, config, description) - { - Ok(lock) => Some(lock), - Err(e) => { - // There is no error here because locking is mostly a - // best-effort attempt. If cargo home is read-only, we don't - // want to fail just because we couldn't create the lock file. - tracing::warn!("failed to acquire cache lock {}: {e:?}", self.filename); - None - } - }; + self.lock = + match config + .home() + .open_ro_shared_create(self.filename, config, description) + { + Ok(lock) => Some(lock), + Err(e) => { + // There is no error here because locking is mostly a + // best-effort attempt. If cargo home is read-only, we don't + // want to fail just because we couldn't create the lock file. + tracing::warn!("failed to acquire cache lock {}: {e:?}", self.filename); + None + } + }; } self.increment(); } @@ -209,7 +210,7 @@ impl RecursiveLock { fn lock_shared_nonblocking(&mut self, config: &Config) -> LockingResult { if self.count == 0 { self.is_exclusive = false; - self.lock = match config.home().try_open_shared_create(self.filename) { + self.lock = match config.home().try_open_ro_shared_create(self.filename) { Ok(Some(lock)) => Some(lock), Ok(None) => { return WouldBlock; @@ -255,7 +256,10 @@ impl RecursiveLock { ) -> CargoResult<()> { if self.count == 0 { self.is_exclusive = true; - match config.home().open_rw(self.filename, config, description) { + match config + .home() + .open_rw_exclusive_create(self.filename, config, description) + { Ok(lock) => self.lock = Some(lock), Err(e) => { if maybe_readonly(&e) { @@ -288,7 +292,7 @@ impl RecursiveLock { fn lock_exclusive_nonblocking(&mut self, config: &Config) -> CargoResult { if self.count == 0 { self.is_exclusive = true; - match config.home().try_open_rw(self.filename) { + match config.home().try_open_rw_exclusive_create(self.filename) { Ok(Some(lock)) => self.lock = Some(lock), Ok(None) => return Ok(WouldBlock), Err(e) => { diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 53a0ba38b27..039cf7cd04f 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -2235,7 +2235,7 @@ pub fn save_credentials( let mut file = { cfg.home_path.create_dir()?; cfg.home_path - .open_rw(filename, cfg, "credentials' config file")? + .open_rw_exclusive_create(filename, cfg, "credentials' config file")? }; let mut contents = String::new(); diff --git a/src/cargo/util/flock.rs b/src/cargo/util/flock.rs index a1dc1c37e42..3fb2397f7f6 100644 --- a/src/cargo/util/flock.rs +++ b/src/cargo/util/flock.rs @@ -1,3 +1,12 @@ +//! File-locking support. +//! +//! This module defines the [`Filesystem`] type which is an abstraction over a +//! filesystem, ensuring that access to the filesystem is only done through +//! coordinated locks. +//! +//! The [`FileLock`] type represents a locked file, and provides access to the +//! file. + use std::fs::{File, OpenOptions}; use std::io; use std::io::{Read, Seek, SeekFrom, Write}; @@ -10,6 +19,18 @@ use anyhow::Context as _; use cargo_util::paths; use sys::*; +/// A locked file. +/// +/// This provides access to file while holding a lock on the file. This type +/// implements the [`Read`], [`Write`], and [`Seek`] traits to provide access +/// to the underlying file. +/// +/// Locks are either shared (multiple processes can access the file) or +/// exclusive (only one process can access the file). +/// +/// This type is created via methods on the [`Filesystem`] type. +/// +/// When this value is dropped, the lock will be released. #[derive(Debug)] pub struct FileLock { f: Option, @@ -95,6 +116,32 @@ impl Drop for FileLock { /// The `Path` of a filesystem cannot be learned unless it's done in a locked /// fashion, and otherwise functions on this structure are prepared to handle /// concurrent invocations across multiple instances of Cargo. +/// +/// The methods on `Filesystem` that open files return a [`FileLock`] which +/// holds the lock, and that type provides methods for accessing the +/// underlying file. +/// +/// If the blocking methods (like [`Filesystem::open_ro_shared`]) detect that +/// they will block, then they will display a message to the user letting them +/// know it is blocked. There are non-blocking variants starting with the +/// `try_` prefix like [`Filesystem::try_open_ro_shared_create`]. +/// +/// The behavior of locks acquired by the `Filesystem` depend on the operating +/// system. On unix-like system, they are advisory using [`flock`], and thus +/// not enforced against processes which do not try to acquire the lock. On +/// Windows, they are mandatory using [`LockFileEx`], enforced against all +/// processes. +/// +/// This **does not** guarantee that a lock is acquired. In some cases, for +/// example on filesystems that don't support locking, it will return a +/// [`FileLock`] even though the filesystem lock was not acquired. This is +/// intended to provide a graceful fallback instead of refusing to work. +/// Usually there aren't multiple processes accessing the same resource. In +/// that case, it is the user's responsibility to not run concurrent +/// processes. +/// +/// [`flock`]: https://linux.die.net/man/2/flock +/// [`LockFileEx`]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-lockfileex #[derive(Clone, Debug)] pub struct Filesystem { root: PathBuf, @@ -147,17 +194,22 @@ impl Filesystem { self.root.display() } - /// Opens exclusive access to a file, returning the locked version of a - /// file. + /// Opens read-write exclusive access to a file, returning the locked + /// version of a file. /// /// This function will create a file at `path` if it doesn't already exist /// (including intermediate directories), and then it will acquire an /// exclusive lock on `path`. If the process must block waiting for the - /// lock, the `msg` is printed to `config`. + /// lock, the `msg` is printed to [`Config`]. /// /// The returned file can be accessed to look at the path and also has /// read/write access to the underlying file. - pub fn open_rw

(&self, path: P, config: &Config, msg: &str) -> CargoResult + pub fn open_rw_exclusive_create

( + &self, + path: P, + config: &Config, + msg: &str, + ) -> CargoResult where P: AsRef, { @@ -170,11 +222,14 @@ impl Filesystem { Ok(FileLock { f: Some(f), path }) } - /// A non-blocking version of [`Filesystem::open_rw`]. + /// A non-blocking version of [`Filesystem::open_rw_exclusive_create`]. /// /// Returns `None` if the operation would block due to another process /// holding the lock. - pub fn try_open_rw>(&self, path: P) -> CargoResult> { + pub fn try_open_rw_exclusive_create>( + &self, + path: P, + ) -> CargoResult> { let mut opts = OpenOptions::new(); opts.read(true).write(true).create(true); let (path, f) = self.open(path.as_ref(), &opts, true)?; @@ -185,16 +240,16 @@ impl Filesystem { } } - /// Opens shared access to a file, returning the locked version of a file. + /// Opens read-only shared access to a file, returning the locked version of a file. /// /// This function will fail if `path` doesn't already exist, but if it does /// then it will acquire a shared lock on `path`. If the process must block - /// waiting for the lock, the `msg` is printed to `config`. + /// waiting for the lock, the `msg` is printed to [`Config`]. /// /// The returned file can be accessed to look at the path and also has read /// access to the underlying file. Any writes to the file will return an /// error. - pub fn open_ro

(&self, path: P, config: &Config, msg: &str) -> CargoResult + pub fn open_ro_shared

(&self, path: P, config: &Config, msg: &str) -> CargoResult where P: AsRef, { @@ -205,11 +260,12 @@ impl Filesystem { Ok(FileLock { f: Some(f), path }) } - /// Opens shared access to a file, returning the locked version of a file. + /// Opens read-only shared access to a file, returning the locked version of a file. /// - /// Compared to [`Filesystem::open_ro`], this will create the file (and - /// any directories in the parent) if the file does not already exist. - pub fn open_shared_create>( + /// Compared to [`Filesystem::open_ro_shared`], this will create the file + /// (and any directories in the parent) if the file does not already + /// exist. + pub fn open_ro_shared_create>( &self, path: P, config: &Config, @@ -224,11 +280,14 @@ impl Filesystem { Ok(FileLock { f: Some(f), path }) } - /// A non-blocking version of [`Filesystem::open_shared_create`]. + /// A non-blocking version of [`Filesystem::open_ro_shared_create`]. /// /// Returns `None` if the operation would block due to another process /// holding the lock. - pub fn try_open_shared_create>(&self, path: P) -> CargoResult> { + pub fn try_open_ro_shared_create>( + &self, + path: P, + ) -> CargoResult> { let mut opts = OpenOptions::new(); opts.read(true).write(true).create(true); let (path, f) = self.open(path.as_ref(), &opts, true)?; @@ -316,7 +375,7 @@ fn try_acquire(path: &Path, lock_try: &dyn Fn() -> io::Result<()>) -> CargoResul /// This function will acquire the lock on a `path`, printing out a nice message /// to the console if we have to wait for it. It will first attempt to use `try` /// to acquire a lock on the crate, and in the case of contention it will emit a -/// status message based on `msg` to `config`'s shell, and then use `block` to +/// status message based on `msg` to [`Config`]'s shell, and then use `block` to /// block waiting to acquire a lock. /// /// Returns an error if the lock could not be acquired or if any error other From 889558f76bb1aec2253ec6ab452fc4c07271cb0d Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 8 Oct 2023 18:12:54 -0700 Subject: [PATCH 09/10] Extend cache_lock documentation. --- src/cargo/util/cache_lock.rs | 57 +++++++++++++++++++++++++++++++----- 1 file changed, 50 insertions(+), 7 deletions(-) diff --git a/src/cargo/util/cache_lock.rs b/src/cargo/util/cache_lock.rs index 2fd16fcd5e2..18b29f8db59 100644 --- a/src/cargo/util/cache_lock.rs +++ b/src/cargo/util/cache_lock.rs @@ -2,7 +2,29 @@ //! //! This implements locking on the package and index caches (source files, //! `.crate` files, and index caches) to coordinate when multiple cargos are -//! running at the same time. There are three styles of locks: +//! running at the same time. +//! +//! ## Usage +//! +//! There is a global [`CacheLocker`] held inside cargo's venerable +//! [`Config`]. The `CacheLocker` manages creating and tracking the locks +//! being held. There are methods on `Config` for managing the locks: +//! +//! - [`Config::acquire_package_cache_lock`] --- Acquires a lock. May block if +//! another process holds a lock. +//! - [`Config::try_acquire_package_cache_lock`] --- Acquires a lock, returning +//! immediately if it would block. +//! - [`Config::assert_package_cache_locked`] --- This is used to ensure the +//! proper lock is being held. +//! +//! Lower-level code that accesses the package cache typically just use +//! `assert_package_cache_locked` to ensure that the correct lock is being +//! held. Higher-level code is responsible for acquiring the appropriate lock, +//! and holding it during the duration that it is performing its operation. +//! +//! ## Types of locking +//! +//! There are three styles of locks: //! //! * [`CacheLockMode::DownloadExclusive`] -- This is an exclusive lock //! acquired while downloading packages and doing resolution. @@ -17,9 +39,14 @@ //! cargo is reading from the cache. //! //! Importantly, a `DownloadExclusive` lock does *not* interfere with a -//! `Shared` lock. The download process generally does not modify source -//! files, so other cargos should be able to safely proceed in reading source -//! files[^1]. +//! `Shared` lock. The download process generally does not modify source files +//! (it only adds new ones), so other cargos should be able to safely proceed +//! in reading source files[^1]. +//! +//! See the [`CacheLockMode`] enum docs for more details on when the different +//! modes should be used. +//! +//! ## Locking implementation details //! //! This is implemented by two separate lock files, the "download" one and the //! "mutate" one. The `MutateExclusive` lock acquired both the "mutate" and @@ -73,6 +100,8 @@ pub enum CacheLockMode { /// A `DownloadExclusive` lock ensures that only one cargo is doing /// resolution and downloading new packages. /// + /// You should use this when downloading new packages or doing resolution. + /// /// If another cargo has a `MutateExclusive` lock, then an attempt to get /// a `DownloadExclusive` lock will block. /// @@ -81,22 +110,36 @@ pub enum CacheLockMode { DownloadExclusive, /// A `Shared` lock allows multiple cargos to read from the source files. /// + /// You should use this when cargo is reading source files from the + /// package cache. This is typically done during the build phase, since + /// cargo only needs to read files during that time. This allows multiple + /// cargo processes to build concurrently without interfering with one + /// another, while guarding against other cargos using `MutateExclusive`. + /// /// If another cargo has a `MutateExclusive` lock, then an attempt to get /// a `Shared` will block. /// - /// If another cargo has a `DownloadExclusive` lock, then the both can + /// If another cargo has a `DownloadExclusive` lock, then they both can /// operate concurrently under the assumption that downloading does not /// modify existing source files. Shared, /// A `MutateExclusive` lock ensures no other cargo is reading or writing /// from the package caches. /// - /// This is used for things like garbage collection to avoid modifying - /// caches while other cargos are running. + /// You should use this when modifying existing files in the package + /// cache. For example, things like garbage collection want to avoid + /// deleting files while other cargos are trying to read (`Shared`) or + /// resolve or download (`DownloadExclusive`). + /// + /// If another cargo has a `DownloadExclusive` or `Shared` lock, then this + /// will block until they all release their locks. MutateExclusive, } /// A locker that can be used to acquire locks. +/// +/// See the [`crate::util::cache_lock`] module documentation for an overview +/// of how cache locking works. #[derive(Debug)] pub struct CacheLocker { /// The state of the locker. From 78bb7c5cb5a061d153f0a3bf5446a27bfa721a71 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 8 Oct 2023 18:15:57 -0700 Subject: [PATCH 10/10] Rearrange impl blocks to be near their types. --- src/cargo/util/cache_lock.rs | 190 +++++++++++++++++------------------ 1 file changed, 95 insertions(+), 95 deletions(-) diff --git a/src/cargo/util/cache_lock.rs b/src/cargo/util/cache_lock.rs index 18b29f8db59..ca9e8d1b07c 100644 --- a/src/cargo/util/cache_lock.rs +++ b/src/cargo/util/cache_lock.rs @@ -136,19 +136,6 @@ pub enum CacheLockMode { MutateExclusive, } -/// A locker that can be used to acquire locks. -/// -/// See the [`crate::util::cache_lock`] module documentation for an overview -/// of how cache locking works. -#[derive(Debug)] -pub struct CacheLocker { - /// The state of the locker. - /// - /// [`CacheLocker`] uses interior mutability because it is stuffed inside - /// the global `Config`, which does not allow mutation. - state: RefCell, -} - /// Whether or not a lock attempt should block. #[derive(Copy, Clone)] enum BlockingMode { @@ -371,6 +358,69 @@ struct CacheState { mutate_lock: RecursiveLock, } +impl CacheState { + fn lock( + &mut self, + config: &Config, + mode: CacheLockMode, + blocking: BlockingMode, + ) -> CargoResult { + use CacheLockMode::*; + if mode == Shared && self.cache_lock.count > 0 && self.mutate_lock.count == 0 { + // Shared lock, when a DownloadExclusive is held. + // + // This isn't supported because it could cause a deadlock. If + // one cargo is attempting to acquire a MutateExclusive lock, + // and acquires the mutate lock, but is blocked on the + // download lock, and the cargo that holds the download lock + // attempts to get a shared lock, they would end up blocking + // each other. + panic!("shared lock while holding download lock is not allowed"); + } + match mode { + Shared => { + if self.mutate_lock.lock_shared(config, SHARED_DESCR, blocking) == WouldBlock { + return Ok(WouldBlock); + } + } + DownloadExclusive => { + if self + .cache_lock + .lock_exclusive(config, DOWNLOAD_EXCLUSIVE_DESCR, blocking)? + == WouldBlock + { + return Ok(WouldBlock); + } + } + MutateExclusive => { + if self + .mutate_lock + .lock_exclusive(config, MUTATE_EXCLUSIVE_DESCR, blocking)? + == WouldBlock + { + return Ok(WouldBlock); + } + + // Part of the contract of MutateExclusive is that it doesn't + // allow any processes to have a lock on the package cache, so + // this acquires both locks. + match self + .cache_lock + .lock_exclusive(config, DOWNLOAD_EXCLUSIVE_DESCR, blocking) + { + Ok(LockAcquired) => {} + Ok(WouldBlock) => return Ok(WouldBlock), + Err(e) => { + self.mutate_lock.decrement(); + return Err(e); + } + } + } + } + Ok(LockAcquired) + } +} + /// A held lock guard. /// /// When this is dropped, the lock will be released. @@ -380,6 +430,25 @@ pub struct CacheLock<'lock> { locker: &'lock CacheLocker, } +impl Drop for CacheLock<'_> { + fn drop(&mut self) { + use CacheLockMode::*; + let mut state = self.locker.state.borrow_mut(); + match self.mode { + Shared => { + state.mutate_lock.decrement(); + } + DownloadExclusive => { + state.cache_lock.decrement(); + } + MutateExclusive => { + state.cache_lock.decrement(); + state.mutate_lock.decrement(); + } + } + } +} + /// The filename for the [`CacheLockMode::DownloadExclusive`] lock. const CACHE_LOCK_NAME: &str = ".package-cache"; /// The filename for the [`CacheLockMode::MutateExclusive`] and @@ -391,6 +460,19 @@ const SHARED_DESCR: &str = "shared package cache"; const DOWNLOAD_EXCLUSIVE_DESCR: &str = "package cache"; const MUTATE_EXCLUSIVE_DESCR: &str = "package cache mutation"; +/// A locker that can be used to acquire locks. +/// +/// See the [`crate::util::cache_lock`] module documentation for an overview +/// of how cache locking works. +#[derive(Debug)] +pub struct CacheLocker { + /// The state of the locker. + /// + /// [`CacheLocker`] uses interior mutability because it is stuffed inside + /// the global `Config`, which does not allow mutation. + state: RefCell, +} + impl CacheLocker { /// Creates a new `CacheLocker`. pub fn new() -> CacheLocker { @@ -450,69 +532,6 @@ impl CacheLocker { } } -impl CacheState { - fn lock( - &mut self, - config: &Config, - mode: CacheLockMode, - blocking: BlockingMode, - ) -> CargoResult { - use CacheLockMode::*; - if mode == Shared && self.cache_lock.count > 0 && self.mutate_lock.count == 0 { - // Shared lock, when a DownloadExclusive is held. - // - // This isn't supported because it could cause a deadlock. If - // one cargo is attempting to acquire a MutateExclusive lock, - // and acquires the mutate lock, but is blocked on the - // download lock, and the cargo that holds the download lock - // attempts to get a shared lock, they would end up blocking - // each other. - panic!("shared lock while holding download lock is not allowed"); - } - match mode { - Shared => { - if self.mutate_lock.lock_shared(config, SHARED_DESCR, blocking) == WouldBlock { - return Ok(WouldBlock); - } - } - DownloadExclusive => { - if self - .cache_lock - .lock_exclusive(config, DOWNLOAD_EXCLUSIVE_DESCR, blocking)? - == WouldBlock - { - return Ok(WouldBlock); - } - } - MutateExclusive => { - if self - .mutate_lock - .lock_exclusive(config, MUTATE_EXCLUSIVE_DESCR, blocking)? - == WouldBlock - { - return Ok(WouldBlock); - } - - // Part of the contract of MutateExclusive is that it doesn't - // allow any processes to have a lock on the package cache, so - // this acquires both locks. - match self - .cache_lock - .lock_exclusive(config, DOWNLOAD_EXCLUSIVE_DESCR, blocking) - { - Ok(LockAcquired) => {} - Ok(WouldBlock) => return Ok(WouldBlock), - Err(e) => { - self.mutate_lock.decrement(); - return Err(e); - } - } - } - } - Ok(LockAcquired) - } -} - /// Returns whether or not the error appears to be from a read-only filesystem. fn maybe_readonly(err: &anyhow::Error) -> bool { err.chain().any(|err| { @@ -528,22 +547,3 @@ fn maybe_readonly(err: &anyhow::Error) -> bool { false }) } - -impl Drop for CacheLock<'_> { - fn drop(&mut self) { - use CacheLockMode::*; - let mut state = self.locker.state.borrow_mut(); - match self.mode { - Shared => { - state.mutate_lock.decrement(); - } - DownloadExclusive => { - state.cache_lock.decrement(); - } - MutateExclusive => { - state.cache_lock.decrement(); - state.mutate_lock.decrement(); - } - } - } -}