Merge branch 'pks-testhelper-goroutine-leakage' into 'master'

testhelper: Enable Goroutine leak checks by default

Closes #3790

See merge request !3909

STYLE.md

@@ -238,22 +238,10 @@ importantly, this includes any calls to `log.Fatal()` and related functions.
 
 ### Common setup
 
-When all tests require a common setup, we use the `TestMain()` function for
-this. `TestMain()` must call `os.Exit()` to indicate whether any tests failed.
-As this will cause deferred function calls to not be processed, we use the
-following pattern:
-
-```
-func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
-}
-```
+The `TestMain()` function shouldn't do any package-specific setup. Instead, all
+tests are supposed to set up required state as part of the tests themselves. All
+`TestMain()` functions must call `testhelper.Run()` though, which performs the
+setup of global state required for tests.
 
 ## Black box and white box testing
 

client/client_test.go

 package client
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }

cmd/gitaly-backup/create.go

@@ -9,6 +9,7 @@ import (
 	"runtime"
 
 	log "github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitaly/v14/client"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/backup"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -46,7 +47,10 @@ func (cmd *createSubcommand) Run(ctx context.Context, stdin io.Reader, stdout io
 		return fmt.Errorf("create: resolve locator: %w", err)
 	}
 
-	manager := backup.NewManager(sink, locator)
+	pool := client.NewPool()
+	defer pool.Close()
+
+	manager := backup.NewManager(sink, locator, pool)
 
 	var pipeline backup.Pipeline
 	pipeline = backup.NewLoggingPipeline(log.StandardLogger())

cmd/gitaly-backup/restore.go

@@ -10,6 +10,7 @@ import (
 	"runtime"
 
 	log "github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitaly/v14/client"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/backup"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -48,7 +49,10 @@ func (cmd *restoreSubcommand) Run(ctx context.Context, stdin io.Reader, stdout i
 		return fmt.Errorf("restore: resolve locator: %w", err)
 	}
 
-	manager := backup.NewManager(sink, locator)
+	pool := client.NewPool()
+	defer pool.Close()
+
+	manager := backup.NewManager(sink, locator, pool)
 
 	var pipeline backup.Pipeline
 	pipeline = backup.NewLoggingPipeline(log.StandardLogger())

cmd/gitaly-backup/testhelper_test.go

 package main
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-
-	cleanup := testhelper.Configure()
-	defer cleanup()
-
-	return m.Run()
+	testhelper.Run(m)
 }

cmd/gitaly-git2go/conflicts/conflicts_test.go

