[golang-dev] code review 10747044: net/http: fix memory leak in Transport (issue 10747044)

Reviewers: golang-dev1,

Message:
Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
net/http: fix memory leak in Transport

Fixes issue 5794

Please review this at https://codereview.appspot.com/10747044/

Affected files:
    M src/pkg/net/http/export_test.go
    M src/pkg/net/http/transport.go
    M src/pkg/net/http/transport_test.go


Index: src/pkg/net/http/export_test.go
===================================================================
--- a/src/pkg/net/http/export_test.go
+++ b/src/pkg/net/http/export_test.go
@@ -48,6 +48,12 @@
    return len(conns)
   }

+func (t *Transport) IdleConnChMapSizeForTesting() int {
+ t.idleMu.Lock()
+ defer t.idleMu.Unlock()
+ return len(t.idleConnCh)
+}
+
   func NewTestTimeoutHandler(handler Handler, ch <-chan time.Time) Handler {
    f := func() <-chan time.Time {
     return ch
Index: src/pkg/net/http/transport.go
===================================================================
--- a/src/pkg/net/http/transport.go
+++ b/src/pkg/net/http/transport.go
@@ -217,6 +217,7 @@
    t.idleMu.Lock()
    m := t.idleConn
    t.idleConn = nil
+ t.idleConnCh = nil
    t.idleMu.Unlock()
    if m == nil {
     return
@@ -295,8 +296,10 @@
     max = DefaultMaxIdleConnsPerHost
    }
    t.idleMu.Lock()
+
+ waitingDialer := t.idleConnCh[key]
    select {
- case t.idleConnCh[key] <- pconn:
+ case waitingDialer <- pconn:
     // We're done with this pconn and somebody else is
     // currently waiting for a conn of this type (they're
     // actively dialing, but this conn is ready
@@ -305,6 +308,11 @@
     t.idleMu.Unlock()
     return true
    default:
+ if waitingDialer != nil {
+ // They had populated this, but their dial won
+ // first, so we can clean up this map entry.
+ delete(t.idleConnCh, key)
+ }
    }
    if t.idleConn == nil {
     t.idleConn = make(map[string][]*persistConn)
@@ -324,7 +332,13 @@
    return true
   }

+// getIdleConnCh returns a channel to receive and return idle
+// persistent connection for the given connectMethod.
+// It may return nil, if persistent connections are not being used.
   func (t *Transport) getIdleConnCh(cm *connectMethod) chan *persistConn {
+ if t.DisableKeepAlives {
+ return nil
+ }
    key := cm.key()
    t.idleMu.Lock()
    defer t.idleMu.Unlock()
Index: src/pkg/net/http/transport_test.go
===================================================================
--- a/src/pkg/net/http/transport_test.go
+++ b/src/pkg/net/http/transport_test.go
@@ -1575,6 +1575,41 @@
    }
   }

+func TestIdleConnChannelLeak(t *testing.T) {
+ var mu sync.Mutex
+ var n int
+
+ ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+ mu.Lock()
+ n++
+ mu.Unlock()
+ }))
+ defer ts.Close()
+
+ tr := &Transport{
+ Dial: func(netw, addr string) (net.Conn, error) {
+ return net.Dial(netw, ts.Listener.Addr().String())
+ },
+ }
+ defer tr.CloseIdleConnections()
+
+ c := &Client{Transport: tr}
+
+ // First, without keep-alives.
+ for _, disableKeep := range []bool{true, false} {
+ tr.DisableKeepAlives = disableKeep
+ for i := 0; i < 5; i++ {
+ _, err := c.Get(fmt.Sprintf("http://foo-host-%d.tld/", i))
+ if err != nil {
+ t.Fatal(err)
+ }
+ }
+ if got := tr.IdleConnChMapSizeForTesting(); got != 0 {
+ t.Fatalf("ForDisableKeepAlives = %v, map size = %d; want 0",
disableKeep, got)
+ }
+ }
+}
+
   // rgz is a gzip quine that uncompresses to itself.
   var rgz = []byte{
    0x1f, 0x8b, 0x08, 0x08, 0x00, 0x00, 0x00, 0x00,