Skip to content

Commit

Permalink
Fix data race issues exposed by running tests in parallel
Browse files Browse the repository at this point in the history
Two changes:
- I missed a couple places to transition from the direct,
  `grpcvtctldserver.NewVtctldServer` to the indirect
  `testutil.NewVtctldServerWithTabletManagerClient`, which guards against
  data races in the tabletmanager protocol registry.
- Added a call to `grpccommon.EnableTracingOpt()` in the `init` of
  vtadmin's tests, to force the one-time write to occur before any
  concurrent tests start up and try to read that value.

Signed-off-by: Andrew Mason <amason@slack-corp.com>
  • Loading branch information
ajm188 committed Mar 9, 2021
1 parent 1a26e62 commit 3f7e006
Showing 1 changed file with 18 additions and 2 deletions.
20 changes: 18 additions & 2 deletions go/vt/vtadmin/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"vitess.io/vitess/go/vt/grpccommon"
"vitess.io/vitess/go/vt/topo"
"vitess.io/vitess/go/vt/topo/memorytopo"
"vitess.io/vitess/go/vt/topo/topoproto"
Expand Down Expand Up @@ -884,8 +885,12 @@ func TestGetKeyspaces(t *testing.T) {
}

servers := []vtctlservicepb.VtctldServer{
grpcvtctldserver.NewVtctldServer(topos[0]),
grpcvtctldserver.NewVtctldServer(topos[1]),
testutil.NewVtctldServerWithTabletManagerClient(t, topos[0], nil, func(ts *topo.Server) vtctlservicepb.VtctldServer {
return grpcvtctldserver.NewVtctldServer(ts)
}),
testutil.NewVtctldServerWithTabletManagerClient(t, topos[1], nil, func(ts *topo.Server) vtctlservicepb.VtctldServer {
return grpcvtctldserver.NewVtctldServer(ts)
}),
}

testutil.WithTestServers(t, func(t *testing.T, clients ...vtctldclient.VtctldClient) {
Expand Down Expand Up @@ -2870,4 +2875,15 @@ func init() {
tmclient.RegisterTabletManagerClientFactory("vtadmin.test", func() tmclient.TabletManagerClient {
return nil
})

// This prevents data-race failures in tests involving grpc client or server
// creation. For example, vtctldclient.New() eventually ends up calling
// grpccommon.EnableTracingOpt() which does a synchronized, one-time
// mutation of the global grpc.EnableTracing. This variable is also read,
// unguarded, by grpc.NewServer(), which is a function call that appears in
// most, if not all, vtadmin.API tests.
//
// Calling this here ensures that one-time write happens before any test
// attempts to read that value by way of grpc.NewServer().
grpccommon.EnableTracingOpt()
}

0 comments on commit 3f7e006

Please sign in to comment.