RFC: Globally Locked Cargo

[Byron]

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]

r? @alexcrichton

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

[alexcrichton]

I think this lockfile may need to be regenerated.

[Byron]

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.

[alexcrichton]

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]

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]

[...] 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]

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]

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]

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]

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]

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

[bors]

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

[bors]

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