Fix bad bug in riversharedtest.DBPool (which is used by TestTx) (#846)

Fixes a bad bug in `riversharedtest.DBPool` in which while lazily
initializing a database pool, *we never actually set the database pool*,
so repeated invocations of `DBPool` would check for an existing pool
that would never exist and create a new one each time. This project
seems to be brute forcing its way by, but I was seeing Postgres
connection exhaustions in other projects.

Boiled down, the problem is that we had this:

    dbPool, err := pgxpool.New(ctx, cmp.Or(
        os.Getenv("TEST_DATABASE_URL"),
        "postgres://localhost:5432/river_test",
    ))

When it should've been this:

    var err error
    dbPool, err = pgxpool.New(ctx, cmp.Or(
        os.Getenv("TEST_DATABASE_URL"),
        "postgres://localhost:5432/river_test",
    ))

Here, fix the problem, and go a step further to simplify the code
somewhat to use a `sync.Once` instead of mutex. The final product is
fewer lines of code, which will hopefully make a bug of this magnitude
easier to spot should it reoccur.

Author: brandur

rivershared/riversharedtest/riversharedtest.go

@@ -32,43 +32,28 @@ func BaseServiceArchetype(tb testing.TB) *baseservice.Archetype {
 	}
 }
 
-// A pool and mutex to protect it, lazily initialized by TestTx. Once open, this
+// A pool and sync.Once to initialize it, invoked by TestTx. Once open, this
 // pool is never explicitly closed, instead closing implicitly as the package
 // tests finish.
 var (
-	dbPool   *pgxpool.Pool //nolint:gochecknoglobals
-	dbPoolMu sync.RWMutex  //nolint:gochecknoglobals
+	dbPool     *pgxpool.Pool //nolint:gochecknoglobals
+	dbPoolOnce sync.Once     //nolint:gochecknoglobals
 )
 
 // DBPool gets a lazily initialized database pool for `TEST_DATABASE_URL` or
 // `river_test` if the former isn't specified.
 func DBPool(ctx context.Context, tb testing.TB) *pgxpool.Pool {
 	tb.Helper()
 
-	tryPool := func() *pgxpool.Pool {
-		dbPoolMu.RLock()
-		defer dbPoolMu.RUnlock()
-		return dbPool
-	}
-
-	if dbPool := tryPool(); dbPool != nil {
-		return dbPool
-	}
-
-	dbPoolMu.Lock()
-	defer dbPoolMu.Unlock()
-
-	// Multiple goroutines may have passed the initial `nil` check on start
-	// up, so check once more to make sure pool hasn't been set yet.
-	if dbPool != nil {
-		return dbPool
-	}
-
-	dbPool, err := pgxpool.New(ctx, cmp.Or(
-		os.Getenv("TEST_DATABASE_URL"),
-		"postgres://localhost:5432/river_test",
-	))
-	require.NoError(tb, err)
+	dbPoolOnce.Do(func() {
+		var err error
+		dbPool, err = pgxpool.New(ctx, cmp.Or(
+			os.Getenv("TEST_DATABASE_URL"),
+			"postgres://localhost:5432/river_test",
+		))
+		require.NoError(tb, err)
+	})
+	require.NotNil(tb, dbPool) // die in case initial connect from another test failed
 
 	return dbPool
 }

rivershared/riversharedtest/riversharedtest_test.go

@@ -15,9 +15,13 @@ func TestDBPool(t *testing.T) {
 
 	ctx := context.Background()
 
-	pool := DBPool(ctx, t)
-	_, err := pool.Exec(ctx, "SELECT 1")
+	pool1 := DBPool(ctx, t)
+	_, err := pool1.Exec(ctx, "SELECT 1")
 	require.NoError(t, err)
+
+	// Both pools should be exactly the same object.
+	pool2 := DBPool(ctx, t)
+	require.Equal(t, pool1, pool2)
 }
 
 func TestTestTx(t *testing.T) {