Close the client's connections when EnableConnectionReuse is disabled

cycletls/index.go

@@ -1413,6 +1413,14 @@ func (client CycleTLS) Do(URL string, options Options, Method string) (Response,
 		return Response{}, err
 	}
 
+	// With reuse off this client is not in the pool and serves only this
+	// request, so nothing else can be using its connections once we return.
+	// Close them here: the caller asked for a fresh connection per request, and
+	// a discarded client with no one left to close it just strands sockets.
+	if !enableConnectionReuse {
+		defer closeClientConnections(httpClient.Transport)
+	}
+
 	// Create request using fhttp
 	var bodyReader io.Reader
 	if len(options.BodyBytes) > 0 {

cycletls/roundtripper.go

@@ -588,6 +588,50 @@ func (rt *roundTripper) getAddressMutex(addr string) *sync.Mutex {
 	return mu
 }
 
+// closeAll drops every connection and transport this roundTripper is holding.
+//
+// Meant for a roundTripper that served exactly one request: the client built
+// outside the pool when EnableConnectionReuse is off. There is no second user
+// to wait for, so unlike CloseIdleConnections this does not care whether a
+// connection is idle -- the request is over.
+//
+// Without this, turning connection reuse off leaks harder than leaving it on.
+// The client is discarded after its request and everything it opened goes with
+// it, still connected, with nothing left holding a reference to close it.
+func (rt *roundTripper) closeAll() {
+	rt.Lock()
+
+	type closer interface{ CloseIdleConnections() }
+	var toClose []closer
+
+	for addr, conn := range rt.cachedConnections {
+		_ = conn.Close()
+		delete(rt.cachedConnections, addr)
+	}
+	for addr, transport := range rt.cachedTransports {
+		if c, ok := transport.(closer); ok {
+			toClose = append(toClose, c)
+		}
+		delete(rt.cachedTransports, addr)
+	}
+	rt.Unlock()
+
+	// Outside the lock: CloseIdleConnections walks the transport's own pool and
+	// has no reason to reach back into this roundTripper, but holding rt while
+	// calling into another package's locks is not worth the risk.
+	for _, c := range toClose {
+		c.CloseIdleConnections()
+	}
+}
+
+// closeClientConnections closes what a client's transport is holding, if that
+// transport is one of ours.
+func closeClientConnections(transport http.RoundTripper) {
+	if rt, ok := transport.(*roundTripper); ok {
+		rt.closeAll()
+	}
+}
+
 // CloseIdleConnections closes connections that have been idle for too long
 // If selectedAddr is provided, only close connections not matching this address
 func (rt *roundTripper) CloseIdleConnections(selectedAddr ...string) {

cycletls/roundtripper_close_test.go

@@ -0,0 +1,107 @@
+package cycletls
+
+import (
+	"net"
+	"sync"
+	"testing"
+	"time"
+
+	http "github.com/Danny-Dasilva/fhttp"
+)
+
+// fakeCloseTransport records whether CloseIdleConnections was called on it.
+type fakeCloseTransport struct {
+	mu     sync.Mutex
+	closed int
+}
+
+func (f *fakeCloseTransport) RoundTrip(*http.Request) (*http.Response, error) { return nil, nil }
+
+func (f *fakeCloseTransport) CloseIdleConnections() {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	f.closed++
+}
+
+func (f *fakeCloseTransport) closes() int {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	return f.closed
+}
+
+// fakeCloseConn is a net.Conn that only remembers whether it was closed.
+type fakeCloseConn struct {
+	mu     sync.Mutex
+	closed bool
+}
+
+func (c *fakeCloseConn) Read([]byte) (int, error)         { return 0, nil }
+func (c *fakeCloseConn) Write([]byte) (int, error)        { return 0, nil }
+func (c *fakeCloseConn) LocalAddr() net.Addr              { return nil }
+func (c *fakeCloseConn) RemoteAddr() net.Addr             { return nil }
+func (c *fakeCloseConn) SetDeadline(time.Time) error      { return nil }
+func (c *fakeCloseConn) SetReadDeadline(time.Time) error  { return nil }
+func (c *fakeCloseConn) SetWriteDeadline(time.Time) error { return nil }
+
+func (c *fakeCloseConn) Close() error {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	c.closed = true
+	return nil
+}
+
+func (c *fakeCloseConn) isClosed() bool {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	return c.closed
+}
+
+func newCloseTestRoundTripper() *roundTripper {
+	return &roundTripper{
+		cachedTransports:  make(map[string]http.RoundTripper),
+		cachedConnections: make(map[string]net.Conn),
+		addressMutexes:    make(map[string]*sync.Mutex),
+	}
+}
+
+// closeAll backs EnableConnectionReuse=false: that client serves one request and
+// is then discarded, so everything it opened has to go with it rather than being
+// left connected with nothing holding a reference to close it.
+func TestCloseAllReleasesEverything(t *testing.T) {
+	rt := newCloseTestRoundTripper()
+	ft := &fakeCloseTransport{}
+	conn := &fakeCloseConn{}
+	addr := "oneshot.example:443"
+
+	rt.cachedTransports[addr] = ft
+	rt.cachedConnections[addr] = conn
+
+	rt.closeAll()
+
+	if !conn.isClosed() {
+		t.Error("cached connection left open")
+	}
+	if got := ft.closes(); got != 1 {
+		t.Errorf("CloseIdleConnections called %d times, want 1", got)
+	}
+	if len(rt.cachedTransports) != 0 || len(rt.cachedConnections) != 0 {
+		t.Errorf("caches not cleared: transports=%d conns=%d",
+			len(rt.cachedTransports), len(rt.cachedConnections))
+	}
+}
+
+// closeAll runs on a roundTripper that may have served nothing at all.
+func TestCloseAllOnEmptyRoundTripper(t *testing.T) {
+	rt := newCloseTestRoundTripper()
+	rt.closeAll()
+	if len(rt.cachedTransports) != 0 || len(rt.cachedConnections) != 0 {
+		t.Error("empty roundTripper did not stay empty")
+	}
+}
+
+// closeClientConnections is handed an arbitrary RoundTripper and must not panic
+// on one it did not create.
+func TestCloseClientConnectionsIgnoresForeignTransport(t *testing.T) {
+	closeClientConnections(&fakeCloseTransport{})
+	closeClientConnections(nil)
+}