go: proposal: io: new Timeout interface implemented by errors

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() { /* ... */ }

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 15 (9 by maintainers)

Most upvoted comments

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.

acln0 on 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

carnott-snap on 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.

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

neild on Jun 24, 2020