Skip to content

Commit b7ee784

Browse files
mdelapenyaclaude
andauthored
fix(compose): close docker clients to prevent goroutine leaks (#3661)
* fix(compose): close docker clients to prevent goroutine leaks Close dockerCli.Client() and dockerClient in Down() to release net/http persistConn.readLoop/writeLoop goroutines that were leaking on every Up+Down cycle. Store the *command.DockerCli instance in the DockerCompose struct so its HTTP transport connections can be closed during teardown. Previously the dockerCli was created in NewDockerComposeWith() but never retained, so its HTTP client connections could not be released. Adds TestDockerComposeGoroutineLeak to demonstrate the fix: it snapshots goroutines before an Up+Down cycle and asserts that no net/http persistConn goroutines remain afterward. Fixes #2008 Co-Authored-By: Claude <noreply@anthropic.com> * fix(compose): address code review feedback on goroutine leak fix - Extract Close() method on DockerCompose for explicit resource cleanup, removing the close defer from Down() to keep teardown and resource release concerns separate. Down() is no longer single-use. - Add go.uber.org/goleak as a direct dependency and replace the hand-rolled goroutineSnapshot/goroutineLeaks helpers with goleak.VerifyNone, which handles the buffer-growth loop, exponential retry and cleaner reporting. - Use goleak.IgnoreCurrent() (snapshotted after NewDockerCompose) plus IgnoreTopFunction for Reaper goroutines to scope the leak check to goroutines introduced by the Up+Down lifecycle only. - Call compose.Close() in t.Cleanup, after Down(), to release HTTP transport connections from both dockerCli.Client() and dockerClient. Co-Authored-By: Claude <noreply@anthropic.com> * fix(compose): address second round of code review feedback - Make Close() idempotent by nil-ing dockerCli and dockerClient after closing them, so double-close is a no-op. - Add TODO(#2008) comment on the IgnoreTopFunction filter explaining that it pins on an internal closure name that may change if Reaper is refactored, and should be removed when the Reaper leak is fixed. - Register compose.Down in a t.Cleanup (LIFO after Close) so teardown runs even if the test panics after Up succeeds. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 74c41a3 commit b7ee784

4 files changed

Lines changed: 90 additions & 0 deletions

File tree

modules/compose/compose.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,7 @@ func NewDockerComposeWith(opts ...ComposeStackOption) (*DockerCompose, error) {
182182
logger: composeOptions.Logger,
183183
projectProfiles: composeOptions.Profiles,
184184
composeService: composeService,
185+
dockerCli: dockerCli,
185186
dockerClient: dockerClient,
186187
waitStrategies: make(map[string]wait.Strategy),
187188
containers: make(map[string]*testcontainers.DockerContainer),

modules/compose/compose_api.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515

1616
"github.com/compose-spec/compose-go/v2/cli"
1717
"github.com/compose-spec/compose-go/v2/types"
18+
"github.com/docker/cli/cli/command"
1819
"github.com/docker/compose/v5/pkg/api"
1920
"github.com/moby/moby/client"
2021
"golang.org/x/sync/errgroup"
@@ -180,6 +181,10 @@ type DockerCompose struct {
180181
// used to synchronize operations
181182
lock sync.RWMutex
182183

184+
// dockerCli is the Docker CLI instance used internally by the compose service.
185+
// It is stored here so its HTTP transport connections can be closed after Down().
186+
dockerCli *command.DockerCli
187+
183188
// name/identifier of the stack that will be started
184189
// by default a UUID will be used
185190
name string
@@ -267,6 +272,34 @@ func (d *DockerCompose) Down(ctx context.Context, opts ...StackDownOption) error
267272
return d.composeService.Down(ctx, d.name, options.DownOptions)
268273
}
269274

275+
// Close releases the HTTP transport connections held by the internal Docker CLI
276+
// and the testcontainers Docker client, preventing net/http persistConn goroutine
277+
// leaks. Call Close after Down when the compose stack will no longer be used.
278+
func (d *DockerCompose) Close() error {
279+
d.lock.Lock()
280+
defer d.lock.Unlock()
281+
282+
var errs []error
283+
284+
if d.dockerCli != nil {
285+
if cli := d.dockerCli.Client(); cli != nil {
286+
if err := cli.Close(); err != nil {
287+
errs = append(errs, fmt.Errorf("close docker cli client: %w", err))
288+
}
289+
}
290+
d.dockerCli = nil
291+
}
292+
293+
if d.dockerClient != nil {
294+
if err := d.dockerClient.Close(); err != nil {
295+
errs = append(errs, fmt.Errorf("close docker client: %w", err))
296+
}
297+
d.dockerClient = nil
298+
}
299+
300+
return errors.Join(errs...)
301+
}
302+
270303
func (d *DockerCompose) Up(ctx context.Context, opts ...StackUpOption) (err error) {
271304
d.lock.Lock()
272305
defer d.lock.Unlock()
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
package compose
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
8+
"go.uber.org/goleak"
9+
)
10+
11+
// TestDockerComposeGoroutineLeak verifies that calling Up() followed by Down()
12+
// and Close() does not leak net/http persistConn goroutines from the internal
13+
// Docker CLI HTTP transport.
14+
//
15+
// Before the fix, two goroutines per Up+Down cycle were leaked permanently:
16+
//
17+
// net/http.(*persistConn).readLoop
18+
// net/http.(*persistConn).writeLoop
19+
//
20+
// Note: Reaper (Ryuk) goroutines are intentionally excluded from this check;
21+
// they are a separate pre-existing issue tracked in the same issue but require
22+
// a distinct fix involving Reaper termination signal handling in Down().
23+
func TestDockerComposeGoroutineLeak(t *testing.T) {
24+
path, _ := RenderComposeSimple(t)
25+
26+
compose, err := NewDockerCompose(path)
27+
require.NoError(t, err, "NewDockerCompose()")
28+
29+
// Snapshot goroutines after NewDockerCompose establishes its initial
30+
// Docker provider connection, so IgnoreCurrent covers the provider's
31+
// keep-alive transport goroutines that pre-date the compose Up/Down cycle.
32+
ignoreExisting := goleak.IgnoreCurrent()
33+
34+
// Register Close cleanup first so it runs last (t.Cleanup is LIFO).
35+
// goleak.VerifyNone is called here so it runs after both Down and Close.
36+
t.Cleanup(func() {
37+
require.NoError(t, compose.Close(), "compose.Close()")
38+
goleak.VerifyNone(t,
39+
ignoreExisting,
40+
// TODO(#2008): Remove this ignore when the Reaper goroutine leak is fixed.
41+
// This references an internal anonymous closure that may change if Reaper is refactored.
42+
goleak.IgnoreTopFunction("github.com/testcontainers/testcontainers-go.(*Reaper).connect.func1"),
43+
)
44+
})
45+
46+
ctx := context.Background()
47+
48+
require.NoError(t, compose.Up(ctx, Wait(true)), "compose.Up()")
49+
50+
// Register Down cleanup after Up so it runs before Close (t.Cleanup is LIFO).
51+
// This ensures Down is called even if the test panics after Up succeeds.
52+
t.Cleanup(func() {
53+
require.NoError(t, compose.Down(ctx, RemoveOrphans(true), RemoveVolumes(true)), "compose.Down()")
54+
})
55+
}

modules/compose/go.mod

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ require (
1313
github.com/moby/moby/client v0.4.0
1414
github.com/stretchr/testify v1.11.1
1515
github.com/testcontainers/testcontainers-go v0.42.0
16+
go.uber.org/goleak v1.3.0
1617
go.yaml.in/yaml/v3 v3.0.4
1718
golang.org/x/sync v0.20.0
1819
)

0 commit comments

Comments
 (0)