diff --git a/src/cargo/core/compiler/build_runner/compilation_files.rs b/src/cargo/core/compiler/build_runner/compilation_files.rs index 46329f947cd..e2df81d316b 100644 --- a/src/cargo/core/compiler/build_runner/compilation_files.rs +++ b/src/cargo/core/compiler/build_runner/compilation_files.rs @@ -444,6 +444,11 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> { .map(Arc::clone) } + /// Returns the path to the acquisition lock, used to avoid multiple Cargos from deadlocking + pub fn acquisition_lock(&self) -> &Path { + self.host.build_dir().acquisition_lock() + } + /// Returns the path where the output for the given unit and `FileType` /// should be uplifted to. /// diff --git a/src/cargo/core/compiler/build_runner/mod.rs b/src/cargo/core/compiler/build_runner/mod.rs index 957fecf55eb..a65cb72fecb 100644 --- a/src/cargo/core/compiler/build_runner/mod.rs +++ b/src/cargo/core/compiler/build_runner/mod.rs @@ -178,6 +178,15 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> { self.check_collisions()?; self.compute_metadata_for_doc_units(); + // When -Zfine-grain-locking is enabled, we take an "acquisition lock" exclusively + // that we hold while taking other locks. This lock will block other Cargos from also + // taking locks possibly causing deadlocks. We drop the acquisition lock before we start + // executing jobs allowing other Cargos to compile in parallel if they do not share build + // units. + if self.bcx.gctx.cli_unstable().fine_grain_locking { + self.lock_manager.acquire_acquisition_lock(&self)?; + } + // We need to make sure that if there were any previous docs already compiled, // they were compiled with the same Rustc version that we're currently using. // See the function doc comment for more. @@ -200,6 +209,11 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> { fingerprint.clear_memoized(); } + // Release acquisition lock allowing other Cargos to proceed + if self.bcx.gctx.cli_unstable().fine_grain_locking { + self.lock_manager.release_acquisition_lock()?; + } + // Now that we've figured out everything that we're going to do, do it! queue.execute(&mut self)?; diff --git a/src/cargo/core/compiler/job_queue/job_state.rs b/src/cargo/core/compiler/job_queue/job_state.rs index 06a672b611d..ae5cc32875d 100644 --- a/src/cargo/core/compiler/job_queue/job_state.rs +++ b/src/cargo/core/compiler/job_queue/job_state.rs @@ -147,10 +147,6 @@ impl<'a, 'gctx> JobState<'a, 'gctx> { .push(Message::Finish(self.id, Artifact::Metadata, Ok(()))); } - pub fn lock_exclusive(&self, lock: &LockKey) -> CargoResult<()> { - self.lock_manager.lock(lock) - } - pub fn downgrade_to_shared(&self, lock: &LockKey) -> CargoResult<()> { self.lock_manager.downgrade_to_shared(lock) } diff --git a/src/cargo/core/compiler/layout.rs b/src/cargo/core/compiler/layout.rs index bfc5608fe13..1aa4ca3b764 100644 --- a/src/cargo/core/compiler/layout.rs +++ b/src/cargo/core/compiler/layout.rs @@ -286,6 +286,7 @@ impl Layout { let build_dest = build_dest.as_path_unlocked(); let deps = build_dest.join("deps"); let artifact = deps.join("artifact"); + let acquisition_lock = build_dest.join(".acquisition-lock"); let artifact_dir = if must_take_artifact_dir_lock { // For now we don't do any more finer-grained locking on the artifact @@ -323,6 +324,7 @@ impl Layout { fingerprint: build_dest.join(".fingerprint"), examples: build_dest.join("examples"), tmp: build_root.join("tmp"), + acquisition_lock, _lock: build_dir_lock, is_new_layout, }, @@ -405,6 +407,7 @@ pub struct BuildDirLayout { examples: PathBuf, /// The directory for temporary data of integration tests and benches tmp: PathBuf, + acquisition_lock: PathBuf, /// The lockfile for a build (`.cargo-lock`). Will be unlocked when this /// struct is `drop`ped. /// @@ -505,4 +508,8 @@ impl BuildDirLayout { paths::create_dir_all(&self.tmp)?; Ok(&self.tmp) } + // Fetch the acquisition lock path + pub fn acquisition_lock(&self) -> &Path { + &self.acquisition_lock + } } diff --git a/src/cargo/core/compiler/locking.rs b/src/cargo/core/compiler/locking.rs index ea9f98632a1..57657b0865b 100644 --- a/src/cargo/core/compiler/locking.rs +++ b/src/cargo/core/compiler/locking.rs @@ -16,16 +16,52 @@ use tracing::instrument; /// A struct to store the lock handles for build units during compilation. pub struct LockManager { + acquisition: RwLock>, locks: RwLock>, } impl LockManager { pub fn new() -> Self { Self { + acquisition: RwLock::new(None), locks: RwLock::new(HashMap::new()), } } + /// Acquires the acquisition lock required to call [`LockManager::lock`] and [`LockManager::lock_shared`] + /// + /// This should be called prior to attempting lock build units and should be released prior to + /// executing compilation jobs to allow other Cargos to proceed if they do not share any build + /// units. + #[instrument(skip_all)] + pub fn acquire_acquisition_lock(&self, build_runner: &BuildRunner<'_, '_>) -> CargoResult<()> { + let path = build_runner.files().acquisition_lock(); + let fs = Filesystem::new(path.to_path_buf()); + + let lock = fs.open_rw_exclusive_create(&path, build_runner.bcx.gctx, "acquisition lock")?; + + let Ok(mut acquisition_lock) = self.acquisition.write() else { + bail!("failed to take acquisition write lock"); + }; + *acquisition_lock = Some(lock); + + Ok(()) + } + + /// Releases the acquisition lock, see [`LockManager::acquire_acquisition_lock`] + #[instrument(skip_all)] + pub fn release_acquisition_lock(&self) -> CargoResult<()> { + let Ok(mut acquisition_lock) = self.acquisition.write() else { + bail!("failed to take acquisition write lock"); + }; + assert!( + acquisition_lock.is_some(), + "attempted to release acquisition while it was not taken" + ); + *acquisition_lock = None; + Ok(()) + } + /// Takes a shared lock on a given [`Unit`] /// This prevents other Cargo instances from compiling (writing) to /// this build unit. @@ -38,6 +74,10 @@ impl LockManager { build_runner: &BuildRunner<'_, '_>, unit: &Unit, ) -> CargoResult { + assert!( + self.acquisition.read().unwrap().is_some(), + "attempted to take shared lock without acquisition lock" + ); let key = LockKey::from_unit(build_runner, unit); tracing::Span::current().record("key", key.0.to_str()); @@ -60,6 +100,10 @@ impl LockManager { #[instrument(skip(self))] pub fn lock(&self, key: &LockKey) -> CargoResult<()> { + assert!( + self.acquisition.read().unwrap().is_some(), + "attempted to take exclusive lock without acquisition lock" + ); let locks = self.locks.read().unwrap(); if let Some(lock) = locks.get(&key) { lock.file().lock()?; diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 014a2781c79..cfad7099bd4 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -237,19 +237,26 @@ fn compile<'gctx>( work.then(link_targets(build_runner, unit, true)?) }); - // If -Zfine-grain-locking is enabled, we wrap the job with an upgrade to exclusive + // If -Zfine-grain-locking is enabled, we take an exclusive // lock before starting, then downgrade to a shared lock after the job is finished. if build_runner.bcx.gctx.cli_unstable().fine_grain_locking && job.freshness().is_dirty() { if let Some(lock) = lock { - // Here we unlock the current shared lock to avoid deadlocking with other cargo - // processes. Then we configure our compile job to take an exclusive lock - // before starting. Once we are done compiling (including both rmeta and rlib) - // we downgrade to a shared lock to allow other cargo's to read the build unit. - // We will hold this shared lock for the remainder of compilation to prevent - // other cargo from re-compiling while we are still using the unit. + // We take an exclusive lock before scheduling the job to keep our locking model + // simple. While this does mean we could potentially be waiting for another job + // when this could begin immediately, it reduces the risk of deadlock. + // + // We are also optimizing for avoiding rust-analyzer's `cargo check` preventing + // the user from running `cargo build` while developing. Generally, only a few + // workspace units will be changing in each build and the units will not be + // shared between `build` and `check` allowing them to run in parallel. + // + // Also note that we unlock before taking the exclusive lock as not all + // platforms support lock upgrading. This is safe as we hold the acquisition + // lock so we should be the only one operating on the locks so we can assume + // that the lock will not be stolen by another instance. build_runner.lock_manager.unlock(&lock)?; - job.before(prebuild_lock_exclusive(lock.clone())); + build_runner.lock_manager.lock(&lock)?; job.after(downgrade_lock_to_shared(lock)); } } @@ -615,13 +622,6 @@ fn verbose_if_simple_exit_code(err: Error) -> Error { } } -fn prebuild_lock_exclusive(lock: LockKey) -> Work { - Work::new(move |state| { - state.lock_exclusive(&lock)?; - Ok(()) - }) -} - fn downgrade_lock_to_shared(lock: LockKey) -> Work { Work::new(move |state| { state.downgrade_to_shared(&lock)?; diff --git a/tests/testsuite/build_dir.rs b/tests/testsuite/build_dir.rs index 92f17e7b5dd..d654d3befc1 100644 --- a/tests/testsuite/build_dir.rs +++ b/tests/testsuite/build_dir.rs @@ -40,6 +40,7 @@ fn binary_with_debug() { [ROOT]/foo/build-dir/.rustc_info.json [ROOT]/foo/build-dir/CACHEDIR.TAG [ROOT]/foo/build-dir/debug/.cargo-lock +[ROOT]/foo/build-dir/debug/.acquisition-lock [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo @@ -95,6 +96,7 @@ fn binary_with_release() { [ROOT]/foo/build-dir/.rustc_info.json [ROOT]/foo/build-dir/CACHEDIR.TAG [ROOT]/foo/build-dir/release/.cargo-lock +[ROOT]/foo/build-dir/release/.acquisition-lock [ROOT]/foo/build-dir/release/build/foo/[HASH]/fingerprint/bin-foo [ROOT]/foo/build-dir/release/build/foo/[HASH]/fingerprint/bin-foo.json [ROOT]/foo/build-dir/release/build/foo/[HASH]/fingerprint/dep-bin-foo @@ -205,6 +207,7 @@ fn should_default_to_target() { [ROOT]/foo/target/.rustc_info.json [ROOT]/foo/target/CACHEDIR.TAG [ROOT]/foo/target/debug/.cargo-lock +[ROOT]/foo/target/debug/.acquisition-lock [ROOT]/foo/target/debug/build/foo/[HASH]/fingerprint/bin-foo [ROOT]/foo/target/debug/build/foo/[HASH]/fingerprint/bin-foo.json [ROOT]/foo/target/debug/build/foo/[HASH]/fingerprint/dep-bin-foo @@ -234,6 +237,7 @@ fn should_respect_env_var() { [ROOT]/foo/build-dir/.rustc_info.json [ROOT]/foo/build-dir/CACHEDIR.TAG [ROOT]/foo/build-dir/debug/.cargo-lock +[ROOT]/foo/build-dir/debug/.acquisition-lock [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo @@ -279,6 +283,7 @@ fn build_script_should_output_to_build_dir() { p.root().join("build-dir").assert_build_dir_layout(str![[r#" [ROOT]/foo/build-dir/CACHEDIR.TAG [ROOT]/foo/build-dir/debug/.cargo-lock +[ROOT]/foo/build-dir/debug/.acquisition-lock [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo.txt [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/build_script_build[..].d [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/build_script_build[..][EXE] @@ -342,6 +347,7 @@ fn cargo_tmpdir_should_output_to_build_dir() { p.root().join("build-dir").assert_build_dir_layout(str![[r#" [ROOT]/foo/build-dir/CACHEDIR.TAG [ROOT]/foo/build-dir/debug/.cargo-lock +[ROOT]/foo/build-dir/debug/.acquisition-lock [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo-[HASH].d [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo.d [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo[..].d @@ -402,6 +408,7 @@ fn examples_should_output_to_build_dir_and_uplift_to_target_dir() { [ROOT]/foo/build-dir/.rustc_info.json [ROOT]/foo/build-dir/CACHEDIR.TAG [ROOT]/foo/build-dir/debug/.cargo-lock +[ROOT]/foo/build-dir/debug/.acquisition-lock [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-example-foo [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/example-foo [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/example-foo.json @@ -448,6 +455,7 @@ fn benches_should_output_to_build_dir() { p.root().join("build-dir").assert_build_dir_layout(str![[r#" [ROOT]/foo/build-dir/CACHEDIR.TAG [ROOT]/foo/build-dir/debug/.cargo-lock +[ROOT]/foo/build-dir/debug/.acquisition-lock [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo-[HASH].d [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo[..].d [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo-[HASH][EXE] @@ -527,6 +535,7 @@ fn cargo_package_should_build_in_build_dir_and_output_to_target_dir() { p.root().join("build-dir").assert_build_dir_layout(str![[r#" [ROOT]/foo/build-dir/.rustc_info.json [ROOT]/foo/build-dir/debug/.cargo-lock +[ROOT]/foo/build-dir/debug/.acquisition-lock [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo @@ -608,6 +617,7 @@ fn cargo_clean_should_clean_the_target_dir_and_build_dir() { [ROOT]/foo/build-dir/.rustc_info.json [ROOT]/foo/build-dir/CACHEDIR.TAG [ROOT]/foo/build-dir/debug/.cargo-lock +[ROOT]/foo/build-dir/debug/.acquisition-lock [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo @@ -678,6 +688,7 @@ fn cargo_clean_should_remove_correct_files() { [ROOT]/foo/build-dir/.rustc_info.json [ROOT]/foo/build-dir/CACHEDIR.TAG [ROOT]/foo/build-dir/debug/.cargo-lock +[ROOT]/foo/build-dir/debug/.acquisition-lock [ROOT]/foo/build-dir/debug/build/bar/[HASH]/out/bar-[HASH].d [ROOT]/foo/build-dir/debug/build/bar/[HASH]/out/libbar-[HASH].rlib [ROOT]/foo/build-dir/debug/build/bar/[HASH]/out/libbar-[HASH].rmeta @@ -705,6 +716,7 @@ fn cargo_clean_should_remove_correct_files() { [ROOT]/foo/build-dir/.rustc_info.json [ROOT]/foo/build-dir/CACHEDIR.TAG [ROOT]/foo/build-dir/debug/.cargo-lock +[ROOT]/foo/build-dir/debug/.acquisition-lock [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo[..][EXE] [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo[..].d [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo @@ -841,6 +853,7 @@ fn template_workspace_root() { [ROOT]/foo/build-dir/.rustc_info.json [ROOT]/foo/build-dir/CACHEDIR.TAG [ROOT]/foo/build-dir/debug/.cargo-lock +[ROOT]/foo/build-dir/debug/.acquisition-lock [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo @@ -889,6 +902,7 @@ fn template_cargo_cache_home() { [ROOT]/home/.cargo/build-dir/.rustc_info.json [ROOT]/home/.cargo/build-dir/CACHEDIR.TAG [ROOT]/home/.cargo/build-dir/debug/.cargo-lock +[ROOT]/home/.cargo/build-dir/debug/.acquisition-lock [ROOT]/home/.cargo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo [ROOT]/home/.cargo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json [ROOT]/home/.cargo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo @@ -951,6 +965,7 @@ fn template_workspace_path_hash() { [ROOT]/foo/foo/[HASH]/build-dir/.rustc_info.json [ROOT]/foo/foo/[HASH]/build-dir/CACHEDIR.TAG [ROOT]/foo/foo/[HASH]/build-dir/debug/.cargo-lock +[ROOT]/foo/foo/[HASH]/build-dir/debug/.acquisition-lock [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo @@ -1019,6 +1034,7 @@ fn template_workspace_path_hash_should_handle_symlink() { [ROOT]/foo/foo/[HASH]/build-dir/.rustc_info.json [ROOT]/foo/foo/[HASH]/build-dir/CACHEDIR.TAG [ROOT]/foo/foo/[HASH]/build-dir/debug/.cargo-lock +[ROOT]/foo/foo/[HASH]/build-dir/debug/.acquisition-lock [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/dep-lib-foo [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/invoked.timestamp [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/lib-foo @@ -1058,6 +1074,7 @@ fn template_workspace_path_hash_should_handle_symlink() { [ROOT]/foo/foo/[HASH]/build-dir/.rustc_info.json [ROOT]/foo/foo/[HASH]/build-dir/CACHEDIR.TAG [ROOT]/foo/foo/[HASH]/build-dir/debug/.cargo-lock +[ROOT]/foo/foo/[HASH]/build-dir/debug/.acquisition-lock [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/dep-lib-foo [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/invoked.timestamp [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/lib-foo