Use a single lock for all git repositories

diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs
index c2254aa976e523e78dae2ec14a4941a834736e3a..3ab8b79c63828f2335aa107f6418ce2197fbe82c 100644 (file)
--- 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<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,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 df319125d83a8a88d2720b5eeea8cdfb84344702..329538663aea7b3590dc7f4f61920c3edcddbb39 100644 (file)
--- 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<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 {

diff --git a/tests/concurrent.rs b/tests/concurrent.rs
index afd8c21e3efa445ac9808fd201969787d618426b..a2b827f565dd33d600af2b37bb5999b151851040 100644 (file)
--- 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
             }