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>

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 3046de5a8e634bc47ec01ed18cce8f514114ae27..4604b90ec0db58bb4e3b634eb90e9463df0852ca 100644 (file)
--- 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 a05ca6ed0d869adc29133824cd48be0fb87770e4..3d24fc127d45e315e031d2231123aa2e277eac39 100644 (file)
--- 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)
+               }
+       }
+}