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

blockstore: use errors when API contracts are broken #195

Merged
merged 1 commit into from
Aug 2, 2021
Merged

blockstore: use errors when API contracts are broken #195

merged 1 commit into from
Aug 2, 2021

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Aug 2, 2021

(see commit message)

@mvdan mvdan requested review from masih, dirkmc and raulk August 2, 2021 11:25
@mvdan
Copy link
Contributor Author

mvdan commented Aug 2, 2021

Err, I forgot to run the tests. Edit: fixed.

It's technically correct to use panics in these situations,
as the downstream user is clearly breaking the contract documented in
the API's godoc.

However, all these methods return errors already,
so panicking is not our only option here.

There's another reason that makes a panic unfortunate.
ReadWrite.Finalize enables the panic behavior on other methods,
which makes it much easier to run into in production
even when the code and its tests work normally.

Finally, there's some precedent for IO interfaces using errors rather
than panics when they are used after closing.
For example, os.File.Read returns an error if the file is closed.

Note that this change doesn't make panics entirely impossible.
The blockstore could still run into exceptional and unexpected panics,
such as those caused by memory corruption or internal bugs.
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.

3 participants