add file locks to prevent git checkout races #3484
Conversation
There was a problem hiding this comment.
Thanks for tackling this @bingcicle!
I'm not sure the Filesystem abstraction is actually providing us much extra safety here, as its trivial for future contributions to ignore the lock and read/write files as they see fit, and Rust doesn't give us any other way to guard against I/O 🤔
That said, I'm also not sure we really need that level of safety at the moment, as our point of contention appears to be in a single place - git checkouts.
What are your thoughts on removing forc-fs (and the Filesystem abstraction), and simply using fslock or fd-lock directly within fetch_git? We should still link your commit with forc-fs to #919 as it might make for a good foundation if we need a more sophisticated Filesystem abstraction in the future.
forc-fs/Cargo.toml
Outdated
| description = "Filesystem abstraction for Forc" | ||
|
|
||
| [dependencies] | ||
| file-lock = "2.1" |
There was a problem hiding this comment.
Ahh I saw fslock but not fd-lock. file-lock seemed at the time to me was more mature than fslock, but it seems like fd-lock is a better alternative since its from a core maintainer. Will re-write using that.
forc-pkg/src/pkg.rs
Outdated
| let path = git_commit_path(name, &pinned.source.repo, &pinned.commit_hash); | ||
| // Checkout the pinned hash to the path. | ||
| with_tmp_git_repo(fetch_id, name, &pinned.source, |repo| { | ||
| // Change HEAD to point to the pinned commit. | ||
| let id = git2::Oid::from_str(&pinned.commit_hash)?; | ||
| repo.set_head_detached(id)?; | ||
|
|
||
| let _lock = path.open_rw(".forc-lock")?; |
There was a problem hiding this comment.
This should be a const.
Yeah, while looking into locks it seems like advisory locks are only as strong as the maintainers choose to abide by them. We would have to enforce using
This sounds good, I'll edit the original PR content to close only #2603 then and link it to #919 instead. |
eightfilms
changed the title
forc-fs
forc-pkg/src/pkg.rs
Outdated
| fs::OpenOptions::new() | ||
| .write(true) | ||
| .create(true) | ||
| .open(path.join(FORC_FILE_LOCK_NAME))?, |
There was a problem hiding this comment.
Could we move this expression into a re-usable function? E.g.
/// Given the path to a directory, lock the directory using a file lock.
///
/// The directory is "unlocked" when the returned `RwLock` is dropped.
///
/// This was introduced to avoid racing on the creation of git checkout directories.
fn lock_dir(dir_path: &Path) -> io::Result<RwLock<File>> {
let file = fs::OpenOptions::new()
.write(true)
.create(true)
.open(path.join(FORC_FILE_LOCK_NAME))?;
Ok(RwLock::new(file))
}
forc-pkg/src/pkg.rs
Outdated
| ); | ||
| let _ = lock.write()?; | ||
|
|
||
| let _ = std::fs::remove_dir_all(&repo_dir); |
There was a problem hiding this comment.
Wait, if we remove the directory here, don't we also remove our lock file?
There was a problem hiding this comment.
I should probably convert this to a draft - I've realized now that in my original PR it was incorrectly implemented as well. Also, file-lock is based on fnctl() which is different from fd-lock which is based on flock(). For whatever reason initializing the lock at the same line did not work for fd-lock - still looking into this and trying to understand how flock() compares against fnctl().
This was me just moving the lock around to see which would work
forc-pkg/src/pkg.rs
Outdated
| .open(path.join(FORC_FILE_LOCK_NAME))?, | ||
| ); | ||
| let _ = lock.write()?; | ||
| let _ = std::fs::remove_dir_all(&path); |
|
Closing this in favor of @mitchmindtree 's #3592. |
This introduces an advisory lock around the re-usable git checkout repo directories. Closes #2603, #3484. Unfortunately, it's a little tricky to reliably test this from this project repository as doing so requires creating a test project with one or more `git` dependencies. We can't reliably add git dependencies (that point to external repos) to projects within this repo as the dependency would necessarily be at least one commit of forc behind, and as a result it's likely that these dependencies would break at some point in the future, e.g. next time forc makes some breaking change. @segfault-magnet, @hal3e would you mind testing this branch in your use-case mentioned in #2603 to make sure we've covered the issue? Edit: I've managed to test this locally by adding some long `sleep`s immediately after acquiring the lock guard, and then spinning up multiple processes of `forc` locally to build a toy project with multiple git dependencies. All appears to work fine, with each process blocking and waiting its turn for access to the checkout repo. Co-authored-by: bing <bingcicle@proton.me>
Closes #2603
Related #919
Introduces Filesystem abstraction as well as locking. Mostly inspired bycargo's flock but using file-lock instead.EDIT: We've abandoned the attempt to introduce Filesystem abstraction (see comment below). This PR will introduce locks only, and instead of file-lock, we will use fd-lock.
Similar to
cargowe open a.forc-lockfile at the root of the directory we want to lock - this is unlocked on Drop.Tested this on a fork of
fuels-rsin my personal repo here. This is using the same setup in the issue raised here: #2603On first glance in terms of speed, it doesn't seem to offer much improvement over the time spent building contracts when ran with
-n 1, at least from just glancing at the duration of the build step in the fuels-rs repo. Improvements in speed aside, introducingfd-lockat least seems to allow concurrent builds.