Skip to content

Commit

Permalink
database: remove race in TestTxContextWait
Browse files Browse the repository at this point in the history
This test contained a data race.
On line 437, db.BeginTx starts a goroutine that runs tx.awaitDone,
which reads tx.keepConnOnRollback.
On line 445, the test writes to tx.keepConnOnRollback.
tx.awaitDone waits on ctx, but because ctx is timeout-based,
there's no ordering guarantee between the write and the read.

The race detector never caught this before
because the context package implementation of Done
contained enough synchronization to make it safe.
That synchronization is not package of the context API or guarantees,
and the first several releases it was not present.
Another commit soon will remove that synchronization,
exposing the latent data race.

To fix the race, emulate a time-based context
using an explicit cancellation-based context.
This gives us enough control to avoid the race.

Change-Id: I103fe9b987b1d4c02e7a20ac3c22a682652128b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/288493
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Daniel Theophanes <kardianos@gmail.com>
  • Loading branch information
josharian committed Feb 24, 2021
1 parent 26001d1 commit e496120
Showing 1 changed file with 7 additions and 8 deletions.
15 changes: 7 additions & 8 deletions src/database/sql/sql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -431,25 +431,24 @@ func TestTxContextWait(t *testing.T) {
db := newTestDB(t, "people")
defer closeDB(t, db)

ctx, cancel := context.WithTimeout(context.Background(), 15*time.Millisecond)
defer cancel()
ctx, cancel := context.WithCancel(context.Background())

tx, err := db.BeginTx(ctx, nil)
if err != nil {
// Guard against the context being canceled before BeginTx completes.
if err == context.DeadlineExceeded {
t.Skip("tx context canceled prior to first use")
}
t.Fatal(err)
}
tx.keepConnOnRollback = false

go func() {
time.Sleep(15 * time.Millisecond)
cancel()
}()
// This will trigger the *fakeConn.Prepare method which will take time
// performing the query. The ctxDriverPrepare func will check the context
// after this and close the rows and return an error.
_, err = tx.QueryContext(ctx, "WAIT|1s|SELECT|people|age,name|")
if err != context.DeadlineExceeded {
t.Fatalf("expected QueryContext to error with context deadline exceeded but returned %v", err)
if err != context.Canceled {
t.Fatalf("expected QueryContext to error with context canceled but returned %v", err)
}

waitForFree(t, db, 5*time.Second, 0)
Expand Down

0 comments on commit e496120

Please sign in to comment.