net/http: timeout error no longer returned from Transport.RoundTrip

[aronatkins]

Please answer these questions before submitting your issue. Thanks!

1. What version of Go are you using (go version)?

go version go1.7rc2 linux/amd64

1. What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH=""
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build273188391=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

1. What did you do?
   Create an HTTP transport that supports timeouts; both when dialing and when reading from the established connection.
   https://play.golang.org/p/LxGp2ZAI54

The example passes when run in the playground. You need to run this code with 1.7rc2 to see the issue.

1. What did you expect to see?
   With Go 1.6.3, the test program is successful.

$ go run transport_bug.go
2016/07/21 22:02:14 Success.

1. What did you see instead?
   With Go1.7rc2, the test program fails because a timeout error is no longer returned.

$ go run transport_bug.go
2016/07/22 02:02:48 expected a net.Error; got &errors.errorString{s:"http: server closed connection"}
exit status 1

This is no longer a net.Error and also an error that does not support a Timeout function.

[bradfitz]

Thanks for an excellent stand-alone test case.

FWIW, this behavior changed in 4d2ac54 which was a follow-up to 5dd372b.

A patch like,

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 9164d0d..e715a03 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -1383,7 +1383,9 @@ func (pc *persistConn) readLoop() {
                if err == nil {
                        resp, err = pc.readResponse(rc, trace)
                } else {
-                       err = errServerClosedConn
+                       if err == io.EOF {
+                               err = errServerClosedConn
+                       }
                        closeErr = err
                }

... fixes your test case, but breaks this:

$ go test -v -run=TestRetryIdempotentRequestsOnError -count=5 net/http
=== RUN   TestRetryIdempotentRequestsOnError
--- FAIL: TestRetryIdempotentRequestsOnError (0.00s)
        transport_test.go:2566: Get http://127.0.0.1:50091: read tcp 127.0.0.1:38416->127.0.0.1:50091: read: connection reset by peer
=== RUN   TestRetryIdempotentRequestsOnError
--- PASS: TestRetryIdempotentRequestsOnError (0.00s)
        transport_test.go:2573: finished after 0 runs
=== RUN   TestRetryIdempotentRequestsOnError
--- FAIL: TestRetryIdempotentRequestsOnError (0.01s)
        transport_test.go:2566: Get http://127.0.0.1:35428: read tcp 127.0.0.1:56660->127.0.0.1:35428: read: connection reset by peer
=== RUN   TestRetryIdempotentRequestsOnError
--- PASS: TestRetryIdempotentRequestsOnError (0.00s)
        transport_test.go:2573: finished after 0 runs
=== RUN   TestRetryIdempotentRequestsOnError
--- FAIL: TestRetryIdempotentRequestsOnError (0.00s)
        transport_test.go:2566: Get http://127.0.0.1:34613: read tcp 127.0.0.1:55003->127.0.0.1:34613: read: connection reset by peer
FAIL
exit status 1
FAIL    net/http        0.025s

(Note how it becomes flaky)

What's happening is that the HTTP transport (client code) is interpreting any read error from the server while the client is idle as the HTTP server just getting bored of us and disconnecting. The errors can take various forms: io.EOF, read: connection reset by peer, etc.

I suppose we could write a function to detect more of them, but they also vary by OS (Windows vs Unix, especially) and there's nothing in the net package to help us distinguish.

Maybe there's something safe we can do for Go 1.7. I'll think.

@ianlancetaylor, any thoughts?

[aronatkins]

It's probably too special-cased, but what about testing if err is a net.Error that is Timeout (maybe Temporary as well). Not clear to me if the original timeout error is making it this far, though.

[aronatkins]

Along those lines, I had been thinking that a net.IsTimeout(err error) would be nice. Tests both that an error implements Timeout() and that it returns true.

[aronatkins]

I should also note: In the real code, I do not care about the type of error that is generated. I care more that the request is aborted should the server not respond. I wrote a test (used to derive the test case in this issue) so I could prove to myself that a timeout was happening. I am OK adjusting how I perform that validation.

[bradfitz]

It's probably too special-cased, but what about testing if err is a net.Error that is Timeout (maybe Temporary as well).

That's approximately what I was thinking.

Not clear to me if the original timeout error is making it this far, though.

It is.

[ianlancetaylor]

It's kind of weird but for ECONNRESET calling Temporary() will return true (that is so because of #6163). This will be passed up to the net package's Temporary() method. But that is really too ugly.

Perhaps we should add a new Closed() method to the net package errors. That might also help with #4373, maybe.

[bradfitz]

Looking at this more, but one observation:

Neither https://golang.org/pkg/net/http/#RoundTripper nor https://golang.org/pkg/net/http/#Transport.RoundTrip make any documentation promises about the type of errors returned, and we obviously have never added unit tests locking in a certain behavior for the past 7 releases. Also, I think this code has changed a fair bit for each of those 7 releases, so I wouldn't be surprised if the exactly handling of this type of error has shifted over time. (though your test does seem to pass with Go 1.5 and Go 1.4 too)

[bradfitz]

I've mailed out https://golang.org/cl/25153 which I'm happy with, but it might be too late for Go 1.7. I'll let @ianlancetaylor and @adg chime in. I'm slightly in favor of fixing it for Go 1.7 considering it would mean keeping the behavior from Go 1.4, Go 1.5, and Go 1.6.

[gopherbot]

CL https://golang.org/cl/25153 mentions this issue.

[adg]

@bradfitz happy for this to go in for 1.7.