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
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Milestone

Comments

@FiloSottile
Copy link
Contributor

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

@FiloSottile FiloSottile added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Aug 1, 2019
@FiloSottile FiloSottile added this to the Go1.13 milestone Aug 1, 2019
@networkimprov
Copy link

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

/cc @ianlancetaylor

@neild
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

neild commented Aug 1, 2019

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

@bcmills
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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

@neild
Copy link
Contributor

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants