net/http: make Transport.CloseIdleConnections also close pending dials

See comment 4 of https://code.google.com/p/go/issues/detail?id=8483#c4:

"So if a user creates a http.Client, issues a bunch of
requests and then wants to shutdown it and all opened connections;
what is she intended to do? The report suggests that just waiting for
all pending requests and calling CloseIdleConnections won't do, as
there can be new racing connections. Obviously she can't do what
you've done in the test, as it uses the unexported function.  If this
happens periodically, it can lead to serious resource leaks (the
transport is also preserved alive).  Am I missing something?"

This CL tracks the user's intention to close all idle
connections (CloseIdleConnections sets it true; and making a
new request sets it false). If a pending dial finishes and
nobody wants it, before it's retained for a future caller, the
"wantIdle" bool is checked and it's closed if the user has
called CloseIdleConnections without a later call to make a new
request.

Fixes #8483

LGTM=adg
R=golang-codereviews, dvyukov, adg
CC=golang-codereviews, rsc
https://golang.org/cl/148970043

diff --git a/src/net/http/export_test.go b/src/net/http/export_test.go
index e5bc02afa261dcd4d1d68e724786758d7a5cb54e..a6980b5389fd9dcc1db2e59b2bf5bc4c6f9e9a1c 100644 (file)
--- a/src/net/http/export_test.go
+++ b/src/net/http/export_test.go
@@ -57,6 +57,26 @@ func (t *Transport) IdleConnChMapSizeForTesting() int {
        return len(t.idleConnCh)
 }
 
+func (t *Transport) IsIdleForTesting() bool {
+       t.idleMu.Lock()
+       defer t.idleMu.Unlock()
+       return t.wantIdle
+}
+
+func (t *Transport) RequestIdleConnChForTesting() {
+       t.getIdleConnCh(connectMethod{nil, "http", "example.com"})
+}
+
+func (t *Transport) PutIdleTestConn() bool {
+       c, _ := net.Pipe()
+       return t.putIdleConn(&persistConn{
+               t:        t,
+               conn:     c,                   // dummy
+               closech:  make(chan struct{}), // so it can be closed
+               cacheKey: connectMethodKey{"", "http", "example.com"},
+       })
+}
+
 func NewTestTimeoutHandler(handler Handler, ch <-chan time.Time) Handler {
        f := func() <-chan time.Time {
                return ch

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index f1a6837527444b22bd1ebce747075f65fea5483e..70e574fc86e24f6571b4593b9c48c4a357521c48 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -47,13 +47,16 @@ const DefaultMaxIdleConnsPerHost = 2
 // HTTPS, and HTTP proxies (for either HTTP or HTTPS with CONNECT).
 // Transport can also cache connections for future re-use.
 type Transport struct {
-       idleMu      sync.Mutex
-       idleConn    map[connectMethodKey][]*persistConn
-       idleConnCh  map[connectMethodKey]chan *persistConn
+       idleMu     sync.Mutex
+       wantIdle   bool // user has requested to close all idle conns
+       idleConn   map[connectMethodKey][]*persistConn
+       idleConnCh map[connectMethodKey]chan *persistConn
+
        reqMu       sync.Mutex
        reqCanceler map[*Request]func()
-       altMu       sync.RWMutex
-       altProto    map[string]RoundTripper // nil or map of URI scheme => RoundTripper
+
+       altMu    sync.RWMutex
+       altProto map[string]RoundTripper // nil or map of URI scheme => RoundTripper
 
        // Proxy specifies a function to return a proxy for a given
        // Request. If the function returns a non-nil error, the
@@ -262,6 +265,7 @@ func (t *Transport) CloseIdleConnections() {
        m := t.idleConn
        t.idleConn = nil
        t.idleConnCh = nil
+       t.wantIdle = true
        t.idleMu.Unlock()
        for _, conns := range m {
                for _, pconn := range conns {
@@ -385,6 +389,11 @@ func (t *Transport) putIdleConn(pconn *persistConn) bool {
                        delete(t.idleConnCh, key)
                }
        }
+       if t.wantIdle {
+               t.idleMu.Unlock()
+               pconn.close()
+               return false
+       }
        if t.idleConn == nil {
                t.idleConn = make(map[connectMethodKey][]*persistConn)
        }
@@ -413,6 +422,7 @@ func (t *Transport) getIdleConnCh(cm connectMethod) chan *persistConn {
        key := cm.key()
        t.idleMu.Lock()
        defer t.idleMu.Unlock()
+       t.wantIdle = false
        if t.idleConnCh == nil {
                t.idleConnCh = make(map[connectMethodKey]chan *persistConn)
        }

diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index 2ffd3597945cffb2dad4dd922c24cc775bec3701..66fcc3c7d4b159be0a8617e78f4966db3194367c 100644 (file)
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -2177,6 +2177,45 @@ func TestRoundTripReturnsProxyError(t *testing.T) {
        }
 }
 
+// tests that putting an idle conn after a call to CloseIdleConns does return it
+func TestTransportCloseIdleConnsThenReturn(t *testing.T) {
+       tr := &Transport{}
+       wantIdle := func(when string, n int) bool {
+               got := tr.IdleConnCountForTesting("|http|example.com") // key used by PutIdleTestConn
+               if got == n {
+                       return true
+               }
+               t.Errorf("%s: idle conns = %d; want %d", when, got, n)
+               return false
+       }
+       wantIdle("start", 0)
+       if !tr.PutIdleTestConn() {
+               t.Fatal("put failed")
+       }
+       if !tr.PutIdleTestConn() {
+               t.Fatal("second put failed")
+       }
+       wantIdle("after put", 2)
+       tr.CloseIdleConnections()
+       if !tr.IsIdleForTesting() {
+               t.Error("should be idle after CloseIdleConnections")
+       }
+       wantIdle("after close idle", 0)
+       if tr.PutIdleTestConn() {
+               t.Fatal("put didn't fail")
+       }
+       wantIdle("after second put", 0)
+
+       tr.RequestIdleConnChForTesting() // should toggle the transport out of idle mode
+       if tr.IsIdleForTesting() {
+               t.Error("shouldn't be idle after RequestIdleConnChForTesting")
+       }
+       if !tr.PutIdleTestConn() {
+               t.Fatal("after re-activation")
+       }
+       wantIdle("after final put", 1)
+}
+
 func wantBody(res *http.Response, err error, want string) error {
        if err != nil {
                return err