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) +}