Skip to content

Commit

Permalink
test(spanner): fix potential data race in test (#5006)
Browse files Browse the repository at this point in the history
Fixes a potential data race condition in the test cases that use the TestExporter.
The TestExporter keeps all exported spans in a map that should only be accessed while the
mutex has been locked. The mutex is however not exported, which makes it impossible for tests
to lock it. The test cases therefore now only create one session, and wait for this session
to have been created, before it tries to access the exported spans. This prevents a potential
race condition where a BatchCreateSession RPC invocation tries to add an element to the map
with exported spans, while the test case is reading the map to check which spans have been
exported.

Fixes #5005
  • Loading branch information
olavloite committed Oct 20, 2021
1 parent 272a0ae commit 9198ecc
Showing 1 changed file with 40 additions and 2 deletions.
42 changes: 40 additions & 2 deletions spanner/client_test.go
Expand Up @@ -2345,9 +2345,26 @@ func TestClient_DoForEachRow_ShouldNotEndSpanWithIteratorDoneError(t *testing.T)
// This test cannot be parallel, as the TestExporter does not support that.
te := itestutil.NewTestExporter()
defer te.Unregister()
_, client, teardown := setupMockedTestServer(t)
minOpened := uint64(1)
_, client, teardown := setupMockedTestServerWithConfig(t, ClientConfig{
SessionPoolConfig: SessionPoolConfig{
MinOpened: minOpened,
WriteSessions: 0,
},
})
defer teardown()

// Wait until all sessions have been created, so we know that those requests will not interfere with the test.
sp := client.idleSessions
waitFor(t, func() error {
sp.mu.Lock()
defer sp.mu.Unlock()
if uint64(sp.idleList.Len()) != minOpened {
return fmt.Errorf("num open sessions mismatch\nWant: %d\nGot: %d", sp.MinOpened, sp.numOpened)
}
return nil
})

iter := client.Single().Query(context.Background(), NewStatement(SelectSingerIDAlbumIDAlbumTitleFromAlbums))
iter.Do(func(r *Row) error {
return nil
Expand All @@ -2357,6 +2374,8 @@ func TestClient_DoForEachRow_ShouldNotEndSpanWithIteratorDoneError(t *testing.T)
case <-time.After(1 * time.Second):
t.Fatal("No stats were exported before timeout")
}
// Preferably we would want to lock the TestExporter here, but the mutex TestExporter.mu is not exported, so we
// cannot do that.
if len(te.Spans) == 0 {
t.Fatal("No spans were exported")
}
Expand All @@ -2370,9 +2389,26 @@ func TestClient_DoForEachRow_ShouldEndSpanWithQueryError(t *testing.T) {
// This test cannot be parallel, as the TestExporter does not support that.
te := itestutil.NewTestExporter()
defer te.Unregister()
server, client, teardown := setupMockedTestServer(t)
minOpened := uint64(1)
server, client, teardown := setupMockedTestServerWithConfig(t, ClientConfig{
SessionPoolConfig: SessionPoolConfig{
MinOpened: minOpened,
WriteSessions: 0,
},
})
defer teardown()

// Wait until all sessions have been created, so we know that those requests will not interfere with the test.
sp := client.idleSessions
waitFor(t, func() error {
sp.mu.Lock()
defer sp.mu.Unlock()
if uint64(sp.idleList.Len()) != minOpened {
return fmt.Errorf("num open sessions mismatch\nWant: %d\nGot: %d", sp.MinOpened, sp.numOpened)
}
return nil
})

sql := "SELECT * FROM"
server.TestSpanner.PutStatementResult(sql, &StatementResult{
Type: StatementResultError,
Expand All @@ -2388,6 +2424,8 @@ func TestClient_DoForEachRow_ShouldEndSpanWithQueryError(t *testing.T) {
case <-time.After(1 * time.Second):
t.Fatal("No stats were exported before timeout")
}
// Preferably we would want to lock the TestExporter here, but the mutex TestExporter.mu is not exported, so we
// cannot do that.
if len(te.Spans) == 0 {
t.Fatal("No spans were exported")
}
Expand Down

0 comments on commit 9198ecc

Please sign in to comment.