Skip to content

Commit 8eac1d6

Browse files
alexcrichtonalexcrichton
committed
Fix running Cargo concurrently
Cargo has historically had no protections against running it concurrently. This is pretty unfortunate, however, as it essentially just means that you can only run one instance of Cargo at a time **globally on a system**. An "easy solution" to this would be the use of file locks, except they need to be applied judiciously. It'd be a pretty bad experience to just lock the entire system globally for Cargo (although it would work), but otherwise Cargo must be principled how it accesses the filesystem to ensure that locks are properly held. This commit intends to solve all of these problems. A new utility module is added to cargo, `util::flock`, which contains two types: * `FileLock` - a locked version of a `File`. This RAII guard will unlock the lock on `Drop` and I/O can be performed through this object. The actual underlying `Path` can be read from this object as well. * `Filesystem` - an unlocked representation of a `Path`. There is no "safe" method to access the underlying path without locking a file on the filesystem first. Built on the [fs2] library, these locks use the `flock` system call on Unix and `LockFileEx` on Windows. Although file locking on Unix is [documented as not so great][unix-bad], but largely only because of NFS, these are just advisory, and there's no byte-range locking. These issues don't necessarily plague Cargo, however, so we should try to leverage them. On both Windows and Unix the file locks are released when the underlying OS handle is closed, which means that if the process dies the locks are released. Cargo has a number of global resources which it now needs to lock, and the strategy is done in a fairly straightforward way: * Each registry's index contains one lock (a dotfile in the index). Updating the index requires a read/write lock while reading the index requires a shared lock. This should allow each process to ensure a registry update happens while not blocking out others for an unnecessarily long time. Additionally any number of processes can read the index. * When downloading crates, each downloaded crate is individually locked. A lock for the downloaded crate implies a lock on the output directory as well. Because downloaded crates are immutable, once the downloaded directory exists the lock is no longer needed as it won't be modified, so it can be released. This granularity of locking allows multiple Cargo instances to download dependencies in parallel. * Git repositories have separate locks for the database and for the project checkout. The datbase and checkout are locked for read/write access when an update is performed, and the lock of the checkout is held for the entire lifetime of the git source. This is done to ensure that any other Cargo processes must wait while we use the git repository. Unfortunately there's just not that much parallelism here. * Binaries managed by `cargo install` are locked by the local metadata file that Cargo manages. This is relatively straightforward. * The actual artifact output directory is just globally locked for the entire build. It's hypothesized that running Cargo concurrently in *one directory* is less of a feature needed rather than running multiple instances of Cargo globally (for now at least). It would be possible to have finer grained locking here, but that can likely be deferred to a future PR. So with all of this infrastructure in place, Cargo is now ready to grab some locks and ensure that you can call it concurrently anywhere at any time and everything always works out as one might expect. One interesting question, however, is what does Cargo do on contention? On one hand Cargo could immediately abort, but this would lead to a pretty poor UI as any Cargo process on the system could kick out any other. Instead this PR takes a more nuanced approach. * First, all locks are attempted to be acquired (a "try lock"). If this succeeds, we're done. * Next, Cargo prints a message to the console that it's going to block waiting for a lock. This is done because it's indeterminate how long Cargo will wait for the lock to become available, and most long-lasting operations in Cargo have a message printed for them. * Finally, a blocking acquisition of the lock is issued and we wait for it to become available. So all in all this should help Cargo fix any future concurrency bugs with file locking in a principled fashion while also allowing concurrent Cargo processes to proceed reasonably across the system. [fs2]: https://github.com/danburkert/fs2-rs [unix-bad]: http://0pointer.de/blog/projects/locking.html Closes #354
1 parent 8c60ada commit 8eac1d6

16 files changed

+860
-137
lines changed

‎Cargo.lock

+11
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ docopt = "0.6"
2525
env_logger = "0.3"
2626
filetime = "0.1"
2727
flate2 = "0.2"
28+
fs2 = "0.2"
2829
git2 = "0.4"
2930
git2-curl = "0.4"
3031
glob = "0.2"

‎src/bin/cargo.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ fn is_executable(metadata: &fs::Metadata) -> bool {
258258
}
259259

260260
fn search_directories(config: &Config) -> Vec<PathBuf> {
261-
let mut dirs = vec![config.home().join("bin")];
261+
let mut dirs = vec![config.home().clone().into_path_unlocked().join("bin")];
262262
if let Some(val) = env::var_os("PATH") {
263263
dirs.extend(env::split_paths(&val));
264264
}

‎src/cargo/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ extern crate curl;
99
extern crate docopt;
1010
extern crate filetime;
1111
extern crate flate2;
12+
extern crate fs2;
1213
extern crate git2;
1314
extern crate glob;
1415
extern crate libc;

‎src/cargo/ops/cargo_install.rs

+43-35
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use std::env;
44
use std::ffi::OsString;
55
use std::fs::{self, File};
66
use std::io::prelude::*;
7-
use std::io;
7+
use std::io::SeekFrom;
88
use std::path::{Path, PathBuf};
99

1010
use toml;
@@ -14,10 +14,12 @@ use core::PackageId;
1414
use ops::{self, CompileFilter};
1515
use sources::{GitSource, PathSource, RegistrySource};
1616
use util::{CargoResult, ChainError, Config, human, internal};
17+
use util::{Filesystem, FileLock};
1718

1819
#[derive(RustcDecodable, RustcEncodable)]
1920
enum CrateListing {
2021
V1(CrateListingV1),
22+
Empty,
2123
}
2224

2325
#[derive(RustcDecodable, RustcEncodable)]
@@ -67,9 +69,15 @@ pub fn install(root: Option<&str>,
6769
specify alternate source"))))
6870
};
6971

70-
let mut list = try!(read_crate_list(&root));
71-
let dst = root.join("bin");
72-
try!(check_overwrites(&dst, &pkg, &opts.filter, &list));
72+
// Preflight checks to check up front whether we'll overwrite something.
73+
// We have to check this again afterwards, but may as well avoid building
74+
// anything if we're gonna throw it away anyway.
75+
{
76+
let metadata = try!(metadata(config, &root));
77+
let list = try!(read_crate_list(metadata.file()));
78+
let dst = metadata.parent().join("bin");
79+
try!(check_overwrites(&dst, &pkg, &opts.filter, &list));
80+
}
7381

7482
let target_dir = if source_id.is_path() {
7583
config.target_dir(&pkg)
@@ -82,6 +90,11 @@ pub fn install(root: Option<&str>,
8290
found at `{}`", pkg, target_dir.display()))
8391
}));
8492

93+
let metadata = try!(metadata(config, &root));
94+
let mut list = try!(read_crate_list(metadata.file()));
95+
let dst = metadata.parent().join("bin");
96+
try!(check_overwrites(&dst, &pkg, &opts.filter, &list));
97+
8598
let mut t = Transaction { bins: Vec::new() };
8699
try!(fs::create_dir_all(&dst));
87100
for bin in compile.binaries.iter() {
@@ -103,7 +116,7 @@ pub fn install(root: Option<&str>,
103116
}).extend(t.bins.iter().map(|t| {
104117
t.file_name().unwrap().to_string_lossy().into_owned()
105118
}));
106-
try!(write_crate_list(&root, list));
119+
try!(write_crate_list(metadata.file(), list));
107120

108121
t.bins.truncate(0);
109122

@@ -230,51 +243,40 @@ fn check_overwrites(dst: &Path,
230243
Ok(())
231244
}
232245

233-
fn read_crate_list(path: &Path) -> CargoResult<CrateListingV1> {
234-
let metadata = path.join(".crates.toml");
235-
let mut f = match File::open(&metadata) {
236-
Ok(f) => f,
237-
Err(e) => {
238-
if e.kind() == io::ErrorKind::NotFound {
239-
return Ok(CrateListingV1 { v1: BTreeMap::new() });
240-
}
241-
return Err(e).chain_error(|| {
242-
human(format!("failed to open crate metadata at `{}`",
243-
metadata.display()))
244-
});
245-
}
246-
};
246+
fn read_crate_list(mut file: &File) -> CargoResult<CrateListingV1> {
247247
(|| -> CargoResult<_> {
248248
let mut contents = String::new();
249-
try!(f.read_to_string(&mut contents));
249+
try!(file.read_to_string(&mut contents));
250250
let listing = try!(toml::decode_str(&contents).chain_error(|| {
251251
internal("invalid TOML found for metadata")
252252
}));
253253
match listing {
254254
CrateListing::V1(v1) => Ok(v1),
255+
CrateListing::Empty => {
256+
Ok(CrateListingV1 { v1: BTreeMap::new() })
257+
}
255258
}
256259
}).chain_error(|| {
257-
human(format!("failed to parse crate metadata at `{}`",
258-
metadata.display()))
260+
human("failed to parse crate metadata")
259261
})
260262
}
261263

262-
fn write_crate_list(path: &Path, listing: CrateListingV1) -> CargoResult<()> {
263-
let metadata = path.join(".crates.toml");
264+
fn write_crate_list(mut file: &File, listing: CrateListingV1) -> CargoResult<()> {
264265
(|| -> CargoResult<_> {
265-
let mut f = try!(File::create(&metadata));
266+
try!(file.seek(SeekFrom::Start(0)));
267+
try!(file.set_len(0));
266268
let data = toml::encode_str::<CrateListing>(&CrateListing::V1(listing));
267-
try!(f.write_all(data.as_bytes()));
269+
try!(file.write_all(data.as_bytes()));
268270
Ok(())
269271
}).chain_error(|| {
270-
human(format!("failed to write crate metadata at `{}`",
271-
metadata.display()))
272+
human("failed to write crate metadata")
272273
})
273274
}
274275

275276
pub fn install_list(dst: Option<&str>, config: &Config) -> CargoResult<()> {
276277
let dst = try!(resolve_root(dst, config));
277-
let list = try!(read_crate_list(&dst));
278+
let dst = try!(metadata(config, &dst));
279+
let list = try!(read_crate_list(dst.file()));
278280
let mut shell = config.shell();
279281
let out = shell.out();
280282
for (k, v) in list.v1.iter() {
@@ -291,7 +293,8 @@ pub fn uninstall(root: Option<&str>,
291293
bins: &[String],
292294
config: &Config) -> CargoResult<()> {
293295
let root = try!(resolve_root(root, config));
294-
let mut metadata = try!(read_crate_list(&root));
296+
let crate_metadata = try!(metadata(config, &root));
297+
let mut metadata = try!(read_crate_list(crate_metadata.file()));
295298
let mut to_remove = Vec::new();
296299
{
297300
let result = try!(PackageIdSpec::query_str(spec, metadata.v1.keys()))
@@ -300,7 +303,7 @@ pub fn uninstall(root: Option<&str>,
300303
Entry::Occupied(e) => e,
301304
Entry::Vacant(..) => panic!("entry not found: {}", result),
302305
};
303-
let dst = root.join("bin");
306+
let dst = crate_metadata.parent().join("bin");
304307
for bin in installed.get() {
305308
let bin = dst.join(bin);
306309
if fs::metadata(&bin).is_err() {
@@ -336,7 +339,7 @@ pub fn uninstall(root: Option<&str>,
336339
installed.remove();
337340
}
338341
}
339-
try!(write_crate_list(&root, metadata));
342+
try!(write_crate_list(crate_metadata.file(), metadata));
340343
for bin in to_remove {
341344
try!(config.shell().status("Removing", bin.display()));
342345
try!(fs::remove_file(bin));
@@ -345,13 +348,18 @@ pub fn uninstall(root: Option<&str>,
345348
Ok(())
346349
}
347350

348-
fn resolve_root(flag: Option<&str>, config: &Config) -> CargoResult<PathBuf> {
351+
fn metadata(config: &Config, root: &Filesystem) -> CargoResult<FileLock> {
352+
root.open_rw(Path::new(".crates.toml"), config, "crate metadata")
353+
}
354+
355+
fn resolve_root(flag: Option<&str>,
356+
config: &Config) -> CargoResult<Filesystem> {
349357
let config_root = try!(config.get_path("install.root"));
350358
Ok(flag.map(PathBuf::from).or_else(|| {
351359
env::var_os("CARGO_INSTALL_ROOT").map(PathBuf::from)
352360
}).or_else(move || {
353361
config_root.map(|v| v.val)
354-
}).unwrap_or_else(|| {
355-
config.home().to_owned()
362+
}).map(Filesystem::new).unwrap_or_else(|| {
363+
config.home().clone()
356364
}))
357365
}

‎src/cargo/ops/cargo_rustc/mod.rs

+9-2
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,12 @@ use std::env;
33
use std::ffi::{OsStr, OsString};
44
use std::fs;
55
use std::io::prelude::*;
6-
use std::path::{self, PathBuf};
6+
use std::path::{self, PathBuf, Path};
77
use std::sync::Arc;
88

99
use core::{Package, PackageId, PackageSet, Target, Resolve};
1010
use core::{Profile, Profiles};
11-
use util::{self, CargoResult, human};
11+
use util::{self, CargoResult, human, Filesystem};
1212
use util::{Config, internal, ChainError, profile, join_paths};
1313

1414
use self::job::{Job, Work};
@@ -85,6 +85,13 @@ pub fn compile_targets<'a, 'cfg: 'a>(pkg_targets: &'a PackagesToBuild<'a>,
8585
layout::Layout::new(config, root, Some(&target), &dest)
8686
});
8787

88+
// For now we don't do any more finer-grained locking on the artifact
89+
// directory, so just lock the entire thing for the duration of this
90+
// compile.
91+
let fs = Filesystem::new(host_layout.root().to_path_buf());
92+
let path = Path::new(".cargo-lock");
93+
let _lock = try!(fs.open_rw(path, config, "build directory"));
94+
8895
let mut cx = try!(Context::new(resolve, packages, config,
8996
host_layout, target_layout,
9097
build_config, profiles));

‎src/cargo/sources/git/source.rs

+44-27
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,13 @@
11
use std::fmt::{self, Debug, Formatter};
22
use std::hash::{Hash, Hasher, SipHasher};
33
use std::mem;
4-
use std::path::PathBuf;
54

65
use url::{self, Url};
76

87
use core::source::{Source, SourceId};
98
use core::GitReference;
109
use core::{Package, PackageId, Summary, Registry, Dependency};
11-
use util::{CargoResult, Config, to_hex};
10+
use util::{CargoResult, Config, FileLock, to_hex};
1211
use sources::PathSource;
1312
use sources::git::utils::{GitRemote, GitRevision};
1413

@@ -17,11 +16,11 @@ use sources::git::utils::{GitRemote, GitRevision};
1716
pub struct GitSource<'cfg> {
1817
remote: GitRemote,
1918
reference: GitReference,
20-
db_path: PathBuf,
21-
checkout_path: PathBuf,
2219
source_id: SourceId,
2320
path_source: Option<PathSource<'cfg>>,
2421
rev: Option<GitRevision>,
22+
checkout_lock: Option<FileLock>,
23+
ident: String,
2524
config: &'cfg Config,
2625
}
2726

@@ -30,25 +29,9 @@ impl<'cfg> GitSource<'cfg> {
3029
config: &'cfg Config) -> GitSource<'cfg> {
3130
assert!(source_id.is_git(), "id is not git, id={}", source_id);
3231

33-
let reference = match source_id.git_reference() {
34-
Some(reference) => reference,
35-
None => panic!("Not a git source; id={}", source_id),
36-
};
37-
3832
let remote = GitRemote::new(source_id.url());
3933
let ident = ident(source_id.url());
4034

41-
let db_path = config.git_db_path().join(&ident);
42-
43-
let reference_path = match *reference {
44-
GitReference::Branch(ref s) |
45-
GitReference::Tag(ref s) |
46-
GitReference::Rev(ref s) => s.to_string(),
47-
};
48-
let checkout_path = config.git_checkout_path()
49-
.join(&ident)
50-
.join(&reference_path);
51-
5235
let reference = match source_id.precise() {
5336
Some(s) => GitReference::Rev(s.to_string()),
5437
None => source_id.git_reference().unwrap().clone(),
@@ -57,11 +40,11 @@ impl<'cfg> GitSource<'cfg> {
5740
GitSource {
5841
remote: remote,
5942
reference: reference,
60-
db_path: db_path,
61-
checkout_path: checkout_path,
6243
source_id: source_id.clone(),
6344
path_source: None,
6445
rev: None,
46+
checkout_lock: None,
47+
ident: ident,
6548
config: config,
6649
}
6750
}
@@ -160,7 +143,34 @@ impl<'cfg> Registry for GitSource<'cfg> {
160143

161144
impl<'cfg> Source for GitSource<'cfg> {
162145
fn update(&mut self) -> CargoResult<()> {
163-
let actual_rev = self.remote.rev_for(&self.db_path, &self.reference);
146+
// First, lock both the global database and checkout locations that
147+
// we're going to use. We may be performing a fetch into these locations
148+
// so we need writable access.
149+
let db_lock = format!(".cargo-lock-{}", self.ident);
150+
let db_lock = try!(self.config.git_db_path()
151+
.open_rw(&db_lock, self.config,
152+
"the git database"));
153+
let db_path = db_lock.parent().join(&self.ident);
154+
155+
let reference_path = match self.source_id.git_reference() {
156+
Some(&GitReference::Branch(ref s)) |
157+
Some(&GitReference::Tag(ref s)) |
158+
Some(&GitReference::Rev(ref s)) => s,
159+
None => panic!("not a git source"),
160+
};
161+
let checkout_lock = format!(".cargo-lock-{}-{}", self.ident,
162+
reference_path);
163+
let checkout_lock = try!(self.config.git_checkout_path()
164+
.join(&self.ident)
165+
.open_rw(&checkout_lock, self.config,
166+
"the git checkout"));
167+
let checkout_path = checkout_lock.parent().join(reference_path);
168+
169+
// Resolve our reference to an actual revision, and check if the
170+
// databaes already has that revision. If it does, we just load a
171+
// database pinned at that revision, and if we don't we issue an update
172+
// to try to find the revision.
173+
let actual_rev = self.remote.rev_for(&db_path, &self.reference);
164174
let should_update = actual_rev.is_err() ||
165175
self.source_id.precise().is_none();
166176

@@ -169,22 +179,29 @@ impl<'cfg> Source for GitSource<'cfg> {
169179
format!("git repository `{}`", self.remote.url())));
170180

171181
trace!("updating git source `{:?}`", self.remote);
172-
let repo = try!(self.remote.checkout(&self.db_path));
182+
let repo = try!(self.remote.checkout(&db_path));
173183
let rev = try!(repo.rev_for(&self.reference));
174184
(repo, rev)
175185
} else {
176-
(try!(self.remote.db_at(&self.db_path)), actual_rev.unwrap())
186+
(try!(self.remote.db_at(&db_path)), actual_rev.unwrap())
177187
};
178188

179-
try!(repo.copy_to(actual_rev.clone(), &self.checkout_path));
189+
// Copy the database to the checkout location. After this we could drop
190+
// the lock on the database as we no longer needed it, but we leave it
191+
// in scope so the destructors here won't tamper with too much.
192+
try!(repo.copy_to(actual_rev.clone(), &checkout_path));
180193

181194
let source_id = self.source_id.with_precise(Some(actual_rev.to_string()));
182-
let path_source = PathSource::new_recursive(&self.checkout_path,
195+
let path_source = PathSource::new_recursive(&checkout_path,
183196
&source_id,
184197
self.config);
185198

199+
// Cache the information we just learned, and crucially also cache the
200+
// lock on the checkout location. We wouldn't want someone else to come
201+
// swipe our checkout location to another revision while we're using it!
186202
self.path_source = Some(path_source);
187203
self.rev = Some(actual_rev);
204+
self.checkout_lock = Some(checkout_lock);
188205
self.path_source.as_mut().unwrap().update()
189206
}
190207

0 commit comments

Comments
 (0)