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: remove ErrTimeout in Go 1.13 #33411

Closed
FiloSottile opened this issue Aug 1, 2019 · 11 comments
Closed

os: remove ErrTimeout in Go 1.13 #33411

FiloSottile opened this issue Aug 1, 2019 · 11 comments

Comments

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Aug 1, 2019

Extracting #32463 (comment) and #32463 (comment) from #32463, which was closed by removing ErrTemporary.

We've seen net.Error.Timeout() become nearly useless because it got overloaded with too many different (and requiring different handling) timeout errors. Did a DNS request timeout, requiring a retry of the Dial? Did the keep-alive hit, breaking the connection? Did the deadline hit, meaning the Read can be retried after resetting it? Did a timeout happen that caused a fatal error that can't be retried? (More at #31449.)

I don't think there would be a semantic meaning to os.ErrTimeout either, except being something that's called a timeout. Adding a single ErrTimeout to the standard library would encourage lack of specificity, without having any upside, because errors.Is(err, ErrTimeout) doesn't mean anything. Libraries and applications are better off using errors.New("this specific action timed out") or defining their own timeout errors.

Instead, we should maybe have net.DeadlineExceeded, which can always be handled by resetting a deadline.

/cc @rsc @bcmills

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Aug 1, 2019

The current plan for net.Error in 1.13, which touches syscall.Errno and os.SyscallError is here: #31449 (comment)

/cc @ianlancetaylor

@neild

This comment has been minimized.

Copy link
Contributor

@neild neild commented Aug 1, 2019

Unwrapping aside, errors.Is(err, os.ErrTimeout) is equivalent to os.IsTimeout(err).

It seems clearly necessary for the os package to have a mechanism for testing errors it returns for deadline-exceeded conditions.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Aug 1, 2019

@neild, this is part of why I argued in #32463 (comment) that Find is actually the more primitive error-classification function.

if errors.Find(err, os.ErrTimeout) {

reads a lot more naturally to me than

if errors.Is(err, os.ErrTimeout) {

The existence of the internal/oserror package is particularly telling: it couples together the os and syscall packages in a way that inverts their usual dependency structure, and in a way that end users generally cannot mimic in their own code. In particular, as far as I can tell end users cannot modify the implementation of (*syscall.Errno).Is to make the errors equivalent to their own arbitrary errors.

I do not understand why it is appropriate for the os package to violate this inherent package layering, particularly in a way that user-defined packages cannot.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Aug 1, 2019

To be clear, the resolution I would prefer for this issue is:

  • Add to the errors package a Find function with one of the following signatures:
    func Find(err error, found func(error) bool) bool
    func Find(err error, found func(error) bool) (err error, ok bool)
  • Remove os.ErrTimeout and the internal/oserror package.
  • In the os.Is* predicate functions, continue to consult an Is method if provided on the error (to support user-defined extensions to the os predicates).

That would restore the os predicates — not errors.Is-equivalence — as the canonical way to check errors returned by the os package for those predicates, and leave the Timeout() bool method as the canonical way to indicate a user-defined “timeout” error.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 1, 2019

@bcmills It seems to me that it is clearly too late in the 1.13 release cycle to add errors.Find. And it seems to me that if we decide to add it in 1.14, we can then use it as you suggest.

For 1.13 we need to decide on ErrTimeout, and we need to decide on whether to keep the comment in the os package saying "Errors returned from this package may be tested against these errors with errors.Is."

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Aug 1, 2019

Yes, you're probably right about that. But users can always write the equivalent loop themselves.

So in the interim, I think it would be better to remove ErrTimeout, internal/oserror, and the promise of detectability via errors.Is, and try again next cycle on the ergonomics of the error checks.

@neild

This comment has been minimized.

Copy link
Contributor

@neild neild commented Aug 1, 2019

FWIW, a function like errors.Find was proposed in #30093 (and eventually rejected).

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Aug 2, 2019

If I understand the implemented behavior of os.IsTimeout correctly, the problem is worse than I feared. For user types that provide even a naive implementation of a Timeout method, os.IsTimeout(err) will return true, but errors.Is(err, os.ErrTimeout) will return false (see example below; code here).

It is my understanding that, as implemented today, all other error/predicate combinations in the os package have the property that the set of errors for which errors.Is returns true is a strict superset of the set for which the corresponding predicate returns true. The inconsistency for ErrTimeout seems like reason enough to pull it back for further consideration.

issue33411$ gotip version
go version devel +8a317ebc Thu Aug 1 02:15:18 2019 +0000 linux/amd64

issue33411$ cat main.go
package main

import (
        "errors"
        "fmt"
        "os"
)

type timeoutError struct{}

func (timeoutError) Error() string { return "operation timed out" }
func (timeoutError) Timeout() bool { return true }

func main() {
        err := timeoutError{}
        fmt.Printf("os.IsTimeout(err) = %v\n", os.IsTimeout(err))
        fmt.Printf("errors.Is(err, ErrTimeout) = %v\n", errors.Is(err, os.ErrTimeout))
}

issue33411$ gotip run main.go
os.IsTimeout(err) = true
errors.Is(err, ErrTimeout) = false
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Aug 2, 2019

I am OK with looking at removing ErrTimeout until we understand better whether to strengthen net.Conn's behavior for deadlines (see #31449). Until things settle down, people who want to do IsTimeout with unwrapping can use

var ne net.Error
if errors.As(err, &ne) && ne.Timeout() {
    ...
}

@neild, do you have time to put together a CL with the implications of removing ErrTimeout?

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 2, 2019

Change https://golang.org/cl/188758 mentions this issue: all: remove os.ErrTimeout

@neild

This comment has been minimized.

Copy link
Contributor

@neild neild commented Aug 2, 2019

@rsc https://golang.org/cl/188758 removes ErrTimeout. This is mostly a partial rollback of https://golang.org/cl/170037.

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
7 participants
You can’t perform that action at this time.