Skip to content

Commit

Permalink
net/http: fix handling of HTTP/2 upgrade failures
Browse files Browse the repository at this point in the history
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 <[email protected]>
Run-TryBot: Russ Cox <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
  • Loading branch information
rsc committed Mar 3, 2020
1 parent 96acb74 commit ea1437a
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 7 deletions.
2 changes: 1 addition & 1 deletion src/net/http/omithttp2.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

Expand Down
14 changes: 8 additions & 6 deletions src/net/http/transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
}

Expand Down

0 comments on commit ea1437a

Please sign in to comment.