cockroachdb / errors

Go error library with error portability over the network
Apache License 2.0
2.07k stars 66 forks source link

CombineErrors equivalent such that all errors are exposed via `Is` and friends #62

Closed ajwerner closed 3 years ago

ajwerner commented 3 years ago

Today we have uses cases in validation where it is very valuable to discover all of the validation errors. Returning early upon the first error greatly diminishes the value of the functionality. However, when more than zero validation errors occur, in some cases, we'd like to synthesize a single error. Today this is done using CombineErrors. This seemed amazing! However, there is a serious flaw with this approach; the validation process may issue RPCs or call into other subsystems which may return errors which are meaningful up the stack. In the case of cockroachdb, I'm thinking about roachpb errors like TransactionRetryWithProtoRefresh specifically. The problem is that if we subsequently encounter some other structural error, then the higher level may not notice that a restart needs to happen. For what it's worth, that may all end up being more or less okay in this case as anything other than a retry error is going to cause the transaction to be aborted.

On the whole, it's not at all clear that it's okay any future RPCs to be issued once we hit that error.

Forgetting that fact, which I should deal with, it still seems like there should be a way to compose errors such that the full set is searched when unwrapping.

ajwerner commented 3 years ago

cc @postamar

ajwerner commented 3 years ago

I do see this quote in the RFC:

The goal of keeping "other" errors is to facilitate troubleshooting by humans, by avoiding the loss of potentially-useful details. It is not meant to enable further in-code processing.

I understand that searching the graph may be expensive. And I guess I understand that the semantics of a non-linear cause chain is complicated. Curious to hear more about the rationale here.

knz commented 3 years ago

The rationale is what was written in the RFC. The Go error abstraction only provisions linked lists for automated error structure analysis, so that's the only thing we can expose in a way that's compatible across our Go package dependencies.

For your use case, you can't use errors.CombineErrors and you need something new instead, which is able to peek into its argument and choose which error to use a main cause depending on its "strength"

I.e. we want the following semantics:

That's a new object entirely.

FWIW I also once wanted an error object that would present all the causes side-by-side (instead of a chain) and I implemented this: https://github.com/knz/shakespeare/blob/master/pkg/cmd/errors.go

It's not exactly what you need, but it would make the implementation of what you need simpler perhaps.