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

os: decide whether to keep ErrTemporary in Go 1.13 #32463

Open
rsc opened this issue Jun 6, 2019 · 16 comments

Comments

Projects
None yet
6 participants
@rsc
Copy link
Contributor

commented Jun 6, 2019

The discussion with @rogpeppe on #32405 (specifically, #32405 (comment)) drives home how weird it is to use errors.Is to check for a cross-cutting error property as opposed to checking for an error kind. It requires the introduction of a variable of type error to pass to errors.Is, but that error is not something you'd ever want to return from a function as a description of what went wrong. You'd never say just "a temporary problem happened". You'd want to explain the specific problem; it may be true that some errors are temporary, but temporary is not what the error is.

ErrTimeout was introduced alongside ErrTemporary but that one does pass the smell test to me: a function might completely reasonably return ErrTimeout to say "the problem is that the operation timed out".

I'm leaning toward removing ErrTemporary for Go 1.13. Thoughts?

(A further complication is that I remain unconvinced that "Temporary" is even well defined as a concept—I cannot actually explain precisely what it means for an error to be temporary—but my comments above apply even if we assume it is well-defined as a property. The problem is that errors aren't properties, and so errors.Is probably isn't appropriate for testing properties.)

/cc @jba @mpvl @neild

@jba

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

I always thought it was a feature of errors.Is that it could answer questions about both properties and kinds. It is intended to subsume uses of error predicate functions.

What are other options for answering "is this error temporary"? You could define an interface and use As:

type TemporaryError interface { IsTemporary() }
...
var terr TemporaryError
if errors.As(err, &terr) ...

That is more cumbersome (although it does give you access to the concrete error if you want it).

The only other option I can think of is to define an IsTemporary function, but now we have three ways of inspecting errors instead of two, and there is no easy way for new errors to mark themselves as temporary.

@bcmills

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

I would expect a check for a general property to take exactly the form in @jba's comment above, using errors.As.

I would reserve errors.Is to mean “indicates the same condition as”, but “temporary” is not a condition — it is an entire class of vaguely-related conditions.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

that error is not something you'd ever want to return from a function as a description of what went wrong.

Philosophically, now that errors can be composed, having errors that only exist to be composed with other errors doesn't seem bad to me. I don't see a difference between a sentinel meant to be wrapped by another error and something like xerrors.Opaque.

Practically, errors.Is(err, os.ErrTemporary) is a lot less verbose than

var t interface { Temporary() bool }
if errors.As(err, &t) && t.Temporary() {

and they ultimately say the same thing.

It also makes it easy to do something like return fmt.Errorf("invalid account: %w", os.ErrPermission). The alternative is:

type invalidAccount struct{}
func (invalidAccount) Error() string { return "invalid account: permission denied" }
func (invalidAccount) IsPermission() bool { return true }
var errInvalidAccount invalidAccount

(That example probably makes more sense for other kinds of sentinels-as-properties not defined in the stdlib but I just grabbed one of the ones that exists now. It's meant to be illustrative not definitive).

@bcmills

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Some more thoughts. The fact that errors.Is is not reflexive implies that it is checking for a refinement rather than an identity: “Is this shape a rectangle?”, not "Is this person bcmills?”

Even so, I have a hard time arguing that specific errors refine an abstract “temporary” error. To me, that would be like saying that “square” refines an abstract “equilateral” shape.

@neild

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

The standard library (mostly the net package) has an existing category of errors identified by those errors for which err.(interface { Temporary() bool }).Temporary() returns true. I heartily agree that this category is poorly defined; documentation on what errors are temporary is lacking, as is an indication of what it means for an error to be temporary.

If the question is whether os.ErrTemporary is well-defined, then the answer is clearly that it is not.

If the question is whether errors.Is(err, os.ErrTemporary) is a good way to test for this ill-defined class of errors, then I don't see a distinction between os.ErrTemporary and os.ErrTimeout. Either using errors.Is to test for a category of errors is reasonable or it is not. Both of these error values exist only to define such a category.

As @jimmyfrasche says, from a purely practicaly standpoint, errors.Is(err, os.ErrTimeout) is a lot less verbose than the alternative.

A small amount of library support can also make sentinels-as-properties quite easy to work with.
Sentinels-as-properties can also be quite easy to use with a very small amount of library support (https://play.golang.org/p/AIfnpVU02Fa):

err := properr.IsAlso(
  errors.New("something took too long"),
  os.ErrTemporary, os.ErrTimeout,
)
@bcmills

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Come to think of it, there actually is a significant difference between errors.Is and errors.As.

The As variant will stop searching at the first error in the chain that has a Temporary method, regardless of whether that method returns true. That allows a permanent error (such as “retry limit exceeded”, I guess?) to wrap a temporary one without preserving its temporariness.

On the other hand, Is will keep searching the chain until it finds an error for which Is returns true, even if it traverses multiple links for which Is(os.ErrTemporary) explicitly returns false.

That seems to imply that under the Is approach, every non-temporary error that may wrap a temporary one must wrap it opaquely and terminate the chain. That seems like a large burden to place on implementors.

@bcmills

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

That same criticism applies to os.ErrTimeout, of course. (I would prefer to remove that too!)

@neild

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

That seems to imply that under the Is approach, every non-temporary error that may wrap a temporary one must wrap it opaquely and terminate the chain. That seems like a large burden to place on implementors.

Wrapping an error (where "wrapping" means returning it from an Unwrap method) implies that you want to retain the properties of that error. If you don't want those properties, then you don't wrap it. Since not wrapping is never more work than wrapping, this doesn't seem burdensome.

In the very unusual case where you want to retain some, but not all, of the properties of an underlying error, this should be achievable by not returning the underlying error from Unwrap but forwarding the desired behaviors through an Is or As method. I suspect that in practice such a complex model would be a bad idea under any error (un)wrapping design.

Practically speaking, the standard library contains a number of places where error types go to some pains to forward Temporary and Timeout methods to a wrapped error. All these cases would be much simpler with the errors.Is model; just add an Unwrap method returning the underlying error and you're done.

@jba

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

I don't see a distinction between os.ErrTemporary and os.ErrTimeout. Either using errors.Is to test for a category of errors is reasonable or it is not. Both of these error values exist only to define such a category.

Russ's point is that while you could imagine returning os.ErrTimeout as an error—it says about the same thing as context.DeadlineExceeded—you would never return os.ErrTemporary.

My opinion, in case it wasn't clear from my previous comment, is that we should live with that oddness for pragmatic reasons. Every other way of testing errors for properties is worse. We already have io.EOF, which is not an error in any way except by type.

@bcmills

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Wrapping an error (where "wrapping" means returning it from an Unwrap method) implies that you want to retain the properties of that error. If you don't want those properties, then you don't wrap it. Since not wrapping is never more work than wrapping, this doesn't seem burdensome.

It's not burdensome, but it is subtle and easy to miss: retaining the properties of the wrapped error within As requires explicit forwarding on the part of the wrapper, whereas Is does not. I suspect that, in practice, authors of As methods will frequently forget (or intentionally omit) the forwarding step, and users of As will not stop to consider the difference between forwarding and deeper search.

As a thought-experiment, consider a unifying form:

// Find calls found on each successive error in the chain of err
// until either found returns true or err is nil.
// The return value from Find reports whether found returned true.
func Find(err error, found func(error) bool) bool

Is and As can both be defined in terms of that function.

Then we have three possible definitions for an IsTemporary function:

func IsTemporary(err error) bool {
	return errors.Is(err, os.errTemporary)
}
func IsTemporary(err error) bool {
	var temp interface{ Temporary() bool }
	return errors.As(err, &temp) && temp.Temporary()
}
func IsTemporary(err error) bool {
	return errors.Find(err, func(err error) bool {
		temp, ok := err.(interface{ Temporary() bool })
		return ok && temp.Temporary()
	})
}

One might naively expect these three versions, using Is, As, and Find, to produce the same results. However, that only holds if all of the following also hold:

  1. Every implementation of a Temporary() bool method includes a stanza equivalent to:
    func (e *SomeError) Temporary() bool {
    	var temp interface{ Temporary() bool }
    	if errors.As(errors.Unwrap(e), &temp) && temp.Temporary() {
    		return true
    	}
    	[…]
    }
    • Note that if SomeError embeds some other type T, adding an Unwrap method to T can cause the existing implementation of SomeErrorwhich does not know anything about errors.Unwrap — to violate this property.
  2. Every error type that provides a Temporary() bool method also implements an Is(err error) bool method that returns true if Temporary() && err == os.ErrTemporary.
    • (Note that this point does not and cannot hold: nearly every existing error type today predates Go 1.13, and thus do not provide an Is method.)
  3. Every type that implements an Is(err error) bool that may return true for os.ErrTemporary also provides a Temporary() bool method that satisfies the above constraints.
    • This is straightforward for Is methods on types that represent specific errors, but relatively subtle for generic wrappers.
@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

@bcmills

That seems to imply that under the Is approach, every non-temporary error that may wrap a temporary one must wrap it opaquely and terminate the chain. That seems like a large burden to place on implementors.

It would be far less burdensome if the Is function considered the result of an Is method definitive. Then to hide temporary-ness (or any other such sentinel property), it's just

if target == os.ErrTemporary {
  return false
}
return errors.Is(err, target)

That would place the small burden (admittedly, on a possibly larger set of people) of calling errors.Is at the end of Is methods to continue the search, if that's the desired behavior.

@neild

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@jimmyfrasche

It would be far less burdensome if the Is function considered the result of an Is method definitive.

The way to make the result of an Is function definitive is to not implement an Unwrap method.

If some, but not all, of the properties of an underlying error should be preserved, then the Is method can forward tests for those properties. I'd be cautious of this in practice, since it seems to me to point at an overly complicated error model. "This error, except..." is quite a bit more subtle than "this error, and...".

func (e *NeverGreen) Is(target error) bool {
  if target == ErrGreen {
    // This error is never green.
    return false
  }
  // Otherwise, it is the wrapped error.
  return errors.Is(e.err, target)
}
@bcmills

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

@jimmyfrasche, that's true, but there would still be an inconsistency between Is([…], os.ErrTemporary) and As([…], temp), and between As and the hypothetical Find.

The inconsistency between Is and As is at least easy to resolve, by removing os.ErrTemporary (and thus removing the temptation to rely on Is).

I'm honestly more concerned about the difference between As and the conceptual Find, especially as it relates to embedding. The suggested workaround “to not implement an Unwrap method” is a forward-looking choice, not one that we can retroactively apply to packages that used embedding without knowing about the future Unwrap method.

@neild

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

The suggested workaround “to not implement an Unwrap method” is a forward-looking choice, not one that we can retroactively apply to packages that used embedding without knowing about the future Unwrap method.

This isn't a workaround. The reason to have an Unwrap method is to retain the properties of the underlying error returned by that method. If you don't want those properties, there's no reason to have an Unwrap method.

I don't think I'm following your point about embedding at all. A concrete example would be useful.

@bcmills

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

I don't think I'm following your point about embedding at all. A concrete example would be useful.

https://play.golang.org/p/Lt3d47IU8uI

@jba

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

Come to think of it, there actually is a significant difference between errors.Is and errors.As.

The As variant will stop searching at the first error in the chain that has a Temporary method, regardless of whether that method returns true. That allows a permanent error (such as “retry limit exceeded”, I guess?) to wrap a temporary one without preserving its temporariness.

The problem here is not the difference between Is and As. Their search behavior is identical: they stop at the first error that satisfies the condition.

The problem is your definition of what it means to be a temporary error. If we want to mark properties by types instead of sentinels, we need to use marker interfaces, like the one I wrote above:

type TemporaryError interface { IsTemporary() }

There is no boolean return value. TemporaryError doesn't let you ask whether an error is temporary or not; it tells you that the error most definitely is temporary.

That resolves the asymmetry, and it also lets you recover the concrete value that is the temporary error. As can't recover that value with the boolean version of the interface.

That seems to imply that under the Is approach, every non-temporary error that may wrap a temporary one must wrap it opaquely and terminate the chain. That seems like a large burden to place on implementors.

Yes, you have to take special action to mask temporariness. But in your design, you have to take special action to propagate it, and that is the more common case. A temporary error will make its way up the call chain, being annotated with context. Only when it gets to the retry loop* will its temporariness be checked, and when that loop fails we're in a new regime: we've exhausted the programmatic actions that "temporary" implies, and it's time for something else. The author of the retry loop will have to recognize that fact when they return the new "retry failed" error. That error should not have an Unwrap method, but should record at least the last temporary error somewhere for debugging.

*I disagree with you all about the vagueness of "temporary." I think it means "this error may resolve itself in time without any action on your part," so the right response is to retry with backoff. Though admittedly there are edge cases—if you're out of quota and you won't get more for an hour, do you really want to retry?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.