brunoczim / fslock

File locking for Rust.
MIT License
40 stars 11 forks source link

version 0.2.0 lock succeeds while version 0.1.6 is still holding the lock #7

Closed rkuhn closed 2 years ago

rkuhn commented 2 years ago

I’m using a very simple function to logically lock a directory:

pub fn lock_working_dir(working_dir: impl AsRef<std::path::Path>) -> anyhow::Result<fslock::LockFile> {
    let mut lf =
        fslock::LockFile::open(&working_dir.as_ref().join("lockfile")).context("Error opening file `lockfile`")?;
    if !lf.try_lock().context("Error locking file `lockfile`")? {
        return Err(WorkdirLocked(working_dir.as_ref().display().to_string()).into());
    }
    Ok(lf)
}

This works fine with version 0.1.6. I now added a new command line tool that shall abide by the same rules and cargo add fslock gave me version 0.2.0. To my surprise, while running the old program the new tool succeeded (i.e. violated the contract of the lock). Downgrading the new tool to use 0.1.6 fixes the issue.

brunoczim commented 2 years ago

What is the state of the lock when this code runs?

rkuhn commented 2 years ago

Not sure what you’re asking: I have a process running having locked the file with version 0.1.6, and when I then run the new process with 0.2.0 the try_lock succeeds. I added a debug log which prints:

locked LockFile { locked: true, desc: 9 }
rkuhn commented 2 years ago

@brunoczim Diving a bit into the code I see that version 0.2 uses flock while 0.1 uses lockf — these mechanisms are independent of each other, which explains my observation. The bad thing is that I probably won’t ever be able to update since my “service” process may well use a much older version (i.e. fslock 0.1) than my “cli” process (or the other way around).

brunoczim commented 2 years ago

I think there is not much to do :/

Not that I could think right now

v0.1.x had a problem that would drop the lock if ANY handle was closed within the process (for Unix), exactly because of lockf behaviour

rkuhn commented 2 years ago

Ah, that’s a good reason. Perhaps add a big fat warning to the docs to highlight this breaking change? I can try my hand at it if you want.

brunoczim commented 2 years ago

I ended up adding a warning in the CHANGELOG

I just published v0.2.1 and I did not see your reply, but I think I should have warned it in the docs too, my bad

brunoczim commented 2 years ago

I added an warning with aea9d72a8864e835ef2698605115f3b17ac21e52 (branch dev), so it will appear in a next release, but I don't know when the release will happen.

rkuhn commented 2 years ago

Yes, that’s good, thanks! I added a possible enhancement for your consideration.