yoshuawuyts / fd-lock

Advisory cross-platform file locks using file descriptors
Apache License 2.0
75 stars 18 forks source link

How to write `try_write` followed by `write`? #28

Open sunfishcode opened 2 years ago

sunfishcode commented 2 years ago

This program does a try_write(), and if that fails due to another process holding the lock, prints a message, and then does a write() to wait until the lock is available:

use std::io::Write;
use fd_lock::RwLock;

fn main() -> std::io::Result<()> {
    let file = std::fs::File::create("test.file")?;
    let mut lock = RwLock::new(file);

    let mut guard = match lock.try_write() {
        Ok(guard) => guard,
        Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => {
            eprintln!("blocking on other processes...");
            lock.write()?
        }
        Err(err) => return Err(err),
    };

    eprintln!("writing to the file");

    writeln!(&mut guard, "hello")?;

    Ok(())
}

However, this doesn't compile:

error[E0499]: cannot borrow `lock` as mutable more than once at a time
  --> examples/foo.rs:12:13
   |
8  |     let mut guard = match lock.try_write() {
   |                           ----------------
   |                           |
   |                           first mutable borrow occurs here
   |                           a temporary with access to the first borrow is created here ...
...
12 |             lock.write()?
   |             ^^^^^^^^^^^^ second mutable borrow occurs here
...
15 |     };
   |      - ... and the first borrow might be used here, when that temporary is dropped and runs the destructor for type `Result<fd_lock::RwLockWriteGuard<'_, File>, std::io::Error>`

Is there a way to do what this code is doing that avoids this problem?

Alternatively, would it makes sense for try_write and write to take a &self instead of a &mut self? They don't need the mut for any of their own state.

Imberflur commented 1 year ago

I found a solution that works with this specific example:

fn main() -> std::io::Result<()> {
    let mut lock = RwLock::new(File::open("test")?);

    // store in a variable so we can later explicitly drop it during the match
    let try_result = lock.try_write(); 
    let mut guard = match try_result {
        Ok(guard) => guard,
        // only borrow the error here via `ref`, instead of moving it out since that would 
        // prevent the explicit early drop of `try_result`
        Err(ref err) if err.kind() == std::io::ErrorKind::WouldBlock => {
            drop(try_result);
            eprintln!("blocking on other processes...");
            lock.write()?
        }
        Err(err) => return Err(err),
    };

    let _use = &mut guard;
    Ok(())
}

However, if you need to return the guard from a function, it will trigger an instance of https://docs.rs/polonius-the-crab/0.3.1/polonius_the_crab/#rationale-limitations-of-the-nll-borrow-checker. Eventually the polonius borrow checker should allow cases like this to work IIUC.

Alternatively, would it makes sense for try_write and write to take a &self instead of a &mut self? They don't need the mut for any of their own state.

As I noted in https://github.com/yoshuawuyts/fd-lock/pull/30#issuecomment-1387523367, system APIs can allow conflicting lock calls to succeed if the same file handle is used. Potentially, additional synchronization could be added internally in fd-lock to allow this to take &self and keep the same functionality (this could be used to also support Arc based guard types). Alternatively, APIs to specifically support this try_write then write case could be introduced.

Andrew15-5 commented 1 month ago

Wow, thanks for that @Imberflur. I didn't think of that, but it works and compiles now. Previously, I cloned File and created a new RwLock in the Err arm, which wasn't pretty or elegant at all. Oh, but I had to add ref to Err(ref err) if err.kind() == ErrorKind::WouldBlock => { even though I don't use err inside the code block. :/

I have 2 small questions that are just for this use case (addressed to anyone that knows). The docs only use File::open() so here is what I wanna know:

  1. Do I have to use read-only open for the library to work correctly, or can I do a more ergonomic:
            let file = match OpenOptions::new()
                .create(true)
                .truncate(true)
                .write(true)
                .open(lock_file_path.as_path())

since I want to delete the file after its usage.

  1. Can I safely use remove_file() before the drop is called?

So far I tried both and it looks like it works just fine.