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 <bcmills@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

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
 		}
 	}