From 58c1c011a694cc9a813b34c83eebab223edae1fd Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 21 Apr 2015 13:13:16 -0700 Subject: [PATCH] net/http: fix rare Transport readLoop goroutine leak There used to be a small window where if a server declared it would do a keep-alive connection but then actually closed the connection before the roundTrip goroutine scheduled after being sent a response from the readLoop goroutine, then the readLoop goroutine would loop around and block forever reading from a channel because the numExpectedResponses accounting was done too late. Fixes #10457 Change-Id: Icbae937ffe83c792c295b7f4fb929c6a24a4f759 Reviewed-on: https://go-review.googlesource.com/9169 Reviewed-by: Daniel Morsing Run-TryBot: Brad Fitzpatrick --- src/net/http/transport.go | 8 ++++---- src/net/http/transport_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/net/http/transport.go b/src/net/http/transport.go index 79a418765b..b754472be6 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -931,6 +931,10 @@ func (pc *persistConn) readLoop() { } } + pc.lk.Lock() + pc.numExpectedResponses-- + pc.lk.Unlock() + // The connection might be going away when we put the // idleConn below. When that happens, we close the response channel to signal // to roundTrip that the connection is gone. roundTrip waits for @@ -1155,10 +1159,6 @@ WaitResponse: } } - pc.lk.Lock() - pc.numExpectedResponses-- - pc.lk.Unlock() - if re.err != nil { pc.t.setReqCanceler(req.Request, nil) } diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go index e2c926d500..2d52f17721 100644 --- a/src/net/http/transport_test.go +++ b/src/net/http/transport_test.go @@ -2092,6 +2092,38 @@ func TestTransportNoReuseAfterEarlyResponse(t *testing.T) { } } +// Tests that we don't leak Transport persistConn.readLoop goroutines +// when a server hangs up immediately after saying it would keep-alive. +func TestTransportIssue10457(t *testing.T) { + defer afterTest(t) // used to fail in goroutine leak check + ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) { + // Send a response with no body, keep-alive + // (implicit), and then lie and immediately close the + // connection. This forces the Transport's readLoop to + // immediately Peek an io.EOF and get to the point + // that used to hang. + conn, _, _ := w.(Hijacker).Hijack() + conn.Write([]byte("HTTP/1.1 200 OK\r\nFoo: Bar\r\nContent-Length: 0\r\n\r\n")) // keep-alive + conn.Close() + })) + defer ts.Close() + tr := &Transport{} + defer tr.CloseIdleConnections() + cl := &Client{Transport: tr} + res, err := cl.Get(ts.URL) + if err != nil { + t.Fatalf("Get: %v", err) + } + defer res.Body.Close() + + // Just a sanity check that we at least get the response. The real + // test here is that the "defer afterTest" above doesn't find any + // leaked goroutines. + if got, want := res.Header.Get("Foo"), "Bar"; got != want { + t.Errorf("Foo header = %q; want %q", got, want) + } +} + type errorReader struct { err error } -- 2.48.1