Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std.fs: support Lock.none on Windows #21306

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

DivergentClouds
Copy link

@DivergentClouds DivergentClouds commented Sep 4, 2024

Fixes #18430. Borrows code from #18566.

I also made std.windows.LockFile and std.windows.UnlockFile use the relevant explicit error set.

The doc comments and Windows API for File.lock File.tryLock have been changed to match the POSIX implimentation.

@DivergentClouds DivergentClouds changed the title std.fs support Lock.none on Windows std.fs: support Lock.none on Windows Sep 4, 2024
@squeek502
Copy link
Collaborator

Looks good, thanks for getting this fixed!

@DivergentClouds

This comment was marked as outdated.

@DivergentClouds
Copy link
Author

I apologize for the early requested review btw, I don't know how that happened

…ors when `windows.UnlockFile` returns `error.Unexpected`
@alexrp alexrp mentioned this pull request Oct 15, 2024
@alexrp alexrp added this to the 0.14.0 milestone Jan 30, 2025
@andrewrk andrewrk removed this from the 0.14.0 milestone Mar 1, 2025
Comment on lines -1761 to +1783
error.Unexpected => unreachable, // Resource deallocation must succeed.
error.Unexpected => return error.Unexpected,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this change also be made in unlock()?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unlock can't return an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it seems questionable to me that we're doing it here. If we're worried that the OS might become able to return an error here in the future, the same concern surely applies to unlock()? Are we just crossing our fingers and hoping that such an error will be safe to ignore in unlock()?

Copy link
Collaborator

@squeek502 squeek502 Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we just crossing our fingers and hoping that such an error will be safe to ignore in unlock()?

I think so; same sort of thing with File.close. I agree that it seems strange for unlock, not sure that should really count as "resource deallocation."

FWIW, there's exactly one instance of File.unlock in a defer that I could find:

const locked = if (key_log_file.lock(.exclusive)) |_| true else |_| false;
defer if (locked) key_log_file.unlock();

@alexrp
Copy link
Member

alexrp commented Mar 26, 2025

I guess this isn't strictly related to this PR, and I might just be showing my ignorance, but I don't understand why file.lock(.none) is even a thing. Why not just file.unlock()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compilation error when attempting to lock a file (File.lock / File.tryLock
4 participants