Auto merge of #2991 - matklad:deadlocks, r=alexcrichton

Deadlocks with git dependencies

Only a test for now to verify that it actually fails on Travis.

Hopefully will close #2987

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<PathSource<'cfg>>,
     rev: Option<GitRevision>,
-    checkout_lock: Option<FileLock>,
     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,28 +126,18 @@ 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)) |
             Some(&GitReference::Tag(ref s)) |
             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()
     }
 

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<bool>,
     frozen: Cell<bool>,
     locked: Cell<bool>,
+    git_lock: LazyCell<FileLock>,
 }
 
 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 {

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..3000 {
+            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))
+    }
+
+}