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

net: deprecate Temporary error status #45729

Closed
neild opened this issue Apr 23, 2021 · 10 comments
Closed

net: deprecate Temporary error status #45729

neild opened this issue Apr 23, 2021 · 10 comments

Comments

@neild
Copy link
Contributor

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
Contributor

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 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.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 11, 2021
@rsc rsc moved this from Incoming to Active in Proposals (old) May 12, 2021
@rsc
Copy link
Contributor

rsc commented May 12, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented May 19, 2021

It sounds like deprecate means "leave things alone, comment that it's not to be used, and stop discussing it".
That sounds great!

@gopherbot
Copy link

Change https://golang.org/cl/322889 mentions this issue: net: deprecate Temporary error status

@rsc
Copy link
Contributor

rsc commented May 26, 2021

Does anyone object to doing this?

@rsc
Copy link
Contributor

rsc commented Jun 2, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Jun 2, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Jun 9, 2021
@rsc
Copy link
Contributor

rsc commented Jun 9, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: net: deprecate Temporary error status net: deprecate Temporary error status Jun 9, 2021
@rsc rsc modified the milestones: Proposal, Backlog Jun 9, 2021
@gopherbot
Copy link

Change https://golang.org/cl/340261 mentions this issue: net: deprecate (net.Error).Temporary

@cespare
Copy link
Contributor

cespare commented Apr 21, 2022

It's not clear to me what the replacement is for Temporary inside server Accept loops (like net/http.Server's). Since this is closed, I asked about it on golang-nuts.

problame added a commit to zrepl/zrepl that referenced this issue Oct 24, 2022
Go 1.18 deprecated net.Error.Temporary().
This commit cleans up places where we use it incorrectly.
Also, the rpc layer defines some errors that implement

  interface { Temporary() bool }

I added comments to all of the implementations to indicate
whether they will be required if net.Error.Temporary is ever
ever removed in the future.

For HandshakeError, the Temporary() return value is actually
important. I moved & rewrote a (previously misplaced) comment
there.

The ReadStreamError changes were
1. necessary to pacify newer staticcheck and
2. technically, an error can implement Temporary()
   without being net.Err. This applies to some syscall
   errors in the standard library.

Reading list for those interested:
- golang/go#45729
- https://groups.google.com/g/golang-nuts/c/-JcZzOkyqYI
- https://man7.org/linux/man-pages/man2/accept.2.html

Note: This change was prompted by staticheck:

> SA1019: neterr.Temporary has been deprecated since Go 1.18 because it
> shouldn't be used: Temporary errors are not well-defined. Most
> "temporary" errors are timeouts, and the few exceptions are surprising.
> Do not use this method. (staticcheck)
@golang golang locked and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

5 participants