-
Notifications
You must be signed in to change notification settings - Fork 19
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
fix: Calling Close fails when running in Wine #20
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this. See one comment below.
@@ -58,8 +53,11 @@ func (m *FileMutex) RUnlock() error { | |||
|
|||
// Close unlocks the lock and closes the underlying file descriptor. | |||
func (m *FileMutex) Close() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to still return the error but just add the call to windows.Close before bailing out? As in:
func (m* FileMutex) Close() error {
if err := windows.UnlockFileEx(...); err != nil && err != errLockUnlocked {
windows.Close(m.fd)
return err
}
return windows.Close(m.fd)
}
I have the sense that it's helpful to make errors accessible to the caller even in cases where they're safe to ignore because the errors can help with debugging and general awareness of what's going on. The caller could always chose to ignore errors returned from FileMutex.Close
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I totally agree with your statement that it's helpful to make errors accessible even if they are safe to ignore.
In this case however the return status of the Close() function is way more important than that of the UnlockFileEx()
function.
There are four possible cases here:
UnlockFileEx()
andClose()
both succeed, everything is fineUnlockFileEx()
fails butClose()
succeedsUnlockFileEx()
succeeds butClose()
failsUnlockFileEx()
andClose()
both fail
With your code proposal, cases 1 and 3 would work perfectly.
But case 2 would return an error although the file was successfully closed. I wouldn't know how to handle this, because there is no way to get the status of m.fd
, because it's private. Is the lock file closed or not? One just doesn't know...
Also case 4 would be problematic, because it probably would be more helpful to get the reason why Close()
failed instead of UnlockFileEx()
.
Besides, one can always explicitly unlock the lock before closing it. And the ones who are interested in proper error handling have probably designed their code in that fashion anyway.
The most accurate solution would probably be to return both error objects and/or to add more functions to view the status of the lock file handle, but this would either mean an interface change or an increase in complexity, so I think it's the most pragmatic solution to just ignore the result of UnlockFileEx()
completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, reasonable!
When running in Wine on Linux, calling Close() always fails because Wine hasn't implemented Locking yet at all and thus always returning an error when calling any locking function.
Because all locks are automatically unlocked when closing a file, it's safe to ignore any error returned by UnlockFileEx() which fixes this issue.