From c14cb985106fbe800e67520d5e5b647684c42db0 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 15 Aug 2016 02:04:07 +0300 Subject: [PATCH 1/2] Add a test for #2987 --- tests/concurrent.rs | 69 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/tests/concurrent.rs b/tests/concurrent.rs index 0243e782f83..afd8c21e3ef 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -9,6 +9,8 @@ use std::io::Write; use std::net::TcpListener; use std::process::Stdio; use std::thread; +use std::sync::mpsc::channel; +use std::time::Duration; use cargotest::install::{has_installed_exe, cargo_home}; use cargotest::support::git; @@ -428,3 +430,70 @@ fn debug_release_ok() { [FINISHED] release [optimized] target(s) in [..] ")); } + +#[test] +fn no_deadlock_with_git_dependencies() { + let dep1 = git::new("dep1", |project| { + project.file("Cargo.toml", r#" + [project] + name = "dep1" + version = "0.5.0" + authors = [] + "#).file("src/lib.rs", "") + }).unwrap(); + + let dep2 = git::new("dep2", |project| { + project.file("Cargo.toml", r#" + [project] + name = "dep2" + version = "0.5.0" + authors = [] + "#).file("src/lib.rs", "") + }).unwrap(); + + let p = project("foo") + .file("Cargo.toml", &format!(r#" + [package] + name = "foo" + authors = [] + version = "0.0.0" + + [dependencies] + dep1 = {{ git = '{}' }} + dep2 = {{ git = '{}' }} + "#, dep1.url(), dep2.url())) + .file("src/main.rs", "fn main() { }"); + p.build(); + + let n_concurrent_builds = 5; + + let (tx, rx) = channel(); + for _ in 0..n_concurrent_builds { + let cmd = p.cargo("build").build_command() + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn(); + let tx = tx.clone(); + thread::spawn(move || { + let result = cmd.unwrap().wait_with_output().unwrap(); + tx.send(result).unwrap() + }); + } + + //TODO: use `Receiver::recv_timeout` once it is stable. + let recv_timeout = |chan: &::std::sync::mpsc::Receiver<_>| { + for _ in 0..200 { + if let Ok(x) = chan.try_recv() { + return x + } + thread::sleep(Duration::from_millis(10)); + } + chan.try_recv().expect("Deadlock!") + }; + + for _ in 0..n_concurrent_builds { + let result = recv_timeout(&rx); + assert_that(result, execs().with_status(0)) + } + +} \ No newline at end of file From 7a71d5bffca2d3b48f94e773e7b9259a8fe0ff69 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 15 Aug 2016 13:48:40 +0300 Subject: [PATCH 2/2] Use a single lock for all git repositories --- src/cargo/sources/git/source.rs | 25 ++++++------------------- src/cargo/util/config.rs | 27 ++++++++++++++++++++------- tests/concurrent.rs | 2 +- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index c2254aa976e..3ab8b79c638 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -6,7 +6,7 @@ use url::Url; use core::source::{Source, SourceId}; use core::GitReference; use core::{Package, PackageId, Summary, Registry, Dependency}; -use util::{CargoResult, Config, FileLock, to_hex}; +use util::{CargoResult, Config, to_hex}; use sources::PathSource; use sources::git::utils::{GitRemote, GitRevision}; @@ -18,7 +18,6 @@ pub struct GitSource<'cfg> { source_id: SourceId, path_source: Option>, rev: Option, - checkout_lock: Option, ident: String, config: &'cfg Config, } @@ -42,7 +41,6 @@ impl<'cfg> GitSource<'cfg> { source_id: source_id.clone(), path_source: None, rev: None, - checkout_lock: None, ident: ident, config: config, } @@ -128,14 +126,9 @@ impl<'cfg> Registry for GitSource<'cfg> { impl<'cfg> Source for GitSource<'cfg> { fn update(&mut self) -> CargoResult<()> { - // First, lock both the global database and checkout locations that - // we're going to use. We may be performing a fetch into these locations - // so we need writable access. - let db_lock = format!(".cargo-lock-{}", self.ident); - let db_lock = try!(self.config.git_db_path() - .open_rw(&db_lock, self.config, - "the git database")); - let db_path = db_lock.parent().join(&self.ident); + let lock = try!(self.config.lock_git()); + + let db_path = lock.parent().join("db").join(&self.ident); let reference_path = match self.source_id.git_reference() { Some(&GitReference::Branch(ref s)) | @@ -143,13 +136,8 @@ impl<'cfg> Source for GitSource<'cfg> { Some(&GitReference::Rev(ref s)) => s, None => panic!("not a git source"), }; - let checkout_lock = format!(".cargo-lock-{}-{}", self.ident, - reference_path); - let checkout_lock = try!(self.config.git_checkout_path() - .join(&self.ident) - .open_rw(&checkout_lock, self.config, - "the git checkout")); - let checkout_path = checkout_lock.parent().join(reference_path); + let checkout_path = lock.parent().join("checkouts") + .join(&self.ident).join(reference_path); // Resolve our reference to an actual revision, and check if the // databaes already has that revision. If it does, we just load a @@ -187,7 +175,6 @@ impl<'cfg> Source for GitSource<'cfg> { // swipe our checkout location to another revision while we're using it! self.path_source = Some(path_source); self.rev = Some(actual_rev); - self.checkout_lock = Some(checkout_lock); self.path_source.as_mut().unwrap().update() } diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index df319125d83..329538663ae 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -15,7 +15,7 @@ use toml; use core::shell::{Verbosity, ColorConfig}; use core::MultiShell; use util::{CargoResult, CargoError, ChainError, Rustc, internal, human}; -use util::{Filesystem, LazyCell}; +use util::{Filesystem, FileLock, LazyCell}; use util::toml as cargo_toml; @@ -31,6 +31,7 @@ pub struct Config { extra_verbose: Cell, frozen: Cell, locked: Cell, + git_lock: LazyCell, } impl Config { @@ -47,6 +48,7 @@ impl Config { extra_verbose: Cell::new(false), frozen: Cell::new(false), locked: Cell::new(false), + git_lock: LazyCell::new(), } } @@ -64,12 +66,23 @@ impl Config { pub fn home(&self) -> &Filesystem { &self.home_path } - pub fn git_db_path(&self) -> Filesystem { - self.home_path.join("git").join("db") - } - - pub fn git_checkout_path(&self) -> Filesystem { - self.home_path.join("git").join("checkouts") + pub fn git_path(&self) -> Filesystem { + self.home_path.join("git") + } + + /// All git sources are protected by a single file lock, which is stored + /// in the `Config` struct. Ideally, we should use a lock per repository, + /// but this leads to deadlocks when several instances of Cargo acquire + /// locks in different order (see #2987). + /// + /// Holding the lock only during checkout is not enough. For example, two + /// instances of Cargo may checkout different commits from the same branch + /// to the same directory. So the lock is acquired when the first git source + /// is updated and released when the `Config` struct is destroyed. + pub fn lock_git(&self) -> CargoResult<&FileLock> { + self.git_lock.get_or_try_init(|| { + self.git_path().open_rw(".cargo-lock-git", self, "the git checkouts") + }) } pub fn registry_index_path(&self) -> Filesystem { diff --git a/tests/concurrent.rs b/tests/concurrent.rs index afd8c21e3ef..a2b827f565d 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -482,7 +482,7 @@ fn no_deadlock_with_git_dependencies() { //TODO: use `Receiver::recv_timeout` once it is stable. let recv_timeout = |chan: &::std::sync::mpsc::Receiver<_>| { - for _ in 0..200 { + for _ in 0..3000 { if let Ok(x) = chan.try_recv() { return x }