Proposal: Unpack standard library error wrapping types

[spenczar]

There are several error types in the standard library that wrap errors. net/url.Error, for example, 'reports an error and the operation and URL that caused it.' This error type is used (among other places) within the net/http.Client to annotate any errors it gets from its http.RoundTripper transport.

As a result, this test (for example) would not pass:

package main

import (
	"fmt"
	"net/http"
	"testing"

	"github.com/pkg/errors"
)

// failingTransport is a http.RoundTripper which always returns an error.
type failingTransport struct {
	err error // the error to return
}

func (t failingTransport) RoundTrip(*http.Request) (*http.Response, error) {
	return nil, t.err
}

func TestClientErrorsCanBeCaused(t *testing.T) {
	rootErr := fmt.Errorf("some root cause")
	c := &http.Client{
		Transport: &failingTransport{rootErr},
	}
	_, err := c.Get("bogus")
	cause := errors.Cause(err)
	if cause != rootErr {
		t.Errorf("err cause is %q, want %q", cause, rootErr)
	}
}

-> % go test -v ./errwrap_test.go
=== RUN   TestClientErrorsCanBeCaused
--- FAIL: TestClientErrorsCanBeCaused (0.00s)
	errwrap_test.go:28: err cause is "Get bogus: some root cause", want "some root cause"
FAIL
exit status 1
FAIL	command-line-arguments	1.090s

I think that test should pass, though. Otherwise, I need to write my own series of type assertions to unpack the real root cause. The errors package could unpack the standard library types which are clearly wrappers. I think those are these:

• net.OpError
• net.DNSConfigError (deprecated)
• net/url.Error
• os.LinkError
• os.PathError
• os.SyscallError
• text/template.ExecError
• encoding/json.MarshalerError

All of these may be useful, but I think the most important are the net ones, in my experience.

The implementation seems straightforward, if you're willing to accept the smelliness of a series of special-case type assertions in the Cause function.

[davecheney]

I don't think I want to do this, for example wrapper types like OpError and URL.Error include additional information that would be discarded if the errors package interpreted them as some kind of special cause method.

Basically adding this would trigger another proposal to make the behaviour optional.

[spenczar]

Hm, I don't fully follow that line of thought. Discarding additional information seems like the point of errors.Cause. I sometimes write errors that implement the causer interface when I want to annotate an error with additional information. Calling errors.Cause on them correctly trims off all annotations to get to the root error.

At least from the side of practicality, I find myself frequently unpacking url.Errors to figure out what the real problem was.

I don't want to sound too strident here, though - I admit that it's not an obvious design decision.

[davecheney]

I'm sorry, this may be my misunderstanding. It doesn't look like I understand what you are asking.

…

On 29 Mar 2017, at 00:50, Spencer Nelson ***@***.***> wrote: Hm, I don't fully follow that line of thought. Discarding additional information seems like the point of errors.Cause. I sometimes write errors that implement the causer interface when I want to annotate an error with additional information. Calling errors.Cause on them correctly trims off all annotations to get to the root error. At least from the side of practicality, I find myself frequently unpacking url.Errors to figure out what the real problem was. I don't want to sound too strident here, though - I admit that it's not an obvious design decision. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[spenczar]

Maybe a concrete implementation will help. Here's what I propose:

func Cause(err error) error {
	type causer interface {
		Cause() error
	}

	for err != nil {
		cause, ok := err.(causer)
		if ok {
			err = cause.Cause()
			continue
		}
		switch typedErr := err.(type) {
		case *url.Error:
			err = typedErr.Err
		case *net.DNSConfigError:
			err = typedErr.Err
		case *net.OpError:
			err = typedErr.Err
		// and so on for other types, maybe?
		default:
			break
		}
	}
	return err
}

Does that help explain my thinking?

[davecheney]

Thank you. My concern with this approach is, what happens to the information in, say, url.Error

…

On Wed, Mar 29, 2017 at 8:41 AM, Spencer Nelson ***@***.***> wrote: Maybe a concrete implementation will help. Here's what I propose: func Cause(err error) error { type causer interface { Cause() error } for err != nil { cause, ok := err.(causer) if ok { err = cause.Cause() continue } switch typedErr := err.(type) { case *url.Error: err = typedErr.Err case *net.DNSConfigError: err = typedErr.Err case *net.OpError: err = typedErr.Err // and so on for other types, maybe? default: break } } return err } Does that help explain my thinking? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#112 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAcA2gNjEHxlYop4aZ00MuFBdtuFECWks5rqX6JgaJpZM4Mqb5f> .

[davecheney]

Urg, misfire. Anyway, by unwrappring through url.Error.Err it's obscuring the infomration in url.Error. I know that you want this to expose the underlying errors.Error that is inside url.Error.Err, but I hope you can understand that adding this feature would result in a request to somehow turn it off or make it optional. It is that that I am pushing back on because flags and options are a net negative to the usefulness of this package.

…

On Wed, Mar 29, 2017 at 9:29 AM, Dave Cheney ***@***.***> wrote: Thank you. My concern with this approach is, what happens to the information in, say, url.Error On Wed, Mar 29, 2017 at 8:41 AM, Spencer Nelson ***@***.***> wrote: > Maybe a concrete implementation will help. Here's what I propose: > > func Cause(err error) error { > type causer interface { > Cause() error > } > > for err != nil { > cause, ok := err.(causer) > if ok { > err = cause.Cause() > continue > } > switch typedErr := err.(type) { > case *url.Error: > err = typedErr.Err > case *net.DNSConfigError: > err = typedErr.Err > case *net.OpError: > err = typedErr.Err > // and so on for other types, maybe? > default: > break > } > } > return err > } > > Does that help explain my thinking? > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#112 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAAcA2gNjEHxlYop4aZ00MuFBdtuFECWks5rqX6JgaJpZM4Mqb5f> > . >

[spenczar]

I completely agree that flags and options should be entirely forbidden, to start. I don't want that either.

I don't think the choice is between "implement this proposal with an option to turn it off" or "don't implement this proposal." There's a third option: "implement this proposal without an option to turn it off." Yes, that would mean saying no to a future request for a flag.

Let's hash out that debate now: why would someone want this to be optional behavior? I guess they'd want to do some logical switch on the intermediate url.Error. I think the answer to that strawman would be the same one you have given elsewhere on similar requests (#84): implement your own error-unwrapping code which calls err.Cause() until it no longer can, or until it hits a url.Error.

Since you'd be asking someone to implement their own custom version, the real question for this library should be "which use case is more common?" Do you expect most people want to stop at url.Error, or would they want to dig deeper?

I can only speak for my experience, but I've generally seen people want to dig deeper to determine whether their HTTP request failed due to a DNS failure or a failure to connect or what. I think that's the common case.

[davecheney]

For example url.Error implements Temporary() bool and Timeout() bool, which people want to use to answer the question "can I retry this operation"

…

On Wed, Mar 29, 2017 at 10:15 AM, Spencer Nelson ***@***.***> wrote: I completely agree that flags and options should be entirely forbidden, to start. I don't want that either. I don't think the choice is between "implement this proposal with an option to turn it off" or "don't implement this proposal." There's a third option: "implement this proposal without an option to turn it off." Yes, that would mean saying no to a future request for a flag. Let's hash out that debate now: why would someone want this to be optional behavior? I guess they'd want to do some logical switch on the intermediate url.Error. I think the answer to that strawman would be the same one you have given elsewhere on similar requests (#84 <#84>): implement your own error-unwrapping code which calls err.Cause() until it no longer can, or until it hits a url.Error. Since you'd be asking someone to implement their own custom version, the real question for this library should be "which use case is more common?" Do you expect most people want to stop at url.Error, or would they want to dig deeper? I can only speak for my experience, but I've generally seen people want to dig deeper to determine whether their HTTP request failed due to a DNS failure or a failure to connect or what. I think that's the common case. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#112 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAcA1CD2K8Wp7Td_-yh0GVWlxRy1qYqks5rqZSUgaJpZM4Mqb5f> .

[spenczar]

url.Error just delegates Temporary() and Timeout() to the error it has wrapped.

How is this different than any other intermediate error that might implement other methods, but which also implements Cause() error?

[rjeczalik]

How about a separate stderrors subpackage with specialised Cause(error) error function which would do the unwrapping?

I faced similar problem - I needed to unwrap error coming from external package which do not implement the causer interface, so I ended up maintaining my own unwrapping logic.

These two use-cases seem similar to me, where for the stdlib's errors we can have a list of error types upfront.

[decibel]

Perhaps a better solution would be to go the other direction: have a package that wraps a supplied error the same way that the underlying package does. That would allow your test scenario to do:

func TestClientErrorsCanBeCaused(t *testing.T) {
	rootErr := error_wrapper.HttpClientErr("some root cause")
	c := &http.Client{
		Transport: &failingTransport{rootErr},
	}
	_, err := c.Get("bogus")
	cause := errors.Cause(err)
	if cause != rootErr {
		t.Errorf("err cause is %q, want %q", cause, rootErr)
	}
}

[jaekwon]

Related proposed solution: #144

[jaekwon]

@spenczar

I think a better solution is to implement #144 (no recursive wrapping in general) and to use something like errors.MaybeUnwrap(err error) error (proposed) in lieu of errors.Cause().

The ultimate cause of an error could be something too fine-grained to be helpful. A cache-miss in the CPU perhaps. Error control flow becomes brittle if an existing error decides to become a causer for whatever reason.

Proposed slogan: "Don't blame the cause, deal with it or organize it." I guess that happens to be good practice in life as well.

I think it should always be possible to unwrap an error an exact number of times to get to what you want. In the case of url.Error(), the programmer should just know to fetch url.Err, because it knows that the type is url.Error.

A function which returns an error should be responsible for returning an error that satisfies all the behavior needed to handle the range of errors that might be returned from the function. It's not possible in Golang to create a general-purpose wrapper that satisfies this requirement, unless the definition of "behavior" is relaxed to include err.HasBehavior(behaviorName) or something (but that isn't compatible with type-switching on interfaces).

[decibel]

@jaekwon

I think it should always be possible to unwrap an error an exact number of times to get to what you want.

Can't you do that by creating your own version of errors.Cause() that looks for something in particular that you're interested in? IE: I have an error interface that wraps another error with a http.Status, as well as implementing Cause(). I also have an HTTPStatus function that walks a stack of Causers, stopping at the first error that implements HTTPStatus().

I just now created func CauseWalker(err error, walkFn func(error) bool) that walks a stack of Causers, calling walkFn for each one until it hits the end of the stack or a walkFn returns false. Perhaps having that as part of pkg/errors would be helpful.