Comments (45)
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.
from errors.
For things like form validation or other situations where lots of errors can occur, you always need to know what failed and what didn't, and what 'things' the errors relate to.
In the past, I've used a map:
func ValidateForm(data map[string]interface{}) map[string]error {
// validate each field and set errors keyed on the fields
}
This is nice because:
- you can check the length of the map to see if any errors occurred (or even use
nil
still) - range over it getting keys and errors, and
- use native types (a
map
) without introducing anything new to learn.
errs := DoSomething(data)
if errs != nil {
// something went wrong
}
// or
if len(errs) > 0 {
for field, err := range errs {
log.Println("field", field, "invalid:", err)
}
}
But this is different in every case (sometimes they're keyed on a string
and sometimes an int
) so in my view each codebase should solve it themselves in an appropriate way, and I'd keep it out of this package.
from errors.
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.
from errors.
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.
from errors.
@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.
from errors.
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.
from errors.
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."
from errors.
@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.
from errors.
@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.
from errors.
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.
from errors.
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.
from errors.
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.
from errors.
@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
from errors.
@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.
from errors.
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!)
from errors.
@erikh thanks for your comment.
I've been thinking about this problem a lot over the last few weeks, and you're right that it really boils down to the API chosen for handling this. eg, it's either going to be
errors.CausedBy(err1, err2)
or your fluent suggestion of
errors.New("something happened).And(errors.New("this happened"))
To dispense with my former suggestion, I cannot bring myself to introduce an API that will have such a high rate of misuse -- which is more important, err1 or err2 ? Can you always remember the rule ? Even when this is the unlikely failure path? We actually have a function like this in Juju's errors package, and it's used very infrequently.
So, to your suggestion. I'm not opposed to the fluent or builder pattern in principal, but it would mean that to make it work, New
, Errorf
, would have to return some concrete builder type. For the sake of argument that might be *errors.ErrorBuilder
(or *errors.Error
, but the former is clearer what is happening).
The problem I see with this approach is this pattern is the following code would not compile
err := errors.New("oops")
err = err.Add(errors.New("something else"))
The reason this does not compile is obvious, but will be a stone in the shoe of people who hit it. It would also mean that this code
err := errors.New("oops")
// something happens
err = errors.Wrap(err, "some more context")
Would also not compile. I see the latter as a common use case and that concerns me. It's not that either suggestions are bad or wrong, but they have some concerning usability issues.
So I think it might be time to step back and look at where I see this kind of function being used. Here is an example piece of code from Juju. It doesn't appear often, but always appears in this form
if _, err := st.Model(); err != nil {
if err := st.Close(); err != nil {
logger.Errorf("error closing state for unreadable model %s: %v", tag.Id(), err)
}
return nil, errors.Annotatef(err, "cannot read model %s", tag.Id())
}
In other words, an error happened using a resource, then cleaning up the resource, a further error occurred.
Thinking about this some more I wonder if there is a logical error to assume anything about the state of st once the error occurred. That is, if st.Model failed, then one line of thinking would suggest that any other method on st will fail, including Close. In which case, what is the point of recording an error that is already known. The other line of thinking suggests that depending on how st is written, each operation is independent and a failure to call one method does not invalidate the object as a whole.
In summary, this pattern isn't as widespread as I first thought, and under investigation I'm not sure there is a one size fits all approach that works. @erikh i'd be interested in hearing your experiences as I am leaning towards closing without changing the errors package.
from errors.
from errors.
@erikh maybe we're talking about different things, this line from your sample
return errored.Errorf("Clearing volume records for %q", vc).Combine(err)
Would be written as
return errors.Wrapf(err, "Clearing volume records for %q", vc)
You're not combining two errors values, just wrapping an existing error with more context.
from errors.
That's correct. We haven't integrated it but this is our solution for
multi-errors:
https://github.com/contiv/errored/blob/master/errored.go#L78
Sorry for the lack of clarity, I'm a little foggy. I'll follow up to any
replies tomorrow.
On 24 May 2016, at 1:14, Dave Cheney wrote:
@erikh maybe we're talking about different things, this line from your
samplereturn errored.Errorf("Clearing volume records for %q",
vc).Combine(err)
Would be written as
return errors.Wrapf(err, "Clearing volume records for %q", vc)
You're not combining two errors values, just wrapping an existing
error with more context.
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#15 (comment)
from errors.
For what it's worth, our own error library has had a way to combine errors and it's been mostly helpful. https://godoc.org/github.com/spacemonkeygo/errors#ErrorGroup
The main downside is that, yeah, it makes it hard to programatically do anything with specific internal errors, but the times we've used it for a 90kloc project are in the high double digits, and the times we've needed to do anything with the errors are maaaaybe 1 or 2.
from errors.
Another potential use case: start multiple goroutines to work on some task in parallel and then wait for them to finish. Some goroutines may return errors. It would be useful to take all the errors and package them all into a single error so it can be reported back as a single error while not losing details on each individual error.
from errors.
All, please read the note at the top of the issue about scope, #15 (comment)
from errors.
Specially, []error is out of scope. The remaining points are #15 (comment)
from errors.
And thanks for your comments, please keep them coming.
from errors.
Apologies - I was answering the MultiError case. @bits01 in that case, an error channel (<-chan error
) has worked well for me in the past. Tweet me @matryer if you'd like me to explain more.
from errors.
@erikh ping
from errors.
Ah! Sorry I didn't get back to you. So, Combine()
, Contains()
both fill and access a []error
and [][]string
(stack traces). The struct is here: https://github.com/contiv/errored/blob/master/errored.go#L64-L74. So they are essentially "multi-errors" that evaluate to a lengthy combined error. There is also a pure errors
generated error it keeps as well, for better or worse.
I'm not sure if that was the answer you were looking for or not. FWIW, I just peeked briefly at #34 and I will have to take a look at applying what zap is doing to errored too, we need this to improve our UX. There's a lot of matching functionality there that could be implemented.
from errors.
@erikh thanks for your reply. I don't really grok fully what errored.Error
is doing, it looks like a stack of errors, by some parts of the api look like a slice of errors. If it's the former then AFAICT we're doing the same thing, #15 (comment). If it's the latter, a slice of errors that represent a set of errors that all occurred during a single operation, then that's out of scope for this issue.
from errors.
yes. it is. Very sorry if I wasted too much of your time.
from errors.
Not at all, it's not a waste of time to see how others have approached this problem. This package is very opinionated, it's never going to be possible to make it adapt to every requirement.
from errors.
If this error package implemented hashicorp's WrappedErrors() []error
style interface, the two could work nicely together, providing different styles of error wrapping, while still being able to represent the notion of wrapping consistently.
This would allow their multierror to play nice with this package, which could also be fit accumulated into those multi errors. The default wrapped error provided by hashicorp's errwrap package is much more simplistic than this package's approach to wrapping.
The nice thing about the errwrap
package is that it isn't really one at all, it's just a convenience function justifying an interface (edit: i should note, I think errwrap.Contains
is a bit evil, but errwrap.Walk
is fits in to the interface driven approach of handling errors nicely in my experience)
from errors.
Would an errors.AndThen(err1, err2, "message")
pattern work? This could return a new, 2-component error; first
and second
which could be printed out, similarly to a wrapped error but with the first's error stack followed by "and subsequently..." followed by the second stack.
The only issue I see with this is how Cause()
would work.
from errors.
@alanraison Sorry, but no. The reason for rejecting this idea is the same as declining errors.Wrap(err1, err2)
-- there is no guideline as to which is the dominant error. This gives an API with a 50% chance of misuse.
from errors.
@davecheney how about a simple AddSuppressed
(or whatever you would like to name it) for the fundamental
type?
func (f *fundamental) AddSuppressed(err error) *fundamental {
f.suppressed = append(f.suppressed, err)
return f
}
Pro:
- Backwards compatible
- Impossible to mistake parameter order
Con:
- This method can only be used with errors created by
err := errors.New("...")
from errors.
from errors.
No less, one would have to interface assert the error to an AddSuppresseder in order to even access the method.
The answer here is not to expand the error interface or error library. If you are possibly going to return multiple errors, you should be returning a <-chan error
, and for err := range errch
it.
Best part of this chan error technique is that you never have to return a nil error, if you just close the channel, it will then start sending nil errors to all readers. So even if someone just uses return <-errch
rather than ranging over it, it will return nil in a no error situation, and return only the first error put on the channel otherwise.
from errors.
@davecheney how about errors.Wrap(outer, inner)
? Prior art hashicorp/errwrap. This is also similar to the pattern used by append().
from errors.
how about errors.Wrap(outer, inner)
Principle concern: this would require a change to the public API, which is unlikely to happen. So, it would need to be named something other than “Wrap”.
from errors.
@puellanivis thanks for your comment, as I’ve said earlier and in other issues I do not plan to add a function where the two parameters are of the same type, it has a 50% chance of being used incorrectly.
from errors.
I understand, it's already out of scope, don't reiterate it, it's just FYI: https://cloud.google.com/appengine/docs/standard/go/reference#MultiError.
from errors.
from errors.
I'm not complaining, not asking for reply, not asking to add new interface to the package or to take any other actions. But I'm following the discussion and interested in it's outcome. I just thought, that, probably, GAE's API is useful detail for the project maintainers to consider. In the real projects we have to combine several 3rd party libraries, with different approaches and different implementations of the pattern. It'd be useful to have some common denominator.
In the discussion above Dave talked about complexity of tracking which operation correspond to which error. Link I posted, illustrates one possible approach.
In my own projects I have to deal with MultiErrors, returned from AppEngine's API, with http responses to bulk requests (when single request contains batch of items and single response contains batch of results / errors) from several 3rd party APIs, I have no luxury to ignore the fact that quite often API can return some combined set of errors and I'd like to deal with them universally. I'm not happy with my current solution, it's not universal and elegant. I combine several approaches - I'm using pkg/errors to wrap error and hashicorp/go-multierror to pass errors to upper levels, sometimes I had to use map approach similar to what matryer mentioned (but in my case I map items of http request to results from response). Also I have helper functions to unwrap wrapped multierrors of wrapped errors... It's quite nasty, actually. So I'm here in search of better solution, looking for a wisdom of more experienced and smart people. Please don't stop discussion.
from errors.
from errors.
I’m closing this issue as answered, see #15 (comment)
from errors.
@davecheney i'm sorry to disturb you, (and i read #15 (comment), it looks pretty correct, that just []error
looks too small to be included in the package.
However, I find solution from hashicorp quite useful: it has convenient formatting, error checking, converting to normalized error, etc. etc. etc. What do you think about examining multierror
package and trying to add something to this library? I'm ready to spend my time to suggest several PRs, because I'm absolutely sure: github.com/pkg/errors
is the second most important package for error handling in the gophers community after stdlib errors
, and the lack of a wrapper for multiple errors makes my life very difficult literally every day.
from errors.
Related Issues (20)
- Proposal: add errors.Is and errors.As
- RFC: Allow custom StackTraces
- func Is() does not works with errors.Wrap or errors.Wrapf HOT 3
- Cause() behavior has changed HOT 11
- respect -trimpath build flag HOT 3
- Unwrap doesn't return the base error HOT 7
- Questions about extending annotations HOT 5
- undefined errors.Wrap HOT 2
- How to change format return wrapped error message HOT 2
- Stacktrace don't print all stack HOT 6
- errors.Wrap() created bugs in our codebase. HOT 3
- StackTrace.Format with "%+v" includes a leading newline HOT 1
- A new error caused by another error HOT 7
- Demo
- Change Wrap() to return an error even if we provide nil HOT 3
- return *stack instead of stack HOT 2
- Wrap() duplicates call stack HOT 6
- Elegant way to check error is wrapped HOT 4
- `Errorf` should be able to wrap errors HOT 4
- Package errors looking for new maintainers HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from errors.