From 7b44acc6eb13bc9eeb812ecffc0046a115bd9bc5 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 15 May 2017 12:09:16 -0700 Subject: [PATCH] Fix a flaky concurrent test with correct locking The recent refactoring to clone the bare registry left in an accidental path where index checkouts could clobber one another. This commit updates the logic with proper locking and attempt ordering, attempting a full retry of the open operation after the index locked. --- src/cargo/sources/registry/remote.rs | 33 ++++++++++++++++++---------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/cargo/sources/registry/remote.rs b/src/cargo/sources/registry/remote.rs index 78c9cb5b5fa..4852bf1d731 100644 --- a/src/cargo/sources/registry/remote.rs +++ b/src/cargo/sources/registry/remote.rs @@ -55,13 +55,18 @@ impl<'cfg> RemoteRegistry<'cfg> { self.repo.get_or_try_init(|| { let path = self.index_path.clone().into_path_unlocked(); + // Fast path without a lock + if let Ok(repo) = git2::Repository::open(&path) { + return Ok(repo) + } + + // Ok, now we need to lock and try the whole thing over again. + let lock = self.index_path.open_rw(Path::new(INDEX_LOCK), + self.config, + "the registry index")?; match git2::Repository::open(&path) { Ok(repo) => Ok(repo), Err(_) => { - self.index_path.create_dir()?; - let lock = self.index_path.open_rw(Path::new(INDEX_LOCK), - self.config, - "the registry index")?; let _ = lock.remove_siblings(); Ok(git2::Repository::init_bare(&path)?) } @@ -153,6 +158,7 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { "the registry index")?; self.config.shell().status("Updating", format!("registry `{}`", self.source_id.url()))?; + let mut needs_fetch = true; if self.source_id.url().host_str() == Some("github.com") { if let Ok(oid) = self.head() { @@ -160,18 +166,21 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { debug!("attempting github fast path for {}", self.source_id.url()); if github_up_to_date(&mut handle, self.source_id.url(), &oid) { - return Ok(()) + needs_fetch = false; + } else { + debug!("fast path failed, falling back to a git fetch"); } - debug!("fast path failed, falling back to a git fetch"); } } - // git fetch origin master - let url = self.source_id.url().to_string(); - let refspec = "refs/heads/master:refs/remotes/origin/master"; - git::fetch(&repo, &url, refspec, self.config).chain_error(|| { - human(format!("failed to fetch `{}`", url)) - })?; + if needs_fetch { + // git fetch origin master + let url = self.source_id.url().to_string(); + let refspec = "refs/heads/master:refs/remotes/origin/master"; + git::fetch(&repo, &url, refspec, self.config).chain_error(|| { + human(format!("failed to fetch `{}`", url)) + })?; + } self.head.set(None); *self.tree.borrow_mut() = None; Ok(())