Fix for Postgres connection leak bug

.pre-commit-config.yaml

@@ -1,6 +1,6 @@
 repos:
   - repo: git://github.com/pre-commit/pre-commit-hooks
-    rev: v4.0.1
+    rev: v4.1.0
     hooks:
       - id: trailing-whitespace
       - id: end-of-file-fixer

CONTRIBUTING.md

@@ -8,12 +8,13 @@ Please note we have a code of conduct, please follow it in all your interactions
 
 ## Pull Request Workflow
 
-1. Fork the project repository and create a topic branch in your fork for your work. This fork should almost always be made against the `main` branch of the project you're working on, unless you have previously discussed basing your changes on a different branch with the team.
-2. Make your changes, including any necessary tests to cover new features or fixes that you make.
-3. Ensure that all tests pass.
-4. Submit a Pull Request against the `develop` branch of the project you're working on.
-5. The project committers review pull requests regularly and will work with contributors to provide feedback and guidance around any changes that are required before the pull request can be accepted.
-6. You may merge the Pull Request once your change is accepted. If you do not have permission to do that, you may request the reviewer to merge it for you.
+1. Fork the project repository and create a topic branch in your fork for your work. This fork should almost always be made against the `develop` branch of the project you're working on, unless you have previously discussed basing your changes on a different branch with the team.
+2. This project uses `pre-commit` to ensure certain consistency of [certain stylistic elements](https://github.com/getgort/gort/blob/main/.pre-commit-config.yaml). Install [pre-commit](https://pre-commit.com/#install), and run `pre-commit install` to set up the git hook scripts.
+3. Make your changes, including any necessary tests to cover new features or fixes that you make.
+4. Ensure that all tests pass.
+5. Submit a Pull Request against the `develop` branch of the project you're working on.
+6. The project committers review pull requests regularly and will work with contributors to provide feedback and guidance around any changes that are required before the pull request can be accepted.
+7. You may merge the Pull Request once your change is accepted. If you do not have permission to do that, you may request the reviewer to merge it for you.
 
 ## Code of Conduct
 

dataaccess/postgres/base_test.go

@@ -25,6 +25,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/getgort/gort/bundles"
 	"github.com/getgort/gort/data"
 	"github.com/getgort/gort/dataaccess/tests"
 
@@ -55,8 +56,7 @@ var (
 var DoNotCleanUpDatabase = false
 
 func TestPostgresDataAccessMain(t *testing.T) {
-	ctx, cancel = context.WithTimeout(context.Background(), time.Minute)
-	defer cancel()
+	ctx = context.Background()
 
 	if testing.Short() {
 		t.Skip("skipping test in short mode.")
@@ -71,19 +71,27 @@ func TestPostgresDataAccessMain(t *testing.T) {
 	}()
 	require.NoError(t, err, "failed to start database container")
 
+	ctx, cancel = context.WithTimeout(ctx, time.Minute)
+	defer cancel()
+
 	t.Run("testInitialize", testInitialize)
 
+	t.Run("testConnectionLeaks", testConnectionLeaks)
+
 	dat := tests.NewDataAccessTester(ctx, cancel, da)
 	t.Run("RunAllTests", dat.RunAllTests)
 }
 
 func startDatabaseContainer(ctx context.Context, t *testing.T) (func(), error) {
+	ctx2, cancel := context.WithTimeout(ctx, time.Minute)
+	defer cancel()
+
 	cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
 	if err != nil {
 		return func() {}, err
 	}
 
-	reader, err := cli.ImagePull(ctx, "docker.io/library/postgres:14", types.ImagePullOptions{})
+	reader, err := cli.ImagePull(ctx2, "docker.io/library/postgres:14", types.ImagePullOptions{})
 	if err != nil {
 		return func() {}, err
 	}
@@ -93,7 +101,7 @@ func startDatabaseContainer(ctx context.Context, t *testing.T) (func(), error) {
 	containerName := fmt.Sprintf("gort-test-%x", r.Int())
 
 	resp, err := cli.ContainerCreate(
-		ctx,
+		ctx2,
 		&container.Config{
 			Image:        "postgres:14",
 			ExposedPorts: nat.PortSet{"5432/tcp": {}},
@@ -142,68 +150,97 @@ func testInitialize(t *testing.T) {
 
 	t.Log("Waiting for database to be ready")
 
+loop:
 	for {
 		if time.Now().After(timeoutAt) {
 			t.Error("timeout waiting for database:", timeout)
 			t.FailNow()
 		}
-		db, err := da.connect(ctx, "postgres")
 
-		if db != nil && err == nil {
-			t.Log("database is ready!")
-			break
+		db, err := da.open(ctx, "postgres")
+		switch {
+		case err != nil:
+			t.Logf("connecting to database: %v", err)
+		case db == nil:
+			t.Log("connecting to database: got nil error but nil db")
+		default:
+			t.Log("connecting to database: database is ready!")
+			break loop
 		}
 
 		t.Log("Sleeping 1 second...")
 		time.Sleep(time.Second)
 	}
 
 	err := da.Initialize(ctx)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	t.Run("testDatabaseExists", testDatabaseExists)
 	t.Run("testTablesExist", testTablesExist)
 }
 
+func testConnectionLeaks(t *testing.T) {
+	const count = 100
+
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*30)
+	defer cancel()
+
+	for i := 0; i < count; i++ {
+		bundle, err := getTestBundle()
+		require.NoError(t, err)
+
+		err = da.BundleCreate(ctx, bundle)
+		require.NoError(t, err)
+
+		err = da.BundleDelete(ctx, bundle.Name, bundle.Version)
+		require.NoError(t, err)
+	}
+
+	for name, db := range da.dbs {
+		require.LessOrEqual(t, db.Stats().OpenConnections, 5, name, " has too many open connections")
+		require.LessOrEqual(t, db.Stats().InUse, 2, name, " has too many in-use connections")
+	}
+}
+
 func testDatabaseExists(t *testing.T) {
 	da = NewPostgresDataAccess(configs)
 
 	err := da.Initialize(ctx)
 	assert.NoError(t, err)
 
 	// Test database "gort" exists
-	conn, err := da.connect(ctx, DatabaseGort)
-	assert.NoError(t, err)
-	defer conn.Close()
-
-	assert.NotNil(t, conn)
+	db, err := da.open(ctx, DatabaseGort)
+	require.NoError(t, err)
+	require.NotNil(t, db)
 
-	if conn != nil {
-		assert.NoError(t, conn.Ping())
-	}
+	assert.NoError(t, db.PingContext(ctx))
 
-	// Meta-test: non-existant database should return nil connection
-	nconn, err := da.connect(ctx, "doesntexist")
+	// Meta-test: non-existent database should return nil database
+	nconn, err := da.open(ctx, "doesntexist")
 	assert.Error(t, err)
 	assert.Nil(t, nconn)
 }
 
 func testTablesExist(t *testing.T) {
 	expectedTables := []string{"users", "groups", "groupusers", "tokens", "bundles"}
 
-	db, err := da.connect(ctx, "gort")
+	conn, err := da.connect(ctx)
 	assert.NoError(t, err)
-	defer db.Close()
+	defer conn.Close()
 
 	// Expects these tables
 	for _, table := range expectedTables {
-		b, err := da.tableExists(ctx, table, db)
+		b, err := da.tableExists(ctx, table, conn)
 		assert.NoError(t, err)
 		assert.True(t, b)
 	}
 
 	// Expect not to find this one.
-	b, err := da.tableExists(ctx, "doestexist", db)
+	b, err := da.tableExists(ctx, "doestexist", conn)
 	assert.NoError(t, err)
 	assert.False(t, b)
 }
+
+func getTestBundle() (data.Bundle, error) {
+	return bundles.LoadBundleFromFile("../../testing/test-bundle.yml")
+}