Fixing all goroutine leaks with client closings

Also adds uber's goroutine leaks checker to almost all tests. The biggest change here is adding context to sidecar pull, which is then cancelled if the container creation fails. This prevents the sidecar goroutine from leaking. The same for making the sidecar channel buffered; the goroutine is not stuck when nobody reads from it. (It is not a memory leak either, GC will remove the channel.) Other changes are just annoying work; putting client.Close() everywhere, db.Close() when Ping() is not successful, client closes in tests/presets, etc. internal/gnomockd/ tests still show goroutine leaks for http transport, but I have no idea what is gnomockd even doing, let alone the tests, so I let that be.
orlangure · May 17, 2024 · dfb7ab0 · dfb7ab0
1 parent cf7544d
commit dfb7ab0
Show file tree

Hide file tree

Showing 37 changed files with 224 additions and 11 deletions.
diff --git a/docker.go b/docker.go
@@ -136,15 +136,17 @@ func (d *docker) startContainer(ctx context.Context, image string, ports NamedPo
 		return nil, fmt.Errorf("can't prepare container: %w", err)
 	}
 
-	sidecarChan := d.setupContainerCleanup(resp.ID, cfg)
+	sidecarChan, cleanupCancel := d.setupContainerCleanup(resp.ID, cfg)
 
 	err = d.client.ContainerStart(ctx, resp.ID, container.StartOptions{})
 	if err != nil {
+		cleanupCancel()
 		return nil, fmt.Errorf("can't start container %s: %w", resp.ID, err)
 	}
 
 	container, err := d.waitForContainerNetwork(ctx, resp.ID, ports)
 	if err != nil {
+		cleanupCancel()
 		return nil, fmt.Errorf("container network isn't ready: %w", err)
 	}
 
@@ -157,8 +159,9 @@ func (d *docker) startContainer(ctx context.Context, image string, ports NamedPo
 	return container, nil
 }
 
-func (d *docker) setupContainerCleanup(id string, cfg *Options) chan string {
-	sidecarChan := make(chan string)
+func (d *docker) setupContainerCleanup(id string, cfg *Options) (chan string, context.CancelFunc) {
+	sidecarChan := make(chan string, 1)
+	bctx, bcancel := context.WithCancel(context.Background())
 
 	go func() {
 		defer close(sidecarChan)
@@ -174,8 +177,9 @@ func (d *docker) setupContainerCleanup(id string, cfg *Options) chan string {
 				return health.HTTPGet(ctx, c.DefaultAddress())
 			}),
 			WithInit(func(ctx context.Context, c *Container) error {
-				return cleaner.Notify(context.Background(), c.DefaultAddress(), id)
+				return cleaner.Notify(bctx, c.DefaultAddress(), id)
 			}),
+			WithContext(bctx),
 		}
 		if cfg.UseLocalImagesFirst {
 			opts = append(opts, WithUseLocalImagesFirst())
@@ -189,7 +193,7 @@ func (d *docker) setupContainerCleanup(id string, cfg *Options) chan string {
 		}
 	}()
 
-	return sidecarChan
+	return sidecarChan, bcancel
 }
 
 func (d *docker) prepareContainer(
@@ -452,6 +456,10 @@ func (d *docker) stopContainer(ctx context.Context, id string) error {
 	return nil
 }
 
+func (d *docker) stopClient() error {
+	return d.client.Close()
+}
+
 func (d *docker) removeContainer(ctx context.Context, id string) error {
 	d.lock.Lock()
 	defer d.lock.Unlock()

diff --git a/gnomock.go b/gnomock.go
@@ -113,6 +113,7 @@ func newContainer(g *g, image string, ports NamedPorts, config *Options) (c *Con
 	if err != nil {
 		return nil, fmt.Errorf("can't create docker client: %w", err)
 	}
+	defer cli.stopClient()
 
 	c, err = cli.startContainer(ctx, image, ports, config)
 	if err != nil {
@@ -229,6 +230,7 @@ func (g *g) stop(c *Container) error {
 	if err != nil {
 		return fmt.Errorf("can't create docker client: %w", err)
 	}
+	defer cli.stopClient()
 
 	id, sidecar := parseID(c.ID)
 	if sidecar != "" {
@@ -237,6 +239,7 @@ func (g *g) stop(c *Container) error {
 			// error in this case won't matter, the container has a self-destruct
 			// timer
 			_ = cli.stopContainer(context.Background(), sidecar)
+			cli.stopClient()
 		}()
 	}
 

diff --git a/gnomock_internal_test.go b/gnomock_internal_test.go
@@ -8,10 +8,15 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"go.uber.org/goleak"
 )
 
 const testImage = "docker.io/orlangure/gnomock-test-image"
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestWaitForContainerNetwork(t *testing.T) {
 	t.Parallel()
 
@@ -35,6 +40,9 @@ func TestWaitForContainerNetwork(t *testing.T) {
 
 	d, err := gg.dockerConnect()
 	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, d.stopClient())
+	}()
 
 	ctx := context.Background()
 
@@ -57,6 +65,8 @@ func TestWaitForContainerNetwork(t *testing.T) {
 		require.Error(t, err)
 		require.True(t, errors.Is(err, ErrPortNotFound), err.Error())
 	})
+
+	require.NoError(t, Stop(container))
 }
 
 func TestEnvAwareClone(t *testing.T) {

diff --git a/gnomock_test.go b/gnomock_test.go
@@ -167,6 +167,7 @@ func TestGnomock_withDebugMode(t *testing.T) {
 	containerList, err = testutil.ListContainerByID(cli, container.ID)
 	require.NoError(t, err)
 	require.Len(t, containerList, 0)
+	require.NoError(t, cli.Close())
 }
 
 func TestGnomock_withLogWriter(t *testing.T) {

diff --git a/go.mod b/go.mod
@@ -39,6 +39,7 @@ require (
 
 require (
 	github.com/rabbitmq/amqp091-go v1.8.1
+	go.uber.org/goleak v1.2.1
 	golang.org/x/mod v0.12.0
 )
 

diff --git a/internal/cleaner/cleaner_test.go b/internal/cleaner/cleaner_test.go
@@ -11,8 +11,14 @@ import (
 	"github.com/orlangure/gnomock/internal/health"
 	"github.com/orlangure/gnomock/internal/testutil"
 	"github.com/stretchr/testify/require"
+
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestCleaner(t *testing.T) {
 	t.Parallel()
 
@@ -53,6 +59,8 @@ func TestCleaner(t *testing.T) {
 	containerList, err = testutil.ListContainerByID(cli, cleanerContainer.ID)
 	require.NoError(t, err)
 	require.Len(t, containerList, 0)
+
+	require.NoError(t, cli.Close())
 }
 
 func TestCleaner_wrongRequest(t *testing.T) {

diff --git a/internal/errors/errors_test.go b/internal/errors/errors_test.go
@@ -8,8 +8,14 @@ import (
 	"github.com/orlangure/gnomock"
 	"github.com/orlangure/gnomock/internal/errors"
 	"github.com/stretchr/testify/require"
+
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestPresetNotFoundError(t *testing.T) {
 	err := errors.NewPresetNotFoundError("invalid")
 	require.Equal(t, "preset 'invalid' not found", err.Error())

diff --git a/internal/health/health_test.go b/internal/health/health_test.go
@@ -8,8 +8,14 @@ import (
 
 	"github.com/orlangure/gnomock/internal/health"
 	"github.com/stretchr/testify/require"
+
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestHTTPGet(t *testing.T) {
 	ctx := context.Background()
 

diff --git a/internal/registry/registry_test.go b/internal/registry/registry_test.go
@@ -6,8 +6,13 @@ import (
 	"github.com/orlangure/gnomock"
 	"github.com/orlangure/gnomock/internal/registry"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 var p gnomock.Preset
 
 func TestRegistry(t *testing.T) {

diff --git a/preset/azurite/preset_test.go b/preset/azurite/preset_test.go
@@ -8,10 +8,15 @@ import (
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob"
 	"github.com/orlangure/gnomock/preset/azurite"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/goleak"
 
 	"github.com/orlangure/gnomock"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestPreset_Blobstorage(t *testing.T) {
 	t.Parallel()
 

diff --git a/preset/cassandra/preset_test.go b/preset/cassandra/preset_test.go
@@ -7,8 +7,14 @@ import (
 	"github.com/orlangure/gnomock"
 	"github.com/orlangure/gnomock/preset/cassandra"
 	"github.com/stretchr/testify/require"
+
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestPreset(t *testing.T) {
 	t.Parallel()
 

diff --git a/preset/cockroachdb/preset.go b/preset/cockroachdb/preset.go
@@ -87,6 +87,10 @@ func (p *P) setDefaults() {
 func healthcheck(_ context.Context, c *gnomock.Container) error {
 	db, err := connect(c, "")
 	if err != nil {
+		if db != nil {
+			db.Close()
+		}
+
 		return err
 	}
 
@@ -108,6 +112,7 @@ func (p *P) initf() gnomock.InitFunc {
 
 		_, err = db.Exec("create database " + p.DB)
 		if err != nil {
+			_ = db.Close()
 			return err
 		}
 

diff --git a/preset/cockroachdb/preset_test.go b/preset/cockroachdb/preset_test.go
@@ -8,8 +8,14 @@ import (
 	"github.com/orlangure/gnomock"
 	"github.com/orlangure/gnomock/preset/cockroachdb"
 	"github.com/stretchr/testify/require"
+
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestPreset(t *testing.T) {
 	t.Parallel()
 
@@ -54,6 +60,7 @@ func testPreset(version string) func(t *testing.T) {
 		require.Equal(t, float64(2), avg)
 		require.Equal(t, float64(1), min)
 		require.Equal(t, float64(3), count)
+		require.NoError(t, db.Close())
 	}
 }
 

diff --git a/preset/elastic/preset_test.go b/preset/elastic/preset_test.go
@@ -9,8 +9,14 @@ import (
 	"github.com/orlangure/gnomock"
 	"github.com/orlangure/gnomock/preset/elastic"
 	"github.com/stretchr/testify/require"
+
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 // these tests have trouble running in parallel, probably due to limited
 // resources
 

diff --git a/preset/influxdb/preset_test.go b/preset/influxdb/preset_test.go
@@ -10,8 +10,14 @@ import (
 	"github.com/orlangure/gnomock"
 	"github.com/orlangure/gnomock/preset/influxdb"
 	"github.com/stretchr/testify/require"
+
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestPreset(t *testing.T) {
 	for _, version := range []string{"alpine"} {
 		t.Run("version", func(t *testing.T) {
@@ -96,5 +102,6 @@ func testPreset(version string, useDefaults bool) func(t *testing.T) {
 		}
 
 		require.Contains(t, orgNames, org)
+		client.Close()
 	}
 }
diff --git a/preset/k3s/preset_test.go b/preset/k3s/preset_test.go
@@ -11,8 +11,14 @@ import (
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
+
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestPreset(t *testing.T) {
 	t.Parallel()
 

diff --git a/preset/kafka/preset_test.go b/preset/kafka/preset_test.go
@@ -10,8 +10,14 @@ import (
 	"github.com/orlangure/gnomock/preset/kafka"
 	kafkaclient "github.com/segmentio/kafka-go"
 	"github.com/stretchr/testify/require"
+
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestPreset(t *testing.T) {
 	messages := []kafka.Message{
 		{

diff --git a/preset/localstack/preset_test.go b/preset/localstack/preset_test.go
@@ -15,8 +15,14 @@ import (
 	"github.com/orlangure/gnomock"
 	"github.com/orlangure/gnomock/preset/localstack"
 	"github.com/stretchr/testify/require"
+
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestPreset_s3(t *testing.T) {
 	t.Parallel()
 

diff --git a/preset/mariadb/preset.go b/preset/mariadb/preset.go
@@ -91,6 +91,9 @@ func (p *P) healthcheck(_ context.Context, c *gnomock.Container) error {
 
 	db, err := p.connect(addr)
 	if err != nil {
+		if db != nil {
+			db.Close()
+		}
 		return err
 	}
 

diff --git a/preset/mariadb/preset_test.go b/preset/mariadb/preset_test.go
@@ -8,8 +8,14 @@ import (
 	"github.com/orlangure/gnomock"
 	"github.com/orlangure/gnomock/preset/mariadb"
 	"github.com/stretchr/testify/require"
+
+	"go.uber.org/goleak"
 )
 
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}
+
 func TestPreset(t *testing.T) {
 	t.Parallel()
 
@@ -52,6 +58,8 @@ func TestPreset(t *testing.T) {
 	require.Equal(t, float64(2), avg)
 	require.Equal(t, float64(1), min)
 	require.Equal(t, float64(3), count)
+
+	require.NoError(t, db.Close())
 }
 
 func TestPreset_withDefaults(t *testing.T) {