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

fix: Calling Close fails when running in Wine #20

Merged
merged 1 commit into from Jan 11, 2024

Conversation

usaran-devk
Copy link
Contributor

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.

Copy link
Owner

@alexflint alexflint left a 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 {
Copy link
Owner

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

Copy link
Contributor Author

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:

  1. UnlockFileEx() and Close() both succeed, everything is fine
  2. UnlockFileEx() fails but Close() succeeds
  3. UnlockFileEx() succeeds but Close() fails
  4. UnlockFileEx() and Close() 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.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, reasonable!

@alexflint alexflint merged commit 6a69275 into alexflint:master Jan 11, 2024
12 checks passed
@usaran-devk usaran-devk deleted the fix/windows_close branch January 31, 2024 07:13
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.

None yet

2 participants