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: io: new Timeout interface implemented by errors #39798

Open
carnott-snap opened this issue Jun 24, 2020 · 14 comments
Open

proposal: io: new Timeout interface implemented by errors #39798

carnott-snap opened this issue Jun 24, 2020 · 14 comments
Labels
Projects
Milestone

Comments

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Jun 24, 2020

background

Currently there is a private interface in net, errors, and os that is used to check for if an error is a timeout:

type timeout interface {
        Timeout() bool
}

While these interfaces can only used inside the packages they are defined in, the specification leaks into many public apis for other packages: net/http, syscall, context, etc.

implementation

We should expose one common interface:

package io

type Timeout interface {
        Timeout() bool
}

alternatives

It would likely be better to expose io.ErrTimeout and ensure everybody implements errors.Is(err, io.ErrTimeout), but that would be a breaking change, so it is not advocated for here. That being said, I am in favour of that too, but for compatibility, it would need to be both. I also recall their being a larger discussion, that I cannot find, about the topic of ErrTemporary, ErrTimeout, and the like.

If we wanted to reserve io.Timeout for something better, we could call this io.Timeouter, but it sounds strange. io may not be the right package either, but os woudl make for a circular dependency and errors did did not seem, strictly speaking, better.

drawbacks

This interface is not strictly speaking required, but it is a compile time contract between library and consumer, and does make the following code read better:

var t interface{ Timeout() bool }
if errors.As(err, t); t.Timeout() { /* ... */ }

// versus

var t io.Timeout
if errors.As(err, t); t.Timeout() { /* ... */ }
@gopherbot gopherbot added this to the Proposal milestone Jun 24, 2020
@gopherbot gopherbot added the Proposal label Jun 24, 2020
@acln0
Copy link
Contributor

@acln0 acln0 commented Jun 24, 2020

ErrTimeout is a tricky one: see #33411 and the things it links to for reference.

As for a Timeout interface in package io, I'm not sure. It seems to me like timeouts are not strictly related to I/O.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 24, 2020

As for a Timeout interface in package io, I'm not sure. It seems to me like timeouts are not strictly related to I/O.

Since almost every usage I could find related to error handling, errors.Timeout works for me and does not introduce a dependency cycle. Upon second review, it looks like os.Timeout does not introduce a cycle for io, but like os.Error may not be a good idea.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 24, 2020

As you say, we generally encourage errors.Is these days. We could add an os.ErrTImeout that supports errors.Is(err, os.ErrTimeout). Note that this can be implemented using an Is method on errors that currently have a Timeout method that returns true, so it should be entirely backward compatible.

Something like

package os

var ErrTimeout timeoutError

type timeoutError struct{}
func (timeoutError) Error() string { return "timeout" }
func (timeoutError) Timeout() bool { return true }

func (e *PathError) Is(err error) bool {
    if err == ErrTimeout {
        return e.Timeout()
    }
    return false
}
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jun 24, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jun 24, 2020

/cc @neild @mpvl @dsnet

We intend for errors.Is to be the way to check these kinds of things moving forward.
We left the Timeout method out because there were some semantic issues that we still haven't resolved.
I don't have the prior discussions at hand but I think they are in the issue tracker.

@rsc rsc moved this from Incoming to Active in Proposals Jun 24, 2020
@rsc rsc changed the title proposal: new io.Timeout interface proposal: io: new Timeout interface implemented by errors Jun 24, 2020
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 24, 2020

To be clear xxx.Timeout would work with errors.As, but I like the confirmation that errors.Is is preferred.

That being said, the proliferation of interface{ Timeout() bool } speaks to some degree of community confusion: os.PathError.Timeout, net.OpError.Timeout, etc.. Especially if this interface is officially deprecated, I think it would be useful to add both symbols and mark xxx.Timeout as deprecated to inform consumers of this fact. The standard library could deprecate all the methods instead, but this would miss third party usage.

Since there seems to be consensus on ErrTimeout I am happy to include that in the proposal, or spawn a new issue, let me know what is preferred, but I would also like to get a decision on xxx.Timeout.

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 24, 2020

Note that this can be implemented using an Is method on errors that currently have a Timeout method that returns true, so it should be entirely backward compatible.

That can be done for errors in the standard library, but we cannot easily update user-defined timeout errors to do the same. (See #33411 (comment).)

@neild
Copy link
Contributor

@neild neild commented Jun 24, 2020

We'd obviously never have used the interface { Timeout() bool } approach of marking timeouts if errors.Is had existed back in the day. If we want to support a common classification for timeout errors, then errors.Is(err, ErrTimeout) seems like the correct approach to me.

#33411 argues against having a common classification for timeouts.

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

So I think the real question is: Should we be providing a way of identifying timeouts in general, or not?

Obviously, we are providing that way right now in the form of interface { Timeout() bool }, but perhaps it was a mistake to do so. If it was a mistake, then we should update documentation to discourage using that method and we should avoid adding new implementations of it in the future.

On the other hand, if a common classification for timeouts is something we want, then adding os.ErrTimeout back seems like a sensible way to provide it.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 24, 2020

So I think the real question is: Should we be providing a way of identifying timeouts in general, or not?

I am relatively agnostic on the issue, so will let the more passionate among us decide.

Obviously, we are providing that way right now in the form of interface { Timeout() bool }, but perhaps it was a mistake to do so. If it was a mistake, then we should update documentation to discourage using that method and we should avoid adding new implementations of it in the future.

Where would you want to update documentation? Even catching all standard library implementations will miss third party libraries. Do you think it is worth adding an immediately deprecated symbol to serve this purpose?

@neild
Copy link
Contributor

@neild neild commented Jun 24, 2020

Where would you want to update documentation? Even catching all standard library implementations will miss third party libraries. Do you think it is worth adding an immediately deprecated symbol to serve this purpose?

If we decide that grouping together timeouts from different sources was a bad idea (which I don't have a strong opinion on at the moment), then we should probably remove documentation that mentions the Timeout method and add deprecation notices to net.Error and any exported instances of Timeout.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 24, 2020

Now that @neild mentions it, I agree: we shouldn't be pushing the Timeout method or the hypothetical os.ErrTimeout at all. It's been historically painful, and we should just let it drop.

I sent the documentation-only https://golang.org/cl/239705 to help with that process.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 24, 2020

I missed this before, but there is an extant testing/iotest.ErrTimeout. Seems limited in scope, but it did not appear to be part of any of these discussions.

If this is the direction we are pushing in, it strikes me that three classes of usage should be amended:

methods

Public symbols should be deprecated to clarify usage to consumers:

  • net.AddrError.Timeout
  • net.DNSConfigError.Timeout
  • net.DNSError.Timeout
  • net.InvalidAddrError.Timeout
  • net.OpError.Timeout
  • net.UnknownNetworkError.Timeout
  • net/url.Error.Timeout
  • os.IsTimeout
  • os.PathError.Timeout
  • os.SyscallError.Timeout
  • syscall.Errno.Timeout (unix, windows, js)
  • syscall.ErrorString.Timeout (plan9)

Probably good to deprecate internal symbols too:

  • context.deadlineExceededError.Timeout
  • crypto/tls.timeoutError.Timeout
  • internal/poll.TimeoutError.Timeout
  • net.addrinfoErrno.Timeout
  • net.addrinfoErrno.Timeout
  • net.addrionfo.Errno.Timeout
  • net/http.http2httpError.Timeout
  • net/http.httpError.Timeout
  • net/http.tlsHandshakeTimeoutError.Timeout
  • net.timeoutError.Timeout
godoc

As already started by @ianlancetaylor, we should purge all mention from documentation, leaving the behaviour as vestigial. His CL 239705 covers:

  • net.PacketConn.ReadFrom
  • net.PacketConn.WriteTo
  • net.Conn.Read
  • net.Conn.Write

But not:

  • os.File.SetDeadline
  • crypto/tls.Conn.Read
  • net/http.Client.Do
  • net/http.Client.Get
  • net/http.Get
callsite

While potentially overkill, it may be good to add a clarifying warning comment around current usage. It would serve to freeze the current implementation, while dissuading readers from copying the pattern.

  • context/context_test.go:650
  • context/net_test.go:18
  • context/net_test.go:19
  • crypto/tls/handshake_client_test.go:1980
  • crypto/tls/handshake_client_test.go:494
  • crypto/tls/handshake_server_test.go:1591
  • crypto/tls/tls_test.go:206
  • net/dnsclient_unix.go:263
  • net/error_test.go:654
  • net/http/client_test.go:1274
  • net/http/client_test.go:1319
  • net/http/client_test.go:1985
  • net/http/client_test.go:1986
  • net/http/server.go:1755
  • net/http/server.go:705
  • net/http/transport_test.go:2275
  • net/http/transport_test.go:3152
  • net/lookup_test.go:559
  • net/lookup_test.go:565
  • net/mockserver_test.go:297
  • net/mockserver_test.go:323
  • net/mockserver_test.go:515
  • net/net.go:502
  • net/net.go:505
  • net/protoconn_test.go:225
  • net/protoconn_test.go:232
  • net/protoconn_test.go:44
  • net/protoconn_test.go:51
  • net/rawconn_test.go:133
  • net/rawconn_test.go:142
  • net/rawconn_test.go:156
  • net/rawconn_test.go:170
  • net/timeout_test.go:197
  • net/timeout_test.go:253
  • net/timeout_test.go:347
  • net/timeout_test.go:414
  • net/timeout_test.go:471
  • net/timeout_test.go:479
  • net/timeout_test.go:525
  • net/timeout_test.go:596
  • net/timeout_test.go:644
  • net/timeout_test.go:688
  • net/timeout_test.go:721
  • net/timeout_test.go:763
  • net/timeout_test.go:88
  • net/timeout_test.go:884
  • net/udpsock_test.go:394
  • net/udpsock_test.go:438
  • net/unixsock_test.go:116
  • net/unixsock_test.go:166
  • net/url/url.go:35
  • net/url/url_test.go:1622
  • net/url/url_test.go:1623
  • os/error.go:107
  • os/error.go:53
  • os/error.go:69
  • syscall/syscall_js.go:78
  • syscall/syscall_plan9.go:72
  • syscall/syscall_unix.go:141
  • syscall/syscall_windows.go:158
  • vendor/golang.org/x/net/nettest/conntest.go:400
  • vendor/golang.org/x/net/nettest/conntest.go:401
@rsc
Copy link
Contributor

@rsc rsc commented Jul 15, 2020

The discussion above has died out, but it sounds like the general consensus was that while there may be some cleanup to do, we should probably not bother trying to define a "generic timeout", nor a predicate for checking for one.

That said, it seems a bit too aggressive to deprecate the Timeout method on existing errors, but that's not what this proposal was suggesting.

Based on the discussion, the proposal as currently titled - "io: new Timeout interface implemented by errors" - seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals Jul 16, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jul 22, 2020

No change in consensus, so declined. We still need to figure out timeouts, but a new Timeout interface isn't the way.

@rsc rsc moved this from Likely Decline to Declined in Proposals Jul 22, 2020
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jul 22, 2020

So, to confirm you are declining both the new io.Timeout symbol, as well as deprecation of existing Timeout() bool methods?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
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.