diff options
author | Brad Fitzpatrick <bradfitz@golang.org> | 2016-08-16 05:05:00 +0000 |
---|---|---|
committer | Brad Fitzpatrick <bradfitz@golang.org> | 2016-08-16 06:20:12 +0000 |
commit | 6fd2d2cf161ec933f206ef57b8ca6062815545d3 (patch) | |
tree | e1df85505b5bda2f910adc378a692c2b63b9e2ef | |
parent | fe27291c0039d4de0748ebd512cb236ca3c24ff6 (diff) | |
download | go-6fd2d2cf161ec933f206ef57b8ca6062815545d3.tar.gz go-6fd2d2cf161ec933f206ef57b8ca6062815545d3.zip |
net/http: make Transport retry non-idempotent requests if no bytes written
If the server failed on us before we even tried to write any bytes,
it's safe to retry the request on a new connection, regardless of the
HTTP method/idempotence.
Fixes #15723
Change-Id: I25360f82aac530d12d2b3eef02c43ced86e62906
Reviewed-on: https://go-review.googlesource.com/27117
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
-rw-r--r-- | src/net/http/transport.go | 14 | ||||
-rw-r--r-- | src/net/http/transport_internal_test.go | 67 |
2 files changed, 72 insertions, 9 deletions
diff --git a/src/net/http/transport.go b/src/net/http/transport.go index 3046de5a8e..4604b90ec0 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -421,19 +421,15 @@ func (pc *persistConn) shouldRetryRequest(req *Request, err error) bool { // our request (as opposed to sending an error). return false } + if _, ok := err.(nothingWrittenError); ok { + // We never wrote anything, so it's safe to retry. + return true + } if !req.isReplayable() { // Don't retry non-idempotent requests. - - // TODO: swap the nothingWrittenError and isReplayable checks, - // putting the "if nothingWrittenError => return true" case - // first, per golang.org/issue/15723 return false } - switch err.(type) { - case nothingWrittenError: - // We never wrote anything, so it's safe to retry. - return true - case transportReadFromServerError: + if _, ok := err.(transportReadFromServerError); ok { // We got some non-EOF net.Conn.Read failure reading // the 1st response byte from the server. return true diff --git a/src/net/http/transport_internal_test.go b/src/net/http/transport_internal_test.go index a05ca6ed0d..3d24fc127d 100644 --- a/src/net/http/transport_internal_test.go +++ b/src/net/http/transport_internal_test.go @@ -72,3 +72,70 @@ func newLocalListener(t *testing.T) net.Listener { } return ln } + +func dummyRequest(method string) *Request { + req, err := NewRequest(method, "http://fake.tld/", nil) + if err != nil { + panic(err) + } + return req +} + +func TestTransportShouldRetryRequest(t *testing.T) { + tests := []struct { + pc *persistConn + req *Request + + err error + want bool + }{ + 0: { + pc: &persistConn{reused: false}, + req: dummyRequest("POST"), + err: nothingWrittenError{}, + want: false, + }, + 1: { + pc: &persistConn{reused: true}, + req: dummyRequest("POST"), + err: nothingWrittenError{}, + want: true, + }, + 2: { + pc: &persistConn{reused: true}, + req: dummyRequest("POST"), + err: http2ErrNoCachedConn, + want: true, + }, + 3: { + pc: &persistConn{reused: true}, + req: dummyRequest("POST"), + err: errMissingHost, + want: false, + }, + 4: { + pc: &persistConn{reused: true}, + req: dummyRequest("POST"), + err: transportReadFromServerError{}, + want: false, + }, + 5: { + pc: &persistConn{reused: true}, + req: dummyRequest("GET"), + err: transportReadFromServerError{}, + want: true, + }, + 6: { + pc: &persistConn{reused: true}, + req: dummyRequest("GET"), + err: errServerClosedIdle, + want: true, + }, + } + for i, tt := range tests { + got := tt.pc.shouldRetryRequest(tt.req, tt.err) + if got != tt.want { + t.Errorf("%d. shouldRetryRequest = %v; want %v", i, got, tt.want) + } + } +} |