net/http: fix "http2: no cached connection..." error with x/net/http2

The net/http Transport was testing for a sentinel x/net/http2 error
value with ==, which meant it was only testing the bundled version. If
a user enabled http2 via golang.org/x/net/http2, the error value had a
different name.

This also updates the bundled x/net/http2 to git rev ab555f36 for:

    http2: add internal function isNoCachedConnError to test for ErrNoCachedConn
    https://golang.org/cl/87297

Fixes #22091

Change-Id: I3fb85e2b7ba7d145dd66767e1795a56de633958c
Reviewed-on: https://go-review.googlesource.com/87298
Reviewed-by: Tom Bergan <tombergan@google.com>

diff --git a/src/net/http/h2_bundle.go b/src/net/http/h2_bundle.go
index 161a1ed1372d87a4eada4c672f654d22227881fa..7a1564f755867eac28318ba95fb25ce122c349f8 100644 (file)
--- a/src/net/http/h2_bundle.go
+++ b/src/net/http/h2_bundle.go
@@ -989,7 +989,7 @@ type http2noDialH2RoundTripper struct{ t *http2Transport }
 
 func (rt http2noDialH2RoundTripper) RoundTrip(req *Request) (*Response, error) {
        res, err := rt.t.RoundTrip(req)
-       if err == http2ErrNoCachedConn {
+       if http2isNoCachedConnError(err) {
                return nil, ErrSkipAltProtocol
        }
        return res, err
@@ -6856,7 +6856,27 @@ func (sew http2stickyErrWriter) Write(p []byte) (n int, err error) {
        return
 }
 
-var http2ErrNoCachedConn = errors.New("http2: no cached connection was available")
+// noCachedConnError is the concrete type of ErrNoCachedConn, needs to be detected
+// by net/http regardless of whether it's its bundled version (in h2_bundle.go with a rewritten type name)
+// or from a user's x/net/http2. As such, as it has a unique method name (IsHTTP2NoCachedConnError) that
+// net/http sniffs for via func isNoCachedConnError.
+type http2noCachedConnError struct{}
+
+func (http2noCachedConnError) IsHTTP2NoCachedConnError() {}
+
+func (http2noCachedConnError) Error() string { return "http2: no cached connection was available" }
+
+// isNoCachedConnError reports whether err is of type noCachedConnError
+// or its equivalent renamed type in net/http2's h2_bundle.go. Both types
+// may coexist in the same running program.
+func http2isNoCachedConnError(err error) bool {
+       _, ok := err.(interface {
+               IsHTTP2NoCachedConnError()
+       })
+       return ok
+}
+
+var http2ErrNoCachedConn error = http2noCachedConnError{}
 
 // RoundTripOpt are options for the Transport.RoundTripOpt method.
 type http2RoundTripOpt struct {

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index c9758e9b38c4366d208ade7b3220c56d782fc5eb..7ef8f0147bdb74f554b719430c9ccf18ad4a1ba1 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -452,7 +452,7 @@ func (t *Transport) RoundTrip(req *Request) (*Response, error) {
 // HTTP request on a new connection. The non-nil input error is the
 // error from roundTrip.
 func (pc *persistConn) shouldRetryRequest(req *Request, err error) bool {
-       if err == http2ErrNoCachedConn {
+       if http2isNoCachedConnError(err) {
                // Issue 16582: if the user started a bunch of
                // requests at once, they can all pick the same conn
                // and violate the server's max concurrent streams.

diff --git a/src/net/http/transport_internal_test.go b/src/net/http/transport_internal_test.go
index 594bf6e2c8374a9b261e2b55ebb3f46204fd8bd2..a5f29c97a9087cc2b7716b50ee247e178e5ef666 100644 (file)
--- a/src/net/http/transport_internal_test.go
+++ b/src/net/http/transport_internal_test.go
@@ -96,6 +96,12 @@ func dummyRequestWithBodyNoGetBody(method string) *Request {
        return req
 }
 
+// issue22091Error acts like a golang.org/x/net/http2.ErrNoCachedConn.
+type issue22091Error struct{}
+
+func (issue22091Error) IsHTTP2NoCachedConnError() {}
+func (issue22091Error) Error() string             { return "issue22091Error" }
+
 func TestTransportShouldRetryRequest(t *testing.T) {
        tests := []struct {
                pc  *persistConn
@@ -123,36 +129,42 @@ func TestTransportShouldRetryRequest(t *testing.T) {
                        want: true,
                },
                3: {
+                       pc:   nil,
+                       req:  nil,
+                       err:  issue22091Error{}, // like an external http2ErrNoCachedConn
+                       want: true,
+               },
+               4: {
                        pc:   &persistConn{reused: true},
                        req:  dummyRequest("POST"),
                        err:  errMissingHost,
                        want: false,
                },
-               4: {
+               5: {
                        pc:   &persistConn{reused: true},
                        req:  dummyRequest("POST"),
                        err:  transportReadFromServerError{},
                        want: false,
                },
-               5: {
+               6: {
                        pc:   &persistConn{reused: true},
                        req:  dummyRequest("GET"),
                        err:  transportReadFromServerError{},
                        want: true,
                },
-               6: {
+               7: {
                        pc:   &persistConn{reused: true},
                        req:  dummyRequest("GET"),
                        err:  errServerClosedIdle,
                        want: true,
                },
-               7: {
+               8: {
                        pc:   &persistConn{reused: true},
                        req:  dummyRequestWithBody("POST"),
                        err:  nothingWrittenError{},
                        want: true,
                },
-               8: {
+               9: {
                        pc:   &persistConn{reused: true},
                        req:  dummyRequestWithBodyNoGetBody("POST"),
                        err:  nothingWrittenError{},