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

proposal: net: deprecate Temporary error status #45729

Open
neild opened this issue Apr 23, 2021 · 2 comments
Open

proposal: net: deprecate Temporary error status #45729

neild opened this issue Apr 23, 2021 · 2 comments
Labels
Milestone

Comments

@neild
Copy link
Contributor

@neild neild commented Apr 23, 2021

The net.Error interface defines Timeout and Temporary methods:

type Error interface {
	error
	Timeout() bool   // Is the error a timeout?
	Temporary() bool // Is the error temporary?
}

The meaning of "timeout" is reasonably intuitive: Was an error returned because an operation ran past a deadline?

However, the meaning of "temporary" is not obvious. What makes an error temporary vs. permanent? Is EHOSTUNREACH temporary (because it may result from a temporary network routing error)? Is an operation that ran past a deadline permanent (because retrying the operation without extending the deadline will still fail)?

There is more discussion of net.Temporary in here, and in the following thread:
#32463 (comment)

Looking at existing places where the standard library returns an error which implements a Temporary() bool method returning true (not counting ones where the temporary status is propagated from a more-specific error):

Timeouts:

  • context: context.DeadlineExceeded
  • crypto/tls: All Dial timeouts.
  • net: Various timeouts.
  • net/http: Timeout when reading headers or bodies. (The error type is named httpError, but it is only used for timeouts.)
  • net/http: Also, HTTP/2 timeout reading response headers.
  • os: os.ErrDeadlineExceeded (defined in internal/poll)

Non-timeouts:

Ignoring cases where "temporary" is redundant with "timeout", the only "temporary" errors I can find are ones resulting from a small number of syscall errors. In most cases, "temporary" appears to mean "a timeout, or out of file descriptors". The documentation for net.Error does not make the limited scope of "temporary" errors obvious.

There are a number of other places in the standard library where errors propagate through the "temporary" status of another error. The existence of these Temporary() methods makes it seem (to me at least) as if temporary errors are more prevalent than they actually are.

Perhaps there is a useful definition of a "temporary" error. Perhaps we should provide a well-defined os.ErrTemporary or similar. That is not this proposal.

I believe that:

  • We currently do not have a good definition of "temporary".
  • Figuring out what errors implement net.Error and indicate a "temporary" status is difficult.
  • The cases where Temporary does not imply Timeout are surprising and not particularly useful.

I propose that we deprecate the Temporary method:

type Error interface {
	error
	Timeout() bool   // Is the error a timeout?

	// Deprecated: Temporary errors are not well-defined.
	// Most temporary errors are timeouts, and the few exceptions are surprising.
	// Do not use this method.
	Temporary() bool
}
@bcmills
Copy link
Member

@bcmills bcmills commented Apr 28, 2021

IMO the Temporary implementation for context.DeadlineExceeded is downright incorrect, and probably os.ErrDeadlineExceeded as well.

A “temporary” error ought to be one that could be resolved by retrying the same call with exactly the same arguments. However, if a Context has already passed its deadline and the call fails with DeadlineExceeded, retrying the same call with the same Context is almost certain to fail in the same way.

@neild
Copy link
Contributor Author

@neild neild commented Apr 28, 2021

I agree, but this isn't specific to context.DeadlineExceeded: A non-retriable timeout has implied Temporary since the first CL introducing the interface. (https://go.googlesource.com/go/+/47a05334117) The definition of temporary has been confused from the start.

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

Successfully merging a pull request may close this issue.

None yet
3 participants