-
Notifications
You must be signed in to change notification settings - Fork 95
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 error message order and reconsider panics #709
Conversation
86ce4db
to
5f56f3d
Compare
} | ||
totalIn, err = safemath.SafeAdd(totalIn, manaStored) | ||
if err != nil { | ||
return 0, ierrors.Wrapf(iotago.ErrManaOverflow, "%w", err) | ||
return 0, ierrors.WithMessagef(iotago.ErrManaOverflow, "%w", err) |
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.
why not ierrors.Join()
here?
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.
Please also change that for all the following occurrences in this file.
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.
Please see the PR description for why I chose to use this style over Join
.
Description of change
Fix error message order (
ierrors.Wrap
vsierrors.WithMessage
) and reconsider panicking in appropriate situations.Implements part of iotaledger/iota-core#798.
Notes on Error Handling
We have basically three methods of error handling available in Go:
Named Errors
A named error is one that is initialized as a static variable, e.g.:
It is part of the public interface and can therefore be matched against, using, for example,
ierrors.Is
.This one should therefore be used whenever we want either users to be able to match against the error or we want to test the specific case in which the error occurs.
Anonymous Errors
Because the idiomatic error handling in Go returns the
error
interface, the returned value can be opaque. All we know is that we can callError()
on the value and get a string representation for human consumption. An anonymous error is constructed ad-hoc, e.g. withierrors.New
orierrors.Errorf
:Anonymous errors should be used for cases where we do not want to panic but we do not need the caller to be able to match against the returned error. This is the case for broken invariants that shouldn't happen, such as passing an incorrect length to
VersionFromBytes
. The error is intended to be read by a developer to figure out why their usage of the API was incorrect during development. It should not happen in a production deployment and therefore does not need to be matched against programmatically.Deciding between named and anonymous errors is never easy. Anonymous errors make it harder to test against. The only way to make sure that an incorrect length passed to
VersionFromBytes
does return an error is to assert that an error is returned or matching against the string, both of which are bad practice. On the other hand, making all errors named bloats the public interface.Panics
Panics are a lot like anonymous errors in that they do not have a public representation in the API and that they represent broken invariants. The major difference between returning an error and panicking is that the caller needs to deal with the returned error and that a panic takes away control from the caller.
Because a panics takes away control it must be used judiciously. A panic says: "this problem is serious enough that 'everything' should come to a halt."
Panics are okay to use when an essential invariant of a type or the system as a whole is broken. A concrete example is when we switch over all
Address
types (the Go types to be specific, notiotago.AddressType
) and land in thedefault
case. If we indeed handled all supported addresses then panicking in thedefault
case is ok, because we can only run into that case if we added a new address type and have not handled it above or a user passed a type implementing the address interface (which is only possible when iota.go is used as a library). In both cases we want to panic to alert us to the unhandled type or the user to the unsupported type.We must not panic in cases when users can construct input that would cause a panic, at least not when the input is passed to iota-core and then to iota.go, rather than to iota.go directly (e.g. as a library).
Because it is generally hard to verify that users can never do that for a given case, erring (no pun intended) on the side of caution and returning anonymous errors tends to be safer.
Error Wrapping
We have two ways of wrapping errors,
Wrap
andWithMessage
(and their corresponding formatting versions).The usage is very simple.
Wrap
for prepending a message:Which results in the error message:
WithMessage
for appending a message:Which results in the error message:
Combining existing errors
Sometimes two existing errors need to be combined. Using
Join
is not desirable here, as it is, in my understanding, intended to group together a collection of errors. However, in our use cases we almost always want to wrap one error in another to create a hierarchical chain of errors. One example of this is:We want to return
ErrManaOverflow
as a higher-level error for all kinds of overflows that can happen with Mana. This error is later mapped to its own transaction failure reason,TxFailureManaOverflow
. We also want to include the underlying safemath error in the error chain so we get the detailed error of which calculation went wrong:If we were to use
Join
we would not create a chain of errors but only group the errors together, which is reflected in the error message as a newline, separating the two errors:This makes it look like they are unrelated and it is only clear from context here that they are in fact related.
We might want to add a dedicated function in
ierrors
for this, perhapsierrors.WithError(iotago.ErrManaOverflow, err)
, analogous toWithMessage
.