From ea1437a8cdf6bb3c2d2447833a5d06dbd75f7ae4 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Tue, 25 Feb 2020 13:01:59 -0500 Subject: [PATCH] net/http: fix handling of HTTP/2 upgrade failures If an error occurs during the HTTP/2 upgrade phase, originally this resulted in a pconn with pconn.alt set to an http2erringRoundTripper, which always fails. This is not wanted - we want to retry in this case. CL 202078 added a check for the http2erringRoundTripper to treat it as a failed pconn, but the handling of the failure was wrong in the case where the pconn is not in the idle list at all (common in HTTP/2). This made the added test TestDontCacheBrokenHTTP2Conn flaky. CL 218097 (unsubmitted) proposed to expand the handling of the http2erringRoundTripper after the new check, to dispose of the pconn more thoroughly. Bryan Mills pointed out in that review that we probably shouldn't make the never-going-to-work pconn in the first place. This CL changes the upgrade phase look for the http2erringRoundTripper and return the underlying error instead of claiming to have a working connection. Having done that, the CL undoes the change in CL 202078 and with it the need for CL 218097, but it keeps the new test added by CL 202078. On my laptop, before this commit, TestDontCacheBrokenHTTP2Conn failed 66 times out of 20,000. With this commit, I see 0 out of 20,000. Fixes #34978. Fixes #35113. Change-Id: Ibd908b63c2ae96e159e8e604213d8373afb350e3 Reviewed-on: https://go-review.googlesource.com/c/go/+/220905 Reviewed-by: Bryan C. Mills Run-TryBot: Russ Cox TryBot-Result: Gobot Gobot --- src/net/http/omithttp2.go | 2 +- src/net/http/transport.go | 14 ++++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/net/http/omithttp2.go b/src/net/http/omithttp2.go index 307d93a3b1..7e2f492579 100644 --- a/src/net/http/omithttp2.go +++ b/src/net/http/omithttp2.go @@ -32,7 +32,7 @@ type http2Transport struct { func (*http2Transport) RoundTrip(*Request) (*Response, error) { panic(noHTTP2) } func (*http2Transport) CloseIdleConnections() {} -type http2erringRoundTripper struct{} +type http2erringRoundTripper struct{ err error } func (http2erringRoundTripper) RoundTrip(*Request) (*Response, error) { panic(noHTTP2) } diff --git a/src/net/http/transport.go b/src/net/http/transport.go index d0bfdb412c..15feeaf41f 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -569,14 +569,11 @@ func (t *Transport) roundTrip(req *Request) (*Response, error) { } // Failed. Clean up and determine whether to retry. - - _, isH2DialError := pconn.alt.(http2erringRoundTripper) - if http2isNoCachedConnError(err) || isH2DialError { + if http2isNoCachedConnError(err) { if t.removeIdleConn(pconn) { t.decConnsPerHost(pconn.cacheKey) } - } - if !pconn.shouldRetryRequest(req, err) { + } else if !pconn.shouldRetryRequest(req, err) { // Issue 16465: return underlying net.Conn.Read error from peek, // as we've historically done. if e, ok := err.(transportReadFromServerError); ok { @@ -1637,7 +1634,12 @@ func (t *Transport) dialConn(ctx context.Context, cm connectMethod) (pconn *pers if s := pconn.tlsState; s != nil && s.NegotiatedProtocolIsMutual && s.NegotiatedProtocol != "" { if next, ok := t.TLSNextProto[s.NegotiatedProtocol]; ok { - return &persistConn{t: t, cacheKey: pconn.cacheKey, alt: next(cm.targetAddr, pconn.conn.(*tls.Conn))}, nil + alt := next(cm.targetAddr, pconn.conn.(*tls.Conn)) + if e, ok := alt.(http2erringRoundTripper); ok { + // pconn.conn was closed by next (http2configureTransport.upgradeFn). + return nil, e.err + } + return &persistConn{t: t, cacheKey: pconn.cacheKey, alt: alt}, nil } }