RFC: Globally Locked Cargo #1781

Byron · Jul 4, 2015

This PR is the successor of 'Concurrent Cargo' and uses a single lock to assure multiple cargo invocations will not interfere with each other in an undefined fashion.

Implementation Details

The file-lock crate provides the actual lock implementation. It allows to try obtaining a lock (non-blocking) or to wait until the lock was obtained. All this is done using standard operating system facilities which are available on both windows and posix-compatible systems.

The lock-file is persistent and is currently placed in ${CARGO_HOME}/.global-lock. Please note it s not comparable to the locking mechanism git uses, as the latter doesn't support blocking until a lock is obtained.

Using the cargo configuration and the key build.lock-kind, it is possible to change the default from nowait to wait and thus cause Cargo processes to wait for each other.

Open Questions

In order of perceived relevance:

How can I implement a test-case which doesn't suffer from races ?
- The preliminary test only works occasionally, and for some reason the spawned background process starts after the foreground process and thus fails to obtain the lock. Adding sleep_ms seems like the wrong thing to do.
Should the lock better be obtained in cargo::process(...) ?
Which name would you prefer for the lock file ?
Which configuration key would you prefer ? The current one seems not to be too fitting.

Testing

Manual tests indicate the system works as expected.

$ git clone https://github.com/Byron/google-apis-rs
$ cd google-apis-rs
# Build 4 targets with plenty of dependencies in parallel. Depending on the `build.jobs` variable 
# in `.cargo.config`, there is more or less intra-process contention.
$ CARGO_HOME=$PWD/cargo_home make -j4 groupsmigration1-cli-cargo discovery1-cli-cargo translate2-cli-cargo audit1-cli-cargo ARGS=build

The .cargo/config contained in the aforementioned repository is used to configure cargo. Currently it looks like this:

[build]
target-dir = "target"
lock-kind = "wait"

Work still to be done

Tests for both, blocking and nonblocking, lock options
Support for the windows platform.
- The latter is achieved through improvements to the file-lock crate and should be transparent to cargo.

rust-highfive · Jul 4, 2015

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · Jul 6, 2015

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton · Jul 6, 2015

Cargo.lock

@@ -93,6 +94,26 @@ dependencies = [
 ]

 [[package]]
+name = "errno"


I think this lockfile may need to be regenerated.

Regenerating it does not remove the errno entry - it just updates dependencies to the latest patch-levels. I am not sure if this is what you intended.

Oh right, duh! Forgot this was a dependency of file-lock.

That being said I believe that this crate doesn't provide anything beyond io::Error::last_os_error() and io::Error::raw_os_error(), so I'd prefer if the file-lock crate were adjusted to not have this dependency.

alexcrichton · Jul 6, 2015

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton · Jul 6, 2015

tests/test_cargo_build_lib.rs

+    let fg_process = bg_process.clone();
+    let mut bg_process_handle = bg_process.build_command().spawn().unwrap();
+
+    assert_eq!(fg_process.build_command().output().unwrap().status.code().unwrap(), 101);


This test looks like it runs a risk of being flaky because of various bits of timing here and there, and I think it'd also be useful to match the stdout/stderr here as well.

Yes, this is also why it doesn't work most of the time. I am not sure how to make it work without having to make the two processes communicate (i.e. master waits for child to come up).

Maybe I could launch the cargo implementation from within the test-case directly (without a sub-process), and then spawn the test-executable. That would give me more control, and is similar to the working tests in the file-lock crate.

Hm yeah, that sounds like a good idea!

alexcrichton · Jul 6, 2015

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton · Jul 6, 2015

src/cargo/util/lock.rs

+    fn from(t: ParseError) -> Self {
+        Box::new(t)
+    }
+}


Could you add these to src/cargo/util/errors.rs? That's currently where these conversions and trait implementations are housed.

alexcrichton · Jul 6, 2015

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton · Jul 6, 2015

Cargo.toml

@@ -36,6 +36,10 @@ toml = "0.1"
 url = "0.2"
 winapi = "0.1"

+[dependencies.file-lock]
+git = "https://github.com/Byron/file-lock"
+branch = "cargo-adjustments"


Before merging I'd prefer to depend on a crates.io version of this library

Absolutely - my preferred path of action is to get the cargo-related integration right, then port file-lock to windows, and see if cargo indeed works and builds on all supported platforms. Probably you can ask bors to do that, as travis can't test windows for instance.

Once it does work everywhere, a new crates.io release of the original crate should be made (I just have a fork).

I'll see if I can't set up appveyor CI for cargo, that should give us some Windows coverage for PRs at least.

alexcrichton · Jul 6, 2015

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton · Jul 6, 2015

src/cargo/util/lock.rs

+        const CONFIG_KEY: &'static str = "build.lock-kind";
+        match try!(config.get_string(CONFIG_KEY)).map(|t| t.0) {
+            None => Ok(LockKind::NonBlocking),
+            Some(kind_string) => match kind_string.parse() {


I'm a little uneasy putting this parsing of configuration into an external crate, it seems like it's easy to make a change upstream which looks like it's not a breaking change but turns out to actually be a breaking change.

I'm a little uneasy putting this parsing of configuration into an external crate [...]

Do you mean external module ? Should I put the configuration handling into the Config type ?

In this case the FromStr implementation is located in the file_lock crate, and I would expect the actual parsing of a value to happen in this crate. If we're just gonna go with "always wait" though this isn't so bad.

alexcrichton · Jul 6, 2015

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton · Jul 6, 2015

src/cargo/util/lock.rs

+    }
+
+    pub fn lock(&mut self) -> CargoResult<()> {
+        // NOTE(ST): This could fail if cargo is run concurrently for the first time


This "NOTE(ST)" and "TODO(ST)" isn't currently present throughout the rest of Cargo, can you remove these annotations?

alexcrichton · Jul 6, 2015

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton · Jul 6, 2015

src/cargo/util/lock.rs

+                    return Err(caused_human(format!("Failed to create parent directory of \
+                                                     lock-file at '{}'", 
+                                                     lock_dir.display()), io_err));
+                }


This can be replaced with try! + chain_error

alexcrichton · Jul 6, 2015

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton · Jul 6, 2015

src/cargo/util/lock.rs

+
+    pub fn lock_kind(config: &Config) -> CargoResult<LockKind> {
+        // TODO(ST): rename config key to something more suitable
+        const CONFIG_KEY: &'static str = "build.lock-kind";


I think it's fine to avoid this for now and just pick a reasonable default behavior

Do you mean cargo should just always abort, or always wait, without an option to configure it ?

For now yeah, and I'd probably choose "always wait" over "always abort"

alexcrichton · Jul 6, 2015

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton · Jul 6, 2015

src/bin/cargo.rs

+                                try!(CargoLock::lock_kind(config)));
+
+    try!(fl.lock().chain_error(|| {
+        human("Failed to obtain global cargo lock")


I think that we'll want to improve this error message slightly, for example how can this come up? If it comes up because another Cargo is running, then it should probably say that, but if it just failed because of an I/O error we may also want to say that.

I think that we'll want to improve this error message slightly, for example how can this come up?

Good idea. Also to check for IOErrors specifically. These shouldn't happen though, and will be provided as cause as FileLock errors are declared human right now.

alexcrichton · Jul 6, 2015

I think that to start off with we can just pick a reasonable default (either wait or don't wait), and in the case of waiting I also think that a message needs to be printed to the effect of "we're now waiting for some other Cargo to exit"

Byron · Jul 6, 2015

[...] and in the case of waiting I also think that a message needs to be printed to the effect of "we're now waiting for some other Cargo to exit"

Unfortunately this doesn't work, as we don't know that we are waiting in case we do so. We can only tell we didn't get a lock if we tried to get a lock first, then print "we are waiting" and then wait on the lock. However, that would clearly be a race.

So far I have the impression you favour to not make the handling configurable, which to my mind would restrict the usefulness of the implementation. After all, both behaviours, wait and nowait have their benefits, and the file-lock crate can handle both.

Maybe I just misunderstood you in this regard.

alexcrichton · Jul 7, 2015

However, that would clearly be a race.

Could you elaborate on the race here? I'm not quite sure what the problem would be in this case.

So far I have the impression you favour to not make the handling configurable, which to my mind would restrict the usefulness of the implementation.

Yeah I think it's fine to perhaps expand into this in the future, but I'd prefer to get some experience with the current implementation before getting more ambitious, and I figure that waiting-by-default is a reasonable way to start out.

Byron · Jul 7, 2015

Thanks for the replies, I believe I know enough now to prepare everything for a next review. I will let you know once I think I addressed all issues.

Good luck with getting a window CI environment ready ! I wonder why it's not possible to build cargo along with rustc, which certainly is already tested on all supported platforms. After all, cargo is bundled with rustc and expected to run equally well on all supported platforms.

Could you elaborate on the race here? I'm not quite sure what the problem would be in this case.

A non-waiting (try) lock followed by blocking lock in case try-lock failed would cause cargo to print "waiting for other process". That other process could, while we are printing this for example, drop the lock, and we get the lock without blocking at all. Two sys-calls in short succession are always a race, to my mind. One could argue that we are talking about a fraction of a second, and even if there is a race, the worst thing that could happen is a message printed for no real reason.

alexcrichton · Jul 7, 2015

That other process could, while we are printing this for example, drop the lock, and we get the lock without blocking at all.

This doesn't sound like a race condition, just something that can happen? There's no harm in doing this and if Cargo continues quickly then even better!

alexcrichton · Jul 8, 2015

Ah and I've now set up AppVeyor CI for Cargo so when this PR is rebased I think it'll trigger a new CI build on Windows.

bors · Jul 29, 2015

☔️ The latest upstream changes (presumably #1860) made this pull request unmergeable. Please resolve the merge conflicts.

bors · Aug 4, 2015

☔️ The latest upstream changes (presumably #1830) made this pull request unmergeable. Please resolve the merge conflicts.

bors · Aug 17, 2015

☔️ The latest upstream changes (presumably #1885) made this pull request unmergeable. Please resolve the merge conflicts.

bors · Oct 6, 2015

☔️ The latest upstream changes (presumably #2022) made this pull request unmergeable. Please resolve the merge conflicts.

bors · Oct 21, 2015

☔️ The latest upstream changes (presumably #2061) made this pull request unmergeable. Please resolve the merge conflicts.

bors · Nov 19, 2015

☔️ The latest upstream changes (presumably #2154) made this pull request unmergeable. Please resolve the merge conflicts.

Byron · Dec 7, 2015

Thanks for the time invested into this PR. However, I have to face the bitter truth that I won't be the one finishing this.

The latest attempt to bring it up-to-date with master revealed that libc has moved or renamed constants used by the file-lock crate, which now would need additional work - something inevitable considering the required windows port.

Therefore I believe it's best to close the issue to prevent it from being dragged along into 2016.

rust-highfive assigned alexcrichton Jul 4, 2015

Byron referenced this pull request Jul 5, 2015
Closed
Concurrent Builds #122

Byron referenced this pull request Jul 21, 2015
Closed
Run rustc/cargo in the background to avoid blocking UI #17

Merge branch 'master' into global-lock

Loading status checks…

4e98db0

Merge remote-tracking branch 'origin/master' into global-lock

Loading status checks…

4c4da88

Merge remote-tracking branch 'origin/master' into global-lock

Loading status checks…

367749f

Merge remote-tracking branch 'origin/master' into global-lock

Loading status checks…

7461f8b

Merge remote-tracking branch 'origin/master' into global-lock
It builds, some tests fail though

Loading status checks…

e83b567

Byron closed this Dec 7, 2015

alexcrichton referenced this pull request Mar 15, 2016
Merged
Fix running Cargo concurrently #2486

rust-lang/cargo

Join GitHub today

RFC: Globally Locked Cargo #1781

Conversation

Byron commented Jul 4, 2015

Implementation Details

Open Questions

Testing

Work still to be done

Byron added some commits Jul 4, 2015

rust-highfive assigned alexcrichton Jul 4, 2015

This comment has been minimized.

rust-highfive commented Jul 4, 2015

Byron referenced this pull request Jul 5, 2015

alexcrichton reviewed Jul 6, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 6, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 6, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Jul 6, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 6, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 6, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Jul 6, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Jul 6, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Jul 6, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Jul 6, 2015

This comment has been minimized.

Byron commented Jul 6, 2015

This comment has been minimized.

alexcrichton commented Jul 7, 2015

This comment has been minimized.

Byron commented Jul 7, 2015

This comment has been minimized.

alexcrichton commented Jul 7, 2015

Byron added some commits Jul 8, 2015

This comment has been minimized.

alexcrichton commented Jul 8, 2015

Byron referenced this pull request Jul 21, 2015

This comment has been minimized.

bors commented Jul 29, 2015

This comment has been minimized.

bors commented Aug 4, 2015

This comment has been minimized.

bors commented Aug 17, 2015

This comment has been minimized.

bors commented Oct 6, 2015

This comment has been minimized.

bors commented Oct 21, 2015

This comment has been minimized.

bors commented Nov 19, 2015

This comment has been minimized.

Byron commented Dec 7, 2015

Byron closed this Dec 7, 2015

alexcrichton referenced this pull request Mar 15, 2016

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton reviewed Jul 6, 2015

View changes

alexcrichton reviewed Jul 6, 2015

View changes