Use fs2 instead of fmutex for cross-platform mutex support #170
Conversation
|
This is a work in progress, @rossmacarthur let me know what we should do for the failing test case |
There was a problem hiding this comment.
Hey @kvnxiao, thanks for the contribution! I have made some comments. I think we will also need to run the tests on windows in GitHub Actions, you should hopefully only have to add this to the Test job.
- { os: windows-latest, target: x86_64-pc-windows-msvc }
- { os: windows-latest, target: aarch64-pc-windows-msvc }
| Err(_) if !matches!(command, Command::Lock | Command::Source) => None, | ||
| Err(err) => { | ||
| return Err(err).context("failed to acquire lock on config directory"); | ||
| let file_mutex = match acquire_mutex(ctx.config_dir()) { |
There was a problem hiding this comment.
This pattern doesn't release the lock in the err case or if anything panics. fmutex had an RAII pattern which ensured this. Either we need to build that or we should use a different library.
| @@ -33,7 +33,7 @@ casual = "0.2.0" | |||
| clap_complete = "4.4.4" | |||
| constcat = "0.4.0" | |||
| curl = "0.4.44" | |||
| fmutex = "0.1.0" | |||
| fs2 = "0.4.3" | |||
There was a problem hiding this comment.
I'm hesitant to use this crate because it is unmaintained, the last version was published 6 years ago! Looking on crates.io a good option to try might be fd-lock
| /// file within the config directory. | ||
| #[cfg(target_os = "windows")] | ||
| fn get_file_for_mutex(path: &Path) -> io::Result<fs::File> { | ||
| let windows_lockfile = path.join(".windowslock"); |
There was a problem hiding this comment.
I think windows in this name redundant. Just .lock is fine, however it might be good to see if there is a standard name that tools use for these types of files?
| let file_open = get_file_for_mutex(path); | ||
| let file = match file_open { | ||
| Ok(file) => file, | ||
| Err(err) => return Err(anyhow!("failed to open `{}`: {}", path.display(), err)), |
There was a problem hiding this comment.
Can use .with_context(..) here to preserve the error chain.
| @@ -175,6 +175,7 @@ fn lock_and_source_clean() -> io::Result<()> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| #[cfg(not(target_os = "windows"))] | |||
There was a problem hiding this comment.
If possible we should create a test which tests the same thing on windows. The test just needs to get an IO error when attempting to clean one the files.
| /// Returns the file to use for the filesystem mutex. On Windows, this is usually a lock | ||
| /// file within the config directory. | ||
| #[cfg(target_os = "windows")] | ||
| fn get_file_for_mutex(path: &Path) -> io::Result<fs::File> { |
There was a problem hiding this comment.
I would suggest adding a method on Context to return the Path for the file lock.
| @@ -19,9 +19,9 @@ checksum = "7079075b41f533b8c61d2a4d073c4676e1f8b249ff94a393b0595db304e0dd87" | |||
|
|
|||
| [[package]] | |||
| name = "anyhow" | |||
| version = "1.0.75" | |||
| version = "1.0.76" | |||
There was a problem hiding this comment.
Did you intentionally bump a bunch of these deps? I don't see how adding fs2 would have changed this 🤔.
|
@rossmacarthur going to have to leave this draft open for a while. I encountered issues aside from the filesystem mutex on Windows, with regards to cloning repositories. Half the time repos won't clone properly when running |
|
@kvnxiao could you elaborate on the issues you encountered in case someone else wants to tackle this in the future? |
|
Honestly not sure what the exact issue was since I don't have the time to investigate deeply but I summarized in my previous comment how the git clone steps fails for random plugins as well as tests |
For resolving #167
Excerpt for error message from failing to perform
File.read: