From 020c2e51cf7d96c800aa80e51837c9565c4c45a3 Mon Sep 17 00:00:00 2001 From: Andrii Soluk Date: Fri, 14 Aug 2026 20:18:54 +0300 Subject: [PATCH] Close the client's connections when reuse is disabled EnableConnectionReuse=false builds a client outside the pool for a single request and then drops it. Nothing closes what that client opened: the roundTripper holding the connection goes out of scope still connected, with no reference left anywhere to close it from. So the option that exists to avoid holding connections open held them open harder than leaving reuse on -- reuse at least reaches the same connection again, where this stranded a fresh one per request. Measured before this change, ten requests with reuse off left eight sockets behind, against nine with it on. Close them at the end of Do() when the client came from outside the pool. That is safe precisely because it did: no other request shares it, and Do() has already read the body to completion by then, so nothing is still reading. The pooled path is untouched, where closing would pull connections out from under concurrent callers. Twelve requests with reuse off now leave nothing behind, and a caller that wants a fresh TLS connection per request gets one for the cost of the handshake. --- cycletls/index.go | 8 +++ cycletls/roundtripper.go | 44 ++++++++++++ cycletls/roundtripper_close_test.go | 107 ++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+) create mode 100644 cycletls/roundtripper_close_test.go diff --git a/cycletls/index.go b/cycletls/index.go index 648ba9a8..0f2c901b 100644 --- a/cycletls/index.go +++ b/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 { diff --git a/cycletls/roundtripper.go b/cycletls/roundtripper.go index 50e78e7c..e1cd8da5 100644 --- a/cycletls/roundtripper.go +++ b/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) { diff --git a/cycletls/roundtripper_close_test.go b/cycletls/roundtripper_close_test.go new file mode 100644 index 00000000..fb8dcb68 --- /dev/null +++ b/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) +}