Skip to content

Commit

Permalink
[release-branch.go1.13] net/http: don't cache http2.erringRoundTrippe…
Browse files Browse the repository at this point in the history
…r connections

Fixes #35087
Updates #34978

Change-Id: I3baf1392ba7366ae6628889c47c343ef702ec438
Reviewed-on: https://go-review.googlesource.com/c/go/+/202078
Reviewed-by: Bryan C. Mills <[email protected]>
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
(cherry picked from commit 88186e5)
Reviewed-on: https://go-review.googlesource.com/c/go/+/202642
Reviewed-by: Emmanuel Odeke <[email protected]>
  • Loading branch information
bradfitz committed Oct 23, 2019
1 parent e64356a commit b2df6bf
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 2 deletions.
9 changes: 7 additions & 2 deletions src/net/http/transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -537,10 +537,15 @@ func (t *Transport) roundTrip(req *Request) (*Response, error) {
if err == nil {
return resp, nil
}
if http2isNoCachedConnError(err) {

// Failed. Clean up and determine whether to retry.

_, isH2DialError := pconn.alt.(http2erringRoundTripper)
if http2isNoCachedConnError(err) || isH2DialError {
t.removeIdleConn(pconn)
t.decConnsPerHost(pconn.cacheKey)
} else if !pconn.shouldRetryRequest(req, err) {
}
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
75 changes: 75 additions & 0 deletions src/net/http/transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5719,3 +5719,78 @@ func TestInvalidHeaderResponse(t *testing.T) {
t.Errorf(`bad "Foo " header value: %q, want %q`, v, "bar")
}
}

// breakableConn is a net.Conn wrapper with a Write method
// that will fail when its brokenState is true.
type breakableConn struct {
net.Conn
*brokenState
}

type brokenState struct {
sync.Mutex
broken bool
}

func (w *breakableConn) Write(b []byte) (n int, err error) {
w.Lock()
defer w.Unlock()
if w.broken {
return 0, errors.New("some write error")
}
return w.Conn.Write(b)
}

// Issue 34978: don't cache a broken HTTP/2 connection
func TestDontCacheBrokenHTTP2Conn(t *testing.T) {
cst := newClientServerTest(t, h2Mode, HandlerFunc(func(w ResponseWriter, r *Request) {}), optQuietLog)
defer cst.close()

var brokenState brokenState

cst.tr.Dial = func(netw, addr string) (net.Conn, error) {
c, err := net.Dial(netw, addr)
if err != nil {
t.Errorf("unexpected Dial error: %v", err)
return nil, err
}
return &breakableConn{c, &brokenState}, err
}

const numReqs = 5
var gotConns uint32 // atomic
for i := 1; i <= numReqs; i++ {
brokenState.Lock()
brokenState.broken = false
brokenState.Unlock()

// doBreak controls whether we break the TCP connection after the TLS
// handshake (before the HTTP/2 handshake). We test a few failures
// in a row followed by a final success.
doBreak := i != numReqs

ctx := httptrace.WithClientTrace(context.Background(), &httptrace.ClientTrace{
GotConn: func(info httptrace.GotConnInfo) {
atomic.AddUint32(&gotConns, 1)
},
TLSHandshakeDone: func(cfg tls.ConnectionState, err error) {
brokenState.Lock()
defer brokenState.Unlock()
if doBreak {
brokenState.broken = true
}
},
})
req, err := NewRequestWithContext(ctx, "GET", cst.ts.URL, nil)
if err != nil {
t.Fatal(err)
}
_, err = cst.c.Do(req)
if doBreak != (err != nil) {
t.Errorf("for iteration %d, doBreak=%v; unexpected error %v", i, doBreak, err)
}
}
if got, want := atomic.LoadUint32(&gotConns), 1; int(got) != want {
t.Errorf("GotConn calls = %v; want %v", got, want)
}
}

0 comments on commit b2df6bf

Please sign in to comment.