Skip to content

Commit 4d6abbe

Browse files
committed
Reimplement driver tests for PGAdvisoryXactLock so they don't involve clock timing
I've gotten a failure a couple times in the driver tests for `PGAdvisoryXactLock` in that it times out when trying to use another transaction to acquire or release the lock: --- FAIL: TestDriverDatabaseSQLLibPQ (19.44s) driver_test.go:40: Generated schema "river_2025_05_08t01_05_22_schema_08" with migrations [1 2 3 4 5 6] on line "main" in 53.591381ms [9 generated] [5 reused] driver_test.go:40: Driver does not support listener; skipping listener tests --- FAIL: TestDriverDatabaseSQLLibPQ/PGAdvisoryXactLock (0.21s) riverdrivertest.go:2691: TestTx using schema: river_2025_05_08t01_05_22_schema_06 riverdrivertest.go:2701: TestTx using schema: river_2025_05_08t01_05_22_schema_06 riverdrivertest.go:2727: Error Trace: /home/runner/work/river/river/internal/riverinternaltest/riverdrivertest/riverdrivertest.go:2727 Error: Goroutine didn't finish in a timely manner Test: TestDriverDatabaseSQLLibPQ/PGAdvisoryXactLock riverdbtest.go:277: Checked in schema "river_2025_05_08t01_05_22_schema_08"; 1 idle schema(s) [137 generated] [133 reused] FAIL FAIL github.com/riverqueue/river 27.844s Here, reimplement this test wholesale, moving away from dependency on clock timing and over to having the other test transaction use the non-blocking `pg_try_advisory_lock `instead. This should eliminate all intermittency from this test case. [1] https://github.com/riverqueue/river/actions/runs/14896356386/job/41839541488
1 parent 4126c71 commit 4d6abbe

1 file changed

Lines changed: 32 additions & 31 deletions

File tree

internal/riverinternaltest/riverdrivertest/riverdrivertest.go

Lines changed: 32 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"github.com/riverqueue/river/internal/rivercommon"
2424
"github.com/riverqueue/river/riverdriver"
2525
"github.com/riverqueue/river/rivershared/testfactory"
26+
"github.com/riverqueue/river/rivershared/util/hashutil"
2627
"github.com/riverqueue/river/rivershared/util/ptrutil"
2728
"github.com/riverqueue/river/rivershared/util/sliceutil"
2829
"github.com/riverqueue/river/rivertype"
@@ -2703,43 +2704,43 @@ func Exercise[TTx any](ctx context.Context, t *testing.T,
27032704

27042705
exec, _ := setup(ctx, t)
27052706

2706-
// Acquire the advisory lock.
2707-
_, err := exec.PGAdvisoryXactLock(ctx, 123456)
2708-
require.NoError(t, err)
2709-
2710-
// Open a new transaction and try to acquire the same lock, which should
2711-
// block because the lock can't be acquired. Verify some amount of wait,
2712-
// cancel the lock acquisition attempt, then verify return.
2713-
{
2714-
otherExec := executorWithTx(ctx, t)
2715-
2716-
goroutineDone := make(chan struct{})
2707+
// It's possible for multiple versions of this test to be running at the
2708+
// same time (from different drivers), so make sure the lock we're
2709+
// acquiring per test is unique by using the complete test name.
2710+
lockHash := hashutil.NewAdvisoryLockHash(0)
2711+
lockHash.Write([]byte(t.Name()))
2712+
lockHash.Write([]byte("123456"))
2713+
key := lockHash.Key()
2714+
2715+
// Tries to acquire the given lock from another test transaction and
2716+
// returns true if the lock was acquired.
2717+
tryAcquireLock := func(exec riverdriver.Executor) bool {
2718+
var lockAcquired bool
2719+
require.NoError(t, exec.QueryRow(ctx, "SELECT pg_try_advisory_lock($1)", key).Scan(&lockAcquired))
2720+
return lockAcquired
2721+
}
27172722

2718-
ctx, cancel := context.WithCancel(ctx)
2719-
t.Cleanup(cancel)
2723+
// Start a transaction to acquire the lock so we can later release the
2724+
// lock by rolling back.
2725+
execTx, err := exec.Begin(ctx)
2726+
require.NoError(t, err)
27202727

2721-
go func() {
2722-
defer close(goroutineDone)
2728+
// Acquire the advisory lock on the main test transaction.
2729+
_, err = execTx.PGAdvisoryXactLock(ctx, key)
2730+
require.NoError(t, err)
27232731

2724-
_, err := otherExec.PGAdvisoryXactLock(ctx, 123456)
2725-
// pgx will produce a context.Canceled error, but pg swallows it to emit its own
2726-
require.Regexp(t, "(context canceled|pq: canceling statement due to user request)", err.Error())
2727-
}()
2732+
// Start another test transaction unrelated to the first.
2733+
otherExec := executorWithTx(ctx, t)
27282734

2729-
select {
2730-
case <-goroutineDone:
2731-
require.FailNow(t, "Unexpectedly acquired lock that should've held by other transaction")
2732-
case <-time.After(50 * time.Millisecond):
2733-
}
2735+
// The other test transaction is unable to acquire the lock because the
2736+
// first test transaction holds it.
2737+
require.False(t, tryAcquireLock(otherExec))
27342738

2735-
cancel()
2739+
// Roll back the first test transaction to release the lock.
2740+
require.NoError(t, execTx.Rollback(ctx))
27362741

2737-
select {
2738-
case <-goroutineDone:
2739-
case <-time.After(50 * time.Millisecond):
2740-
require.FailNow(t, "Goroutine didn't finish in a timely manner")
2741-
}
2742-
}
2742+
// The other test transaction can now acquire the lock.
2743+
require.True(t, tryAcquireLock(otherExec))
27432744
})
27442745

27452746
t.Run("QueueCreateOrSetUpdatedAt", func(t *testing.T) {

0 commit comments

Comments
 (0)