Skip to content

Make windows filelocking exclusive #3285

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.

Sign up for GitHub

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

Merged
AkihiroSuda merged 1 commit into from
Aug 12, 2024
Merged

Make windows filelocking exclusive #3285

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions pkg/lockutil/lockutil_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,12 @@ func WithDirLock(dir string, fn func() error) error {
}
defer dirFile.Close()
// see https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx
// 1 lock immediately
Copy link
Copy Markdown
Contributor Author

@apostasie apostasie Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This was never "1 = lock immediately" 🤦‍♂️. It is "1 = The function returns immediately if it is unable to acquire the requested lock. Otherwise, it waits.". We obviously want "wait".

if err = windows.LockFileEx(windows.Handle(dirFile.Fd()), 1, 0, 1, 0, &windows.Overlapped{}); err != nil {
if err = windows.LockFileEx(windows.Handle(dirFile.Fd()), windows.LOCKFILE_EXCLUSIVE_LOCK, 0, ^uint32(0), ^uint32(0), new(windows.Overlapped)); err != nil {
Copy link
Copy Markdown
Contributor

@jsturtevant jsturtevant Aug 9, 2024

Choose a reason for hiding this comment

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

The docs say:

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. The system will signal the event specified in the [OVERLAPPED]
(https://learn.microsoft.com/en-us/windows/desktop/api/minwinbase/ns-minwinbase-overlapped) structure
 after the lock is granted.

So it might be possible that this fails if something already has the lock and the file was opened with asynchronous i/o. It doesn't appear to be the case here so we should be good with this approach, I mention it just in case something changes in the future.

return fmt.Errorf("failed to lock %q: %w", dir, err)
}

defer func() {
if err := windows.UnlockFileEx(windows.Handle(dirFile.Fd()), 0, 1, 0, &windows.Overlapped{}); err != nil {
if err := windows.UnlockFileEx(windows.Handle(dirFile.Fd()), 0, ^uint32(0), ^uint32(0), new(windows.Overlapped)); err != nil {
log.L.WithError(err).Errorf("failed to unlock %q", dir)
}
}()
Expand All @@ -50,8 +49,7 @@ func Lock(dir string) (*os.File, error) {
return nil, err
}
// see https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx
// 1 lock immediately
if err = windows.LockFileEx(windows.Handle(dirFile.Fd()), 1, 0, 1, 0, &windows.Overlapped{}); err != nil {
if err = windows.LockFileEx(windows.Handle(dirFile.Fd()), windows.LOCKFILE_EXCLUSIVE_LOCK, 0, ^uint32(0), ^uint32(0), new(windows.Overlapped)); err != nil {
return nil, fmt.Errorf("failed to lock %q: %w", dir, err)
}
return dirFile, nil
Expand All @@ -62,5 +60,5 @@ func Unlock(locked *os.File) error {
_ = locked.Close()
}()

return windows.UnlockFileEx(windows.Handle(locked.Fd()), 0, 1, 0, &windows.Overlapped{})
return windows.UnlockFileEx(windows.Handle(locked.Fd()), 0, ^uint32(0), ^uint32(0), new(windows.Overlapped))
}