@@ -4,7 +4,6 @@
 package conflicts
 
 import (
-	"os"
 	"testing"
 
 	git "github.com/libgit2/git2go/v31"
@@ -17,14 +16,7 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
 
 func TestConflicts(t *testing.T) {

cmd/gitaly-git2go/main_test.go

@@ -4,19 +4,11 @@
 package main
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }

cmd/gitaly-hooks/hooks_test.go

@@ -96,14 +96,7 @@ func envForHooks(t testing.TB, ctx context.Context, cfg config.Cfg, repo *gitaly
 }
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
 
 func TestHooksPrePostWithSymlinkedStoragePath(t *testing.T) {

cmd/gitaly-lfs-smudge/lfs_smudge_test.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"encoding/json"
 	"net/http"
-	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -52,14 +51,7 @@ type mapConfig struct {
 }
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
 
 func (m *mapConfig) Get(key string) string {

cmd/gitaly-ssh/testhelper_test.go

 package main
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-
-	cleanup := testhelper.Configure()
-	defer cleanup()
-
-	return m.Run()
+	testhelper.Run(m)
 }

cmd/praefect/main.go

@@ -340,6 +340,7 @@ func run(cfgs []starter.Config, conf config.Config) error {
 		nodeManager = nodeMgr
 
 		nodeMgr.Start(conf.Failover.BootstrapInterval.Duration(), conf.Failover.MonitorInterval.Duration())
+		defer nodeMgr.Stop()
 	}
 
 	logger.Infof("election strategy: %q", conf.Failover.ElectionStrategy)
@@ -401,6 +402,8 @@ func run(cfgs []starter.Config, conf config.Config) error {
 		if err != nil {
 			return fmt.Errorf("create gRPC server: %w", err)
 		}
+		defer srv.Stop()
+
 		b.RegisterStarter(starter.New(cfg, srv))
 	}
 

cmd/praefect/main_test.go

@@ -2,7 +2,6 @@ package main
 
 import (
 	"errors"
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -13,14 +12,7 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }
 
 func TestNoConfigFlag(t *testing.T) {

cmd/praefect/subcmd_track_repository_test.go

@@ -154,6 +154,7 @@ func TestAddRepository_Exec(t *testing.T) {
 				nodeMgr, err := nodes.NewManager(testhelper.DiscardTestEntry(t), addCmdConf, db.DB, nil, promtest.NewMockHistogramVec(), protoregistry.GitalyProtoPreregistered, nil, nil, nil)
 				require.NoError(t, err)
 				nodeMgr.Start(0, time.Hour)
+				defer nodeMgr.Stop()
 
 				relativePath := fmt.Sprintf("path/to/test/repo_%s", tn)
 				repoDS := datastore.NewPostgresRepositoryStore(db, conf.StorageNames())

internal/backup/backup.go

@@ -122,10 +122,10 @@ type Manager struct {
 }
 
 // NewManager creates and returns initialized *Manager instance.
-func NewManager(sink Sink, locator Locator) *Manager {
+func NewManager(sink Sink, locator Locator, pool *client.Pool) *Manager {
 	return &Manager{
 		sink:     sink,
-		conns:    client.NewPool(),
+		conns:    pool,
 		locator:  locator,
 		backupID: time.Now().UTC().Format("20060102150405"),
 	}

internal/backup/backup_test.go

@@ -89,7 +89,10 @@ func TestManager_Create(t *testing.T) {
 			ctx, cancel := testhelper.Context()
 			defer cancel()
 
-			fsBackup := NewManager(NewFilesystemSink(path), LegacyLocator{})
+			pool := client.NewPool()
+			defer testhelper.MustClose(t, pool)
+
+			fsBackup := NewManager(NewFilesystemSink(path), LegacyLocator{}, pool)
 			err := fsBackup.Create(ctx, &CreateRequest{
 				Server:     storage.ServerInfo{Address: gitalyAddr, Token: cfg.Auth.Token},
 				Repository: tc.repo,
@@ -351,11 +354,14 @@ func testManagerRestore(t *testing.T, cfg config.Cfg, gitalyAddr string) {
 					repo, bundles := tc.setup(t)
 					repoPath := filepath.Join(cfg.Storages[0].Path, repo.RelativePath)
 
+					pool := client.NewPool()
+					defer testhelper.MustClose(t, pool)
+
 					sink := NewFilesystemSink(path)
 					locator, err := ResolveLocator(locatorName, sink)
 					require.NoError(t, err)
 
-					fsBackup := NewManager(sink, locator)
+					fsBackup := NewManager(sink, locator, pool)
 					err = fsBackup.Restore(ctx, &RestoreRequest{
 						Server:       storage.ServerInfo{Address: gitalyAddr, Token: cfg.Auth.Token},
 						Repository:   repo,

internal/backup/testhelper_test.go

 package backup
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	defer testhelper.MustHaveNoChildProcess()
-
-	cleanup := testhelper.Configure()
-	defer cleanup()
-
-	return m.Run()
+	// gocloud.dev/blob leaks the HTTP connection even if we make sure to close all buckets.
+	//nolint:staticcheck
+	testhelper.Run(m, testhelper.WithDisabledGoroutineChecker())
 }

internal/bootstrap/bootstrap.go

@@ -38,6 +38,7 @@ type upgrader interface {
 	HasParent() bool
 	Ready() error
 	Upgrade() error
+	Stop()
 }
 
 // New performs tableflip initialization
@@ -172,6 +173,7 @@ func (b *Bootstrap) Wait(gracefulTimeout time.Duration) error {
 		err = fmt.Errorf("graceful upgrade: %v", waitError)
 	case s := <-immediateShutdown:
 		err = fmt.Errorf("received signal %q", s)
+		b.upgrader.Stop()
 	case err = <-b.errChan:
 	}
 

internal/bootstrap/bootstrap_test.go

@@ -26,6 +26,8 @@ func (m *mockUpgrader) Exit() <-chan struct{} {
 	return m.exit
 }
 
+func (m *mockUpgrader) Stop() {}
+
 func (m *mockUpgrader) HasParent() bool {
 	return m.hasParent
 }
@@ -104,7 +106,10 @@ func waitWithTimeout(t *testing.T, waitCh <-chan error, timeout time.Duration) e
 }
 
 func TestImmediateTerminationOnSocketError(t *testing.T) {
-	b, server := makeBootstrap(t)
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	b, server := makeBootstrap(t, ctx)
 
 	waitCh := make(chan error)
 	go func() { waitCh <- b.Wait(2 * time.Second) }()
@@ -119,7 +124,10 @@ func TestImmediateTerminationOnSocketError(t *testing.T) {
 func TestImmediateTerminationOnSignal(t *testing.T) {
 	for _, sig := range []syscall.Signal{syscall.SIGTERM, syscall.SIGINT} {
 		t.Run(sig.String(), func(t *testing.T) {
-			b, server := makeBootstrap(t)
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			b, server := makeBootstrap(t, ctx)
 
 			done := server.slowRequest(3 * time.Minute)
 
@@ -146,7 +154,10 @@ func TestImmediateTerminationOnSignal(t *testing.T) {
 }
 
 func TestGracefulTerminationStuck(t *testing.T) {
-	b, server := makeBootstrap(t)
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	b, server := makeBootstrap(t, ctx)
 
 	err := testGracefulUpdate(t, server, b, 3*time.Second, 2*time.Second, nil)
 	require.Contains(t, err.Error(), "grace period expired")
@@ -158,7 +169,9 @@ func TestGracefulTerminationWithSignals(t *testing.T) {
 
 	for _, sig := range []syscall.Signal{syscall.SIGTERM, syscall.SIGINT} {
 		t.Run(sig.String(), func(t *testing.T) {
-			b, server := makeBootstrap(t)
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+			b, server := makeBootstrap(t, ctx)
 
 			err := testGracefulUpdate(t, server, b, 1*time.Second, 2*time.Second, func() {
 				require.NoError(t, self.Signal(sig))
@@ -169,7 +182,9 @@ func TestGracefulTerminationWithSignals(t *testing.T) {
 }
 
 func TestGracefulTerminationServerErrors(t *testing.T) {
-	b, server := makeBootstrap(t)
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+	b, server := makeBootstrap(t, ctx)
 
 	done := make(chan error, 1)
 	// This is a simulation of receiving a listener error during waitGracePeriod
@@ -192,7 +207,9 @@ func TestGracefulTerminationServerErrors(t *testing.T) {
 }
 
 func TestGracefulTermination(t *testing.T) {
-	b, server := makeBootstrap(t)
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+	b, server := makeBootstrap(t, ctx)
 
 	// Using server.Close we bypass the graceful shutdown faking a completed shutdown
 	b.StopAction = func() { server.server.Close() }
@@ -202,21 +219,21 @@ func TestGracefulTermination(t *testing.T) {
 }
 
 func TestPortReuse(t *testing.T) {
-	var addr string
-
 	b, err := New()
 	require.NoError(t, err)
 
 	l, err := b.listen("tcp", "localhost:")
 	require.NoError(t, err, "failed to bind")
 
-	addr = l.Addr().String()
+	addr := l.Addr().String()
 	_, port, err := net.SplitHostPort(addr)
 	require.NoError(t, err)
 
 	l, err = b.listen("tcp", "localhost:"+port)
 	require.NoError(t, err, "failed to bind")
 	require.NoError(t, l.Close())
+
+	b.upgrader.Stop()
 }
 
 func testGracefulUpdate(t *testing.T, server *testServer, b *Bootstrap, waitTimeout, gracefulWait time.Duration, duringGracePeriodCallback func()) error {
@@ -251,7 +268,7 @@ func testGracefulUpdate(t *testing.T, server *testServer, b *Bootstrap, waitTime
 	return waitErr
 }
 
-func makeBootstrap(t *testing.T) (*Bootstrap, *testServer) {
+func makeBootstrap(t *testing.T, ctx context.Context) (*Bootstrap, *testServer) {
 	mux := http.NewServeMux()
 	mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) {
 		w.WriteHeader(200)
@@ -260,12 +277,16 @@ func makeBootstrap(t *testing.T) (*Bootstrap, *testServer) {
 		sec, err := strconv.Atoi(r.URL.Query().Get("seconds"))
 		require.NoError(t, err)
 
-		time.Sleep(time.Duration(sec) * time.Second)
+		select {
+		case <-ctx.Done():
+		case <-time.After(time.Duration(sec) * time.Second):
+		}
 
 		w.WriteHeader(200)
 	})
 
 	s := http.Server{Handler: mux}
+	t.Cleanup(func() { testhelper.MustClose(t, &s) })
 	u := &mockUpgrader{exit: make(chan struct{})}
 
 	b, err := _new(u, net.Listen, false)

internal/bootstrap/testhelper_test.go

 package bootstrap
 
 import (
-	"os"
 	"testing"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
 func TestMain(m *testing.M) {
-	os.Exit(testMain(m))
-}
-
-func testMain(m *testing.M) int {
-	cleanup := testhelper.Configure()
-	defer cleanup()
-	return m.Run()
+	testhelper.Run(m)
 }

internal/cache/diskcache.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"github.com/prometheus/client_golang/prometheus"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/dontpanic"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/safe"
@@ -79,6 +80,9 @@ type DiskCache struct {
 	af          activeFiles
 	cacheConfig cacheConfig
 
+	walkersDone chan struct{}
+	walkerLoops []*dontpanic.Forever
+
 	requestTotals              prometheus.Counter
 	missTotals                 prometheus.Counter
 	bytesStoredtotals          prometheus.Counter
@@ -107,6 +111,7 @@ func New(cfg config.Cfg, locator storage.Locator, opts ...Option) *DiskCache {
 			m:     map[string]int{},
 		},
 		cacheConfig: cacheConfig,
+		walkersDone: make(chan struct{}),
 
 		requestTotals: prometheus.NewCounter(
 			prometheus.CounterOpts{