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

Avoid panic on dropping lock #34

Closed
nathaniel-daniel opened this issue Dec 8, 2022 · 3 comments · Fixed by #37
Closed

Avoid panic on dropping lock #34

nathaniel-daniel opened this issue Dec 8, 2022 · 3 comments · Fixed by #37

Comments

@nathaniel-daniel
Copy link
Contributor

nathaniel-daniel commented Dec 8, 2022

I noticed the documentation for the locks states that the drop impl may panic if there is a problem with unlocking. Looking at unix and windows, only windows seems to panic on drop due to errors while unlocking, while the unix implementation seems to ignore errors. Is it even possible for unlocks on unix to create an error?

Regardless, I think making all platforms avoid panics and ignore errors would be better, as that mirrors Rust file behavior more closely.
At the very least, I think adding a method to allow manually unlocking and returning a result on errors would be helpful so that users who wish to do so can avoid panicking if they fail to unlock a file.

If this is a desired change, I am willing to submit a PR.

If the panic on drop behavior is intended, I think that unix should also panic on drop so that all platforms behave the same.

@sunfishcode
Copy link
Collaborator

That makes sense to me. I'd like to hear from @yoshuawuyts before committing to this change though, and they're out of office until December 19th.

I don't know of any situation where flock on any Unix platform can fail on unlock, if an flock has previously succeeded on the same file descriptor.

@yoshuawuyts
Copy link
Owner

Hi, yes I'd be in favor of this change!

@nathaniel-daniel
Copy link
Contributor Author

Great thanks! I'll open a PR in the next few days when I get time. Just to be clear, I should just ensure that errors on unlocking are ignored across all platforms, and I don't need to introduce a new function to allow consuming a lock, catching errors while unlocking?

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 a pull request may close this issue.

3 participants