Change File::try_lock() and try_lock_shared() to return io::Result<()> #140718
Conversation
rustbot
added
S-waiting-on-review
library/std/src/sys/fs/windows.rs
Outdated
| @@ -444,9 +443,9 @@ impl File { | |||
| if err.raw_os_error() == Some(c::ERROR_IO_PENDING as i32) | |||
| || err.raw_os_error() == Some(c::ERROR_LOCK_VIOLATION as i32) => | |||
There was a problem hiding this comment.
@ChrisDenton Would it make sense to have these directly convert to ErrorKind::WouldBlock?
There was a problem hiding this comment.
For ERROR_LOCK_VIOLATION that would make sense.
For ERROR_IO_PENDING it would be very wrong. And in fact looking at the code here I'm pretty sure it's currently unsound, assuming ERROR_IO_PENDING were ever returned (which it shouldn't be with LOCKFILE_FAIL_IMMEDIATELY). The function must not return while there is a pending I/O operation. It must wait for it to complete because we've passed in a reference to the stack.
There was a problem hiding this comment.
@ChrisDenton ah, thanks for flagging that. I have very little experience with the Windows platform, so I appreciate the help!
The LockFileEx documentation says:
The LockFileEx function operates asynchronously if the file handle was opened for asynchronous I/O, unless the LOCKFILE_FAIL_IMMEDIATELY flag is specified. If an exclusive lock is requested for a range of a file that already has a shared or exclusive lock, the function returns the error ERROR_IO_PENDING.
and I interpreted that to mean that when LOCKFILE_FAIL_IMMEDIATELY is specified, then waiting on the event is unnecessary. But am I misreading that, and we need the same event waiting code that's in acquire_lock() in the try_lock()/try_lock_shared() functions?
There was a problem hiding this comment.
Yes, with LOCKFILE_FAIL_IMMEDIATELY waiting should not be necessary so checking for ERROR_IO_PENDING is also unnecessary. The code can just check for ERROR_LOCK_VIOLATION .
There was a problem hiding this comment.
Ah, got it. Thanks! Yes, I added a test for this case and it seems to pass just fine with ERROR_IO_PENDING removed.
And I added a separate commit that changes decode_error_kind() to map ERROR_LOCK_VIOLATION to ErrorKind::WouldBlock along with that test case. I assume that's the right place to do the conversion that you and @Amanieu were talking about?
|
This looks good to me, pending the question about Windows. |
This changes decode_error_kind() on Windows to map ERROR_LOCK_VIOLATION to ErrorKind::WouldBlock instead of ErrorKind::Uncategorized
This PR changes the signatures of
File::try_lockandFile::try_lock_sharedas requested by @rust-lang/libs-api in this discussion: #130994 (comment)These methods are unstable under the "file_lock" feature. The related tracking issue is #130994