Tracking Issue for File lock API

[cberner]

Feature gate: #![feature(file_lock)]

This is a tracking issue for rust-lang/libs-team#412

This feature exposes advisory file locks on File. They allow a file handle to acquire an exclusive or shared file lock, which blocks other file handles to the same file from acquiring a conflicting lock. Some semantics are platform dependent, and these are documented in the API documentation.

Public API

impl File {
    fn lock(&self) -> io::Result<()>;
    fn lock_shared(&self) -> io::Result<()>;
    fn try_lock(&self) -> io::Result<bool>;
    fn try_lock_shared(&self) -> io::Result<bool>;
    fn unlock(&self) -> io::Result<()>;
}

Steps / History

• Implementation: Implement file_lock feature #130999
• Final comment period (FCP)¹
• Signature changed due to feedback: Change signature of File::try_lock and File::try_lock_shared #139343
• Signature changed again due to feedback: Change File::try_lock() and try_lock_shared() to return io::Result<()> #140718
• Stabilization PR

Unresolved Questions

• None yet.

Footnotes

1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩

[workingjubilee]

apparently not supported on all tier 2 OS: #132921

[eric-seppanen]

Note that this triggers the unstable_name_collisions lint for code using the fs2/fs3/fs4 crates, all of which have a FileExt trait with methods called lock_shared, try_lock_shared, and unlock.

This lint fires on the 1.84 beta release, so there might be a number of people who discover this once 1.84 stable goes out.

[NilsIrl]

It would also be useful to atomically create and lock a file. This is possible on MacOS using the O_SHLOCK and O_EXLOCK flags on open and on Linux using O_TMPFILE.

[joshtriplett]

While there are more features we may want to add to this in the future, the current state of this seems useful, and works. Stabilizing it would let people who encounter the warnings about a future conflict switch to the new API.

Shall we stabilize the current File locking APIs?

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

Concerns:

• api-is-unergonomic (Tracking Issue for File lock API #130994 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[m-ou-se]

I'd like to see some documentation on the lock methods that make it clear that you don't need to manually unlock / that the lock is automatically unlocked when the File is dropped. Right now, that's only documented on the unlock method.

Other than that, the documentation could be made a lot less confusing by adding 'by another process' and 'by the same process' in a few places. Right now, the try methods say "Returns false if the file is locked.", but then go on to say it might deadlock if it's already locked. I assume the former should be "locked by another process" and the latter should be "locked by this process".

[m-ou-se]

The unlock method should document whether it's okay to call it if no locks are held.

If calling unlock() is only acceptable when a lock is actually held, this should probably be a Guard style of API. If it's always okay to call unlock(), the current design makes sense to me.

[m-ou-se]

I'm checking my box with the assumption that these are just small docs changes that we'll do during/before stabilization.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[joshtriplett]

I've submitted #136288 which should address all the documentation requests.

@m-ou-se wrote:

I'd like to see some documentation on the lock methods that make it clear that you don't need to manually unlock / that the lock is automatically unlocked when the File is dropped. Right now, that's only documented on the unlock method.

Done.

Other than that, the documentation could be made a lot less confusing by adding 'by another process' and 'by the same process' in a few places. Right now, the try methods say "Returns false if the file is locked.", but then go on to say it might deadlock if it's already locked. I assume the former should be "locked by another process" and the latter should be "locked by this process".

"process" isn't the granularity here, but I've added clear distinctions about locks acquired via the same handle/descriptor (may deadlock) vs locks acquired via a different handle/descriptor (will block the blocking methods or make the try_ methods return Ok(false)). I've also clarified the return value documentation to avoid that ambiguity.

The unlock method should document whether it's okay to call it if no locks are held.

It's always safe to call (in the Rust sense). I've documented that it'll either return an error or return without doing anything. It'll never explode.

If calling unlock() is only acceptable when a lock is actually held, this should probably be a Guard style of API. If it's always okay to call unlock(), the current design makes sense to me.

It's always OK (unlike a mutex or similar). It'll never explode.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[cberner]

@juntyr oh ya, that gives the best of both worlds. I didn't realize that would automatically work with ?. Thanks! I'll update the PR

[BurntSushi]

@juntyr oh ya, that gives the best of both worlds. I didn't realize that would automatically work with ?. Thanks! I'll update the PR

Please keep that separate for now. It isn't clear to me that is a good idea, and it is an impl that can be added later after we gain experience with the API.

[cberner]

Sounds good. I reverted that part of the PR

[Amanieu]

@Amanieu sounds good. I will put together a PR to make that change. One other option came to mind though. Did you discuss having the signature be Result<(), io::Error> and returning ErrorKind::WouldBlock to indicate that lock acquisition failed?

On the one hand, it's nice because then try_lock()? can be used in a function that returns io::Result, but on the other hand it's less obvious from the signature how acquisition failure is handled.

This was discussed during the meeting and there was a slightly preference towards a custom error type since that makes it very obvious if the failure was due to a lock already existing or some other I/O error. Also note that currently Windows doesn't turn ERROR_IO_PENDING into ErrorKind::WouldBlock so that would have to be added.

[det]

Adding some personal experience. I find the new try_lock API less ergonomic than the previous version.

I can no longer write:

if file.try_lock()? {
    // Do this thing
} else {
    // Do this other thing.
}

I now have to write

match file.try_lock() {
    Ok(_) => {
        // Do this thing.
    }
    Err(TryLockError::WouldBlock) => {
        // Do this other thing.
    }
    Err(TryLockError::Error(err)) => return Err(err),
}

I have to do this in several places in my code base, so it makes sense for me (and probably others) to just add a helper function that is essentially the old API.

I think the fundamental problem is that the new API treats blocking as an error when the whole point of the API is to handle blocking gracefully.

If the goal is to make the return type more clear, another option is to replace the bool with an enum TryLock { Locked, WouldBlock }, but I think a bool is perfectly clear here already.

[cberner]

@det, ya it does make that use case a bit more involved. However, the current API is now consistent with Mutex::try_lock() which seems like a good thing. And it eliminates the possibility of forgetting to check the Ok() result bool in the old API.

[cberner]

@joshtriplett the try_lock() and try_lock_shared() APIs have been updated. I just wanted to check in on the next steps for this feature. We already had a FCP, but then stabilization was reverted to make that change. Can we stabilize it again now that those have been updated, or should it go through a second FCP?

[det]

@cberner thanks, I didn't realize this was matching another API. I guess that makes sense then.

[joshtriplett]

Nominating for subsequent discussion.

[joshtriplett]

Speaking for myself:

I think the new API is a usability downgrade as well.

I think we may want to consider a dedicated two-value enum with #[must_use], which would solve this problem. People can compare to the desired value with ==, or we could add helper functions to that enum that return bool.

[dpc]

It is usability downgrade, and it was expected. But it is (supposed to be) more robust by being harder to get confused about and misuse, and harder to not notice during code reviews.

Would be nice to have some

impl<T> Result<(), TryLockError> {
  fn transpose_io(self) -> Result<Result<(), ()>, io::Error> {
    // ...
  }
}

or something like it in stdlib, and have a cake and eat it, but I'm not even sure if it's a terrible idea. Not exactly all that much work to define downstream, though then it requires extension trait.

No opinion about an enum. I guess it depends how user-proof it is in practice. Thought intuitively, IMO, not acquiring the lock should be an Err.

[joshtriplett]

@dpc In practice, one common pattern I've seen try_lock used for is:

• Attempt to acquire the lock via try_lock
• If it fails, print a message so the user knows what's going on, then block attempting to acquire the lock

Treating try_lock failure as an error makes that pattern much more annoying to write.

[joshtriplett]

@rfcbot concern api-is-unergonomic

[decathorpe]

Treating try_lock failure as an error makes that pattern much more annoying to write.

Wouldn't this also apply to the existing Mutex::try_lock? It would be really awkward if there were two try_lock APIs in std that did basically the same thing but with different error handling ...

[dpc]

@dpc I'm using this exact pattern in my code (though through fs-lock+fs4 crates). That's exactly the crate that had broken locking after it mishandled the bool, and the event that made me post here and make everyone's life harder. :D

BTW. when using this pattern, isn't it easier to just ignore the error (io or already-locked) altogether?

if let Ok(guard) = f.try_lock() {
   // quiet happy path
   return Ok(guard);
}
// locked, or broken disk, or wrong permission, who cares, just print and wait to acquire
info!("Waiting to acquire file lock);
return f.lock();

This assumes that if there's an IO issue during try_lock, it will also happen during lock.

[joshtriplett]

We talked about this at length in today's @rust-lang/libs-api meeting.

We went back and forth about several different API designs. @Amanieu also looked extensively at code search results, and found that a huge number of users of fs4 get this wrong, either by using ? and ignoring the bool, or by looking at .is_err() or .is_ok() (and thus ignoring the bool). And even the ones that use match sometimes get this wrong, by looking for a would-block error in the Err case even though that can't actually happen.

We considered the possibility of Result<WouldBlock, io::Error>, where WouldBlock (name to be bikeshed) would be a two-value enum that has #[must_use]. However, several people felt that this would be differently error-prone, because people might check .is_err() or .is_ok(), which would still ignore the WouldBlock. @Amanieu advocated for the type signature to have lock acquisition failure be in the Err case, not the Ok case, for this reason.

Ultimately, the only API we could come to consensus on was io::Result<()>, with lock acquisition failure being an IO error with a WouldBlock error kind. That will avoid all of the common failure modes observed in code searches.

There are two interesting ways people want to use try_lock.

1. Bail on any failure, including lock acquisition failure; with the new API, that would be file.try_lock()?;.

2. Handle lock acquisition failure specifically. This new API does not make that case particularly easy, though it's not any worse than the match required by the current nightly API. Or, if you're willing to treat any failure as lock acquisition failure, you can use if file.try_lock().is_err().

Given all of the above, and with the @rust-lang/libs-api team's apologies for the churn: we'd like to change the API to return io::Result<()>, with lock acquisition failure represented as an IO error with a WouldBlock error kind.

[dpc]

and found that a huge number of users of fs4 get this wrong, either by using ? and ignoring the bool,

I'm myself surprised at this discovery. I thought that people will sometimes do that, but I thought the API will be just mildly-error prone. Well, great job actually checking.

There are two interesting ways people want to use try_lock.

• Bail on any failure, including lock acquisition failure; with the new API, that would be file.try_lock()?;.

What's the rationale for this? It seems functionally it is never a good idea as the would-block and io-error (wrong permissions, etc.) have completely different consequences, and treating them as such seem to only result in higher level issues. Not a tragic mistake, but still probably always incorrect in theoretical sense.

we'd like to change the API to return io::Result<()>, with lock acquisition failure represented as an IO error with a WouldBlock error kind.

Personally, I'm OK with this. At least not acquiring the lock is always an Err, so handling it the most naive way avoids the most dreadful scenario: running code that assumes it has a lock, while actually not having it. Comparing with WouldBlock seems self-explanatory for code reviews and such. And it matches the lower level semantics, so I there's also that.

[Amanieu]

What's the rationale for this? It seems functionally it is never a good idea as the would-block and io-error (wrong permissions, etc.) have completely different consequences, and treating them as such seem to only result in higher level issues. Not a tragic mistake, but still probably always incorrect in theoretical sense.

There's quite a lot of code that treats failure to acquire a lock as a fatal error, usually CLI tools that would just report an error to the user.

[cberner]

Given all of the above, and with the @rust-lang/libs-api team's apologies for the churn: we'd like to change the API to return io::Result<()>, with lock acquisition failure represented as an IO error with a WouldBlock error kind.

No worries, thanks for taking the time to discuss it! I've submitted #140718 which changes the API to return io::Result<()>

[BurntSushi]

@Amanieu @joshtriplett Wait, what happened to the custom error type? That seems like it has all the benefits of io::Result<()> plus the benefit of being a little more explicit. And we could make the file.try_lock()? call work in a function that returns io::Result<T> with a From impl.

[dpc]

There's quite a lot of code that treats failure to acquire a lock as a fatal error, usually CLI tools that would just report an error to the user.

If they use a lock ... don't they want to wait for it to be released to acquire it? Or are these programs that do not expect to be ran in parallel and use the lock just as a safeguard mechanism? That would make a sense, I guess.