Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

temporary error wrapping #41673

Closed
pierrre opened this issue Sep 28, 2020 · 2 comments
Closed

temporary error wrapping #41673

pierrre opened this issue Sep 28, 2020 · 2 comments

Comments

@pierrre
Copy link

@pierrre pierrre commented Sep 28, 2020

It seems there is a convention in Go for temporary errors:

interface {
    Temporary() bool
}

However there is no "recommended" way to know if an error is temporary (or not) if it's wrapped.

In my application, I'm using this helper function:

// IsTemporary returns true if an error is temporary, false otherwise.
// By default, an error is temporary.
func IsTemporary(err error) bool {
	var werr interface {
		Temporary() bool
	}
	ok := As(err, &werr)
	if ok {
		return werr.Temporary()
	}
	return true
}
  • It looks for the first error in the chain that implements the interface, and return the result of the method call
  • If no error implement the interface, it returns true, which means that all errors are temporary by default (which seems a good default from my point of view)

I encountered a very strange issue related to this.
In my application, I'm doing an outgoing HTTP request, which may result in a i/o timeout error.
Usually, the chain of error types is:

  • *net.timeoutError => always return true
  • net.*OpError => in this case it returns the same result as the wrapped error
  • *url.Error => returns the same result as the wrapped error

In *url.Error, the logic is implemented like this:

func (e *Error) Temporary() bool {
	t, ok := e.Err.(interface {
		Temporary() bool
	})
	return ok && t.Temporary()
}

So, if the wrapped error doesn't implement the interface, it returns false.

But in my application, I've got a custom "transport", which wraps the error between net.*OpError and *url.Error.
My custom error type implements the Unwrap() error interface, however it doesn't implement the Temporary() bool interface.
So, even if net.*OpError.Temporary() returns true, *url.Error.Temporary() will return false, because my custom error type doesn't implement Temporary() bool.

What is the correct way of managing temporary errors ?

Does my helper function IsTemporary() make sense ? Is it OK to consider that all errors are temporary by default ?
Personally I think that error types like *url.Error shouldn't implement Temporary() bool at all, and we should simply use errors.As()as I did (but I guess it's too late to change it).

What should I do in my application ?

  • not wrap the error between net.*OpError and *url.Error ?
  • implement Temporary() bool in my custom error type, in the same way as *url.Error ? (which implies that I should do the same in all custom error types across all my projects) ?
@mdlayher
Copy link
Member

@mdlayher mdlayher commented Sep 28, 2020

@mdlayher mdlayher closed this Sep 28, 2020
@pierrre
Copy link
Author

@pierrre pierrre commented Sep 29, 2020

That wasn't really a question.

However there is no "recommended" way to know if an error is temporary (or not) if it's wrapped.

url.Error.Temporary() assumes that the wrapped error implements the Temporary() bool interface. (e.g. net.OpError)
However if a custom transport/round tripper wraps the error, the value returned by url.Error.Temporary() is wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.