Create some form of MultiError type

[kisielk]

Note: the scope of this issue has tightened, see #15 (comment)

This is an issue for discussion of a potential MutliError (or other name...) type API that allows the accumulation of multiple errors in a collection which still implements the error interface but allows retrieval of the individual errors.

There are a few other options out there: https://godoc.org/?q=multierror but since github.com/pkg/errors already implements, in my opinion, the canonical best practices for error handling in Go it would be be nice if it included this feature.

[natefinch]

While this is definitely a constant problem in my experience... I'm not sure canonicalizing it here is super necessary.

Here's my example: https://play.golang.org/p/B9_v2E4Rrv
The only part that requires any logic is how you concatenate the error strings, which seems like the kind of bike-sheddy thing no one's going to agree on, so everyone will just write their own.

What would be nice is to canonicalize the interface for getting back the list of underlying errors. This is actually true of all the common error handling interfaces for Dave's "check by behavior" - IsTemporary() bool etc - if they are dictated by the stdlib, a la io.Reader, then everyone will use them and we'll all be singing Happy Days. If they're not, we'll have a hodgepodge and interoperability will still be a problem.

[davecheney]

Thanks for raising this issue. I think there are several things going on here. The first is an error which represents a set of errors, aka, something like

var errs []error
for _, x := range things {
        err := x.Do()
        if err != nil {
              errs = append(errs, err)
        }
}
return errs

So some kind of errors.Wrap(errs...), thing. If this is what you're talking about then I'd like to take a pass. I think multierror is too complicated. Ie, if an error does not occur, then the number of errors returned will not match the number of operations attempted. So, does multierror track errors along with operations, or does it track only errors and you have to figure out how to tie them back to the operation that failed. This ambiguity makes me want to not touch this.

With that said, perhaps there is cause for another interface, like Cause() error, possibly Causes() []error for an error which is caused by multiple events. I dunno, It doesn't seem to solve the question above.

The other kind of multierror could be something like this

rc := SomeReadCloser()
_, err := rc.Read(...)
if err != nil {
     err2 := rc.Close()
     if err2 != nil {
           // what do we do here ?
     }
     return err
}
...

In this example, which we hit a few times in Juju the operation failed, then the cleanup operation failed -- now we have two errors that are related, call them a sequence, rather than a set (above), which one do return?

If we choose err then possibly we're ignoring the more serious, or more detailed error, returned from close. If we choose err2 then the caller sees that the "close" operation failed, but not the actual "read" which was the first failure.

At the moment i've seen this handled in a number of ways, none very good. The first one is to return the first error and log the second barf. The other is to return err.Error()+":"+err2.Error() or something.

This is a problem I think the errors package should try to solve, I'm not sure if that would solve the other use case at the same time.

[ChrisHines]

I have found a collection of errors useful when doing input validation where the goal is to report all problems with the data rather than just the first problem. I am not sure that this use case is related closely enough to this package to make it a good fit though.

[davecheney]

@ChrisHines exploring this idea a bit further, say we have

var errs []error = ... // from somewhere !?
err := errors.Wrap(errs...)

What does

errors.Cause(err) 

return ?

We could make some kind of MultiError structure, so the underlying cause is something like

type Multierror []error

But that doens't seem like something that needs to be added to the errors package.

[seh]

It's never popular to cite Java for prior art, but Java 7 added to class Throwable the getSuppressed() method. The motivation for this method and the broader concept of suppressed exceptions was to accommodate these failed cleanup attempts that occur in the finally block of a try-with-resources statement.

Translating your example above, in Java you'd get err thrown from the method, with err2 exposed as the only element in the array of err's suppressed exceptions.

[ChrisHines]

I agree that this use case is orthogonal to the Wrap/Cause feature of this package. I guess it depends what you want the scope of pkg/errors to be. The package docs imply a general purpose set of error helpers.

Package errors implements functions for manipulating errors.

So although type Multierror []error is orthogonal to Wrap/Cause, maybe it belongs in pkg/errors if it adds another useful way to manipulate errors. Or perhaps pkg/errors should stay one dimensional (often a good thing) and the package docs should say something like "Package errors implements functions for creating and inspecting contextual errors."

[davecheney]

@seh this is an interesting point. I think there are two ideas here, but first some background.

err := errors.Wrap(err, "oops")

We say that the error on the right is the 'cause', and the error on the left is a wrapper, or it wraps the cause. It gets a little more complicated if we have

err := errors.Wrap(errors.Wrap(errors.Wrap(err, "oh), "damn"), "blast)

So, the error in the middle is the cause, and the one on the left is a wrapper, so I guess that means all the others are wrappers, even if they are retrieved by the Cause behaviour.

So with that background, none of these wrappers are considered to be the cause of the error, they simply wrap the error, exploiting the error interface contract to add additional information.

What you're talking about is an error that is the cause, but also notes some other cause as related. So in the example I gave, and to make up a function name

if err := nil {
       err2 := thing.Close()
       if err2 := nil {
           err = errors.CausedBy(err2, err) 
      }
      return err
}

This has a few problems. Firstly the name. Second, the arguments, they'll both be the same type so getting them in the wrong order is almost guaranteed; an api with a 50% chance getting it wrong is a bad idea. But I don't have any better ideas at the moment.

[davecheney]

@ChrisHines i'm fine with a PR to change the comment on the package declaration.

I'm not sold that type Multierror []error is something that should be added until the result of passing it to errors.Cause is defined.

[seh]

Java's Throwable has a complementary method addSuppressed(), for which the documentation is admirably thorough. This part is pertinent:

In the try-with-resources statement, when there are two such exceptions, the exception originating from the try block is propagated and the exception from the finally block is added to the list of exceptions suppressed by the exception from the try block. As an exception unwinds the stack, it can accumulate multiple suppressed exceptions.

An exception may have suppressed exceptions while also being caused by another exception. Whether or not an exception has a cause is semantically known at the time of its creation, unlike whether or not an exception will suppress other exceptions which is typically only determined after an exception is thrown.

Note that programmer written code is also able to take advantage of calling this method in situations where there are multiple sibling exceptions and only one can be propagated.

There they acknowledge a difference between accumulating siblings and causing those siblings to arise. The finally block would get executed whether the try block throws or not; that is, we're going to close the file whether or not we ran into an error reading from it once it was open. Hence, the read failure doesn't cause the failure to close the file. The read failure merely "dominates" the closing failure.

I see the quandary you face with parameter order; Java skirts that problem by dispatching via method on the accumulating exception instance. I'm not trying to sell you on accommodating this capability. I just want to point to other attempts at handling the same problem.

[ChrisHines]

i'm fine with a PR to change the comment on the package declaration.

I would wait and see how the package evolves toward version 1.0. I was simply trying to gain some insight into the scope you have in mind for the package.

I'm not sold that type Multierror []error is something that should be added until the result of passing it to errors.Cause is defined.

Should Multierror implement errors.causer? My guess is probably not, in which case passing it to errors.Cause returns the Multierror itself.

However, my quick survey of the various multierrr packages indexed by godoc.org indicates that a type Multierror []error is probably just a collection of errors that implements the error interface with some opinion about formatting the aggregate error messages.

I have to agree with you, I don't see the value in adding it to pkg/errors yet.

[natefinch]

Seems like there's two different types of multierrors - parallel and serial. Parallel errors occur when you have one entrypoint that does N independent things, each of which can independently error or not, such as "read these 3 files". Serial errors occur when one entrypoint has N errors related to different parts of the same logical operation, such as Dave's Read and Close errors.

Parallel errors really should just be a slice of errors - it's just one function call that returned N unrelated errors. In that case, it's probably correct to correlate errors to input 1:1, which means some of the errors in the slice may be nil. This is probably the less interesting of the two types... you should just be returning []error and not a single error as the signature of the function with this kind of failure is possible.

Serial errors are more interesting. They likely have one primary error, and some associated errors. In this case, I think the main error may well have a cause (which is not any of the associated errors). This type of multi error would then also have a slice of associated errors, each of which may also have a cause.

I'm not sure if the serial type of multierror should exist in this package or not.

[bits01]

@natefinch Re your first comment about a common interface to get the list of underlying errors, there's a Wrapper interface in the Hashicorp errwrap package:
https://github.com/hashicorp/errwrap/blob/master/errwrap.go#L48
While MultiError may not belong in this errors pkg, I think it's worth looking at the following for some ideas (though I don't agree with everything that happens in there either -- in the end it seems that everyone will cut & paste from multiple projects and make their own):
https://github.com/hashicorp/go-multierror
https://github.com/hashicorp/errwrap

[davecheney]

@bits01 thanks for your reply. To make the scope of this issue clear, I do not want to add

 type MultiError []error

To this package -- it's not complicated enough to warrant someone having to depend on this package -- remember that any type which has an underlying type of []error can be converted to an []error so there is no need for everyone to agree on the name or location of that declaration.

wrt. the remaining bits in scope, @natefinch 's so named Serial error, I think this is something that the errors package should handle, assuming a non footgun API can be found for it.

[erikh]

over at https://github.com/contiv/errored we use this notion of .Combined

errored.Errorf("Error message").Combined(err)

(No, I am not here to promote my project, genuinely interested!)