-
Notifications
You must be signed in to change notification settings - Fork 699
Create some form of MultiError type #15
Comments
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 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" - |
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
So some kind of With that said, perhaps there is cause for another interface, like The other kind of multierror could be something like this
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 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. |
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. |
@ChrisHines exploring this idea a bit further, say we have
What does
return ? We could make some kind of MultiError structure, so the underlying cause is something like
But that doens't seem like something that needs to be added to the |
It's never popular to cite Java for prior art, but Java 7 added to class Translating your example above, in Java you'd get |
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.
So although |
@seh this is an interesting point. I think there are two ideas here, but first some background.
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
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 So with that background, none of these wrappers are considered to be the cause of the error, they simply wrap the error, exploiting the 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
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. |
@ChrisHines i'm fine with a PR to change the comment on the package declaration. I'm not sold that |
Java's
There they acknowledge a difference between accumulating siblings and causing those siblings to arise. The 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. |
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.
Should However, my quick survey of the various multierrr packages indexed by godoc.org indicates that a I have to agree with you, I don't see the value in adding it to pkg/errors yet. |
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. |
@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: |
@bits01 thanks for your reply. To make the scope of this issue clear, I do not want to add
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 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. |
over at https://github.com/contiv/errored we use this notion of
(No, I am not here to promote my project, genuinely interested!) |
@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
or your fluent suggestion of
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, 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. |
Sure. So, I can give you some concrete examples of how we use this;
perhaps it will help you.
We use it in this project: https://github.com/contiv/volplugin. Here are
a couple of chunks of code that use it.
https://github.com/contiv/volplugin/blob/master/volmaster/daemon.go#L428
https://github.com/contiv/volplugin/blob/master/volmaster/daemon.go#L622
Particularly, we wrap any error that is not an `errored` error with
`.Combine`. This way we get the basic functionality (stack traces and
some additional debug logging notably).
We also wrap `errored` errors occasionally to extend/augment the
debugging data, but this is less common, e.g. propagating up from the
storage driver(s).
We aren't really doing typed errors with errored though -- we still use
`errors` for that most of the time. Typed errors aren't very elegant for
the kind of diagnostics we're attempting to get out of the system.
Let me know if you want more, I'm sure I can find something.
…-Erik
|
@erikh maybe we're talking about different things, this line from your sample
Would be written as
You're not combining two errors values, just wrapping an existing error with more context. |
That's correct. We haven't integrated it but this is our solution for Sorry for the lack of clarity, I'm a little foggy. I'll follow up to any On 24 May 2016, at 1:14, Dave Cheney wrote:
|
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. |
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:
This is nice because:
But this is different in every case (sometimes they're keyed on a |
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. |
All, please read the note at the top of the issue about scope, #15 (comment) |
Specially, []error is out of scope. The remaining points are #15 (comment) |
And thanks for your comments, please keep them coming. |
@erikh ping |
Ah! Sorry I didn't get back to you. So, 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. |
@erikh thanks for your reply. I don't really grok fully what |
yes. it is. Very sorry if I wasted too much of your time. |
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. |
If this error package implemented hashicorp's This would allow their multierror to play nice with this package, which could also be The nice thing about the |
Breaking change from package errorv. API has been simplified down to three methods (New, Wrap, With). There could be problems with the API going fluent (as described by Dave Cheney: see pkg/errors#15 (comment)), but the simplicity of the API makes me want to try this out more in practice to see how often it causes problems. Changed the package name from errorv to errors. It seems a bit pretentious to re-use the standard library name (github.com/pkg/errors aims to replace the standard library, so that explains its name). Given that the standard errors package only exports one function and that this package is a drop-in replacement, I'll try it out. It might get a name change again later if using "errors" causes any problems.
Would an The only issue I see with this is how |
@alanraison Sorry, but no. The reason for rejecting this idea is the same as declining |
@davecheney how about a simple func (f *fundamental) AddSuppressed(err error) *fundamental {
f.suppressed = append(f.suppressed, err)
return f
} Pro:
Con:
|
What happens to suppressed errors? How are they recovered? Are they recoverable?
… On 3 Dec 2017, at 01:22, max-rsYkRh ***@***.***> wrote:
@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("...")
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
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 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 |
@davecheney how about |
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”. |
@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. |
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. |
I know I'm speaking out of turn here, but I think a more constructive
approach instead of complaining about it, TBH, is posting your alternatives
that supply the type or functionality so Dave can carry on as he sees fit,
yet give alternatives for those that require/absolutely want the feature.
I hope Dave agrees. :)
…On Sun, Jan 21, 2018 at 3:29 AM, Nikolay Turpitko ***@***.***> wrote:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABJ63ZhS5ptMhH5c5a7C9rD1Nz17aV5ks5tMx-FgaJpZM4IVHrO>
.
|
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. |
Speaking in general, to nobody directly. Plus, I would love to evaluate
what the community thinks would solve this problem.
Sorry if I implied otherwise.
…On Sun, Jan 21, 2018 at 7:29 AM, Nikolay Turpitko ***@***.***> wrote:
@erikh <https://github.com/erikh>
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABJ61nQMU9c7mGb0lndiyPzixcCz31Tks5tM1e8gaJpZM4IVHrO>
.
|
I’m closing this issue as answered, see #15 (comment) |
@davecheney i'm sorry to disturb you, (and i read #15 (comment), it looks pretty correct, that just 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 |
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.
The text was updated successfully, but these errors were encountered: