From ec21715bba18bb1d607621d0adcf05f99ae09090 Mon Sep 17 00:00:00 2001 From: Brian Meek Date: Thu, 21 Jul 2022 18:27:29 -0400 Subject: [PATCH] Add race testing to tests, and fix a few small race conditions in the tests --- build/scripts/build-test-lint.sh | 2 +- docs/CONTRIBUTING.md | 12 ++++++------ federationapi/federationapi_test.go | 10 ++++++++++ go.mod | 2 ++ go.sum | 4 ++++ internal/sqlutil/migrate.go | 4 ++++ keyserver/storage/storage_test.go | 10 +++++++++- setup/jetstream/nats.go | 12 ++++++------ syncapi/sync/requestpool_test.go | 20 ++++++++++++++++---- 9 files changed, 58 insertions(+), 18 deletions(-) diff --git a/build/scripts/build-test-lint.sh b/build/scripts/build-test-lint.sh index 8f0b775b1..32f89c076 100755 --- a/build/scripts/build-test-lint.sh +++ b/build/scripts/build-test-lint.sh @@ -13,4 +13,4 @@ go build ./cmd/... ./build/scripts/find-lint.sh echo "Testing..." -go test -v ./... +go test --race -v ./... diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 169224b9e..c359615df 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -35,16 +35,16 @@ submitting your contribution. ## Comments -Please make sure that the comments adequately explain *why* your code does what it +Please make sure that the comments adequately explain _why_ your code does what it does. If there are statements that are not obvious, please comment what they do. We also have some special tags which we use for searchability. These are: -* `// TODO:` for places where a future review, rewrite or refactor is likely required; -* `// FIXME:` for places where we know there is an outstanding bug that needs a fix; -* `// NOTSPEC:` for places where the behaviour specifically does not match what the +- `// TODO:` for places where a future review, rewrite or refactor is likely required; +- `// FIXME:` for places where we know there is an outstanding bug that needs a fix; +- `// NOTSPEC:` for places where the behaviour specifically does not match what the [Matrix Specification](https://spec.matrix.org/) prescribes, along with a description - of *why* that is the case. + of _why_ that is the case. ## Linting @@ -64,7 +64,7 @@ comment. Please avoid doing this if you can. We also have unit tests which we run via: ```bash -go test ./... +go test --race ./... ``` In general, we like submissions that come with tests. Anything that proves that the diff --git a/federationapi/federationapi_test.go b/federationapi/federationapi_test.go index ae244c566..817ea5aab 100644 --- a/federationapi/federationapi_test.go +++ b/federationapi/federationapi_test.go @@ -22,6 +22,7 @@ import ( "github.com/matrix-org/gomatrix" "github.com/matrix-org/gomatrixserverlib" "github.com/nats-io/nats.go" + "github.com/sasha-s/go-deadlock" ) type fedRoomserverAPI struct { @@ -48,6 +49,7 @@ func (f *fedRoomserverAPI) QueryRoomsForUser(ctx context.Context, req *rsapi.Que // TODO: This struct isn't generic, only works for TestFederationAPIJoinThenKeyUpdate type fedClient struct { + fedClientMutex deadlock.Mutex api.FederationClient allowJoins []*test.Room keys map[gomatrixserverlib.ServerName]struct { @@ -59,6 +61,8 @@ type fedClient struct { } func (f *fedClient) GetServerKeys(ctx context.Context, matrixServer gomatrixserverlib.ServerName) (gomatrixserverlib.ServerKeys, error) { + f.fedClientMutex.Lock() + defer f.fedClientMutex.Unlock() fmt.Println("GetServerKeys:", matrixServer) var keys gomatrixserverlib.ServerKeys var keyID gomatrixserverlib.KeyID @@ -122,6 +126,8 @@ func (f *fedClient) MakeJoin(ctx context.Context, s gomatrixserverlib.ServerName return } func (f *fedClient) SendJoin(ctx context.Context, s gomatrixserverlib.ServerName, event *gomatrixserverlib.Event) (res gomatrixserverlib.RespSendJoin, err error) { + f.fedClientMutex.Lock() + defer f.fedClientMutex.Unlock() for _, r := range f.allowJoins { if r.ID == event.RoomID() { r.InsertEvent(f.t, event.Headered(r.Version)) @@ -134,6 +140,8 @@ func (f *fedClient) SendJoin(ctx context.Context, s gomatrixserverlib.ServerName } func (f *fedClient) SendTransaction(ctx context.Context, t gomatrixserverlib.Transaction) (res gomatrixserverlib.RespSend, err error) { + f.fedClientMutex.Lock() + defer f.fedClientMutex.Unlock() for _, edu := range t.EDUs { if edu.Type == gomatrixserverlib.MDeviceListUpdate { f.sentTxn = true @@ -242,6 +250,8 @@ func testFederationAPIJoinThenKeyUpdate(t *testing.T, dbType test.DBType) { testrig.MustPublishMsgs(t, jsctx, msg) time.Sleep(500 * time.Millisecond) + fc.fedClientMutex.Lock() + defer fc.fedClientMutex.Unlock() if !fc.sentTxn { t.Fatalf("did not send device list update") } diff --git a/go.mod b/go.mod index 0a965ec8e..3c6a8ccb8 100644 --- a/go.mod +++ b/go.mod @@ -39,6 +39,7 @@ require ( github.com/pkg/errors v0.9.1 github.com/pressly/goose v2.7.0+incompatible github.com/prometheus/client_golang v1.12.2 + github.com/sasha-s/go-deadlock v0.3.1 github.com/sirupsen/logrus v1.8.1 github.com/stretchr/testify v1.7.1 github.com/tidwall/gjson v1.14.1 @@ -95,6 +96,7 @@ require ( github.com/onsi/gomega v1.17.0 // indirect github.com/opencontainers/go-digest v1.0.0 // indirect github.com/opencontainers/image-spec v1.0.3-0.20211202183452-c5a74bcca799 // indirect + github.com/petermattis/goid v0.0.0-20180202154549-b0b1615b78e5 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/client_model v0.2.0 // indirect github.com/prometheus/common v0.32.1 // indirect diff --git a/go.sum b/go.sum index df3feeb6e..1018ea511 100644 --- a/go.sum +++ b/go.sum @@ -424,6 +424,8 @@ github.com/opentracing/opentracing-go v1.2.0/go.mod h1:GxEUsuufX4nBwe+T+Wl9TAgYr github.com/openzipkin/zipkin-go v0.1.1/go.mod h1:NtoC/o8u3JlF1lSlyPNswIbeQH9bJTmOf0Erfk+hxe8= github.com/patrickmn/go-cache v2.1.0+incompatible h1:HRMgzkcYKYpi3C8ajMPV8OFXaaRUnok+kx1WdO15EQc= github.com/patrickmn/go-cache v2.1.0+incompatible/go.mod h1:3Qf8kWWT7OJRJbdiICTKqZju1ZixQ/KpMGzzAfe6+WQ= +github.com/petermattis/goid v0.0.0-20180202154549-b0b1615b78e5 h1:q2e307iGHPdTGp0hoxKjt1H5pDo6utceo3dQVK3I5XQ= +github.com/petermattis/goid v0.0.0-20180202154549-b0b1615b78e5/go.mod h1:jvVRKCrJTQWu0XVbaOlby/2lO20uSCHEMzzplHXte1o= github.com/philhofer/fwd v1.0.0/go.mod h1:gk3iGcWd9+svBvR0sR+KPcfE+RNWozjowpeBVG3ZVNU= github.com/pingcap/errors v0.11.4 h1:lFuQV/oaUMGcD2tqt+01ROSmJs75VG1ToEOkZIZ4nE4= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= @@ -466,6 +468,8 @@ github.com/rogpeppe/go-internal v1.6.1 h1:/FiVV8dS/e+YqF2JvO3yXRFbBLTIuSDkuC7aBO github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc= github.com/russross/blackfriday v1.5.2/go.mod h1:JO/DiYxRf+HjHt06OyowR9PTA263kcR/rfWxYHBV53g= github.com/ryszard/goskiplist v0.0.0-20150312221310-2dfbae5fcf46/go.mod h1:uAQ5PCi+MFsC7HjREoAz1BU+Mq60+05gifQSsHSDG/8= +github.com/sasha-s/go-deadlock v0.3.1 h1:sqv7fDNShgjcaxkO0JNcOAlr8B9+cV5Ey/OB71efZx0= +github.com/sasha-s/go-deadlock v0.3.1/go.mod h1:F73l+cr82YSh10GxyRI6qZiCgK64VaZjwesgfQ1/iLM= github.com/sergi/go-diff v1.0.0/go.mod h1:0CfEIISq7TuYL3j771MWULgwwjU+GofnZX9QAmXWZgo= github.com/shurcooL/component v0.0.0-20170202220835-f88ec8f54cc4/go.mod h1:XhFIlyj5a1fBNx5aJTbKoIq0mNaPvOagO+HjB3EtxrY= github.com/shurcooL/events v0.0.0-20181021180414-410e4ca65f48/go.mod h1:5u70Mqkb5O5cxEA8nxTsgrgLehJeAw6Oc4Ab1c/P1HM= diff --git a/internal/sqlutil/migrate.go b/internal/sqlutil/migrate.go index 7518df3c8..a8826343f 100644 --- a/internal/sqlutil/migrate.go +++ b/internal/sqlutil/migrate.go @@ -8,9 +8,11 @@ import ( "github.com/matrix-org/dendrite/setup/config" "github.com/pressly/goose" + "github.com/sasha-s/go-deadlock" ) type Migrations struct { + gooseMutex deadlock.Mutex registeredGoMigrations map[int64]*goose.Migration } @@ -42,6 +44,8 @@ func (m *Migrations) AddNamedMigration(filename string, up func(*sql.Tx) error, // RunDeltas up to the latest version. func (m *Migrations) RunDeltas(db *sql.DB, props *config.DatabaseOptions) error { + m.gooseMutex.Lock() + defer m.gooseMutex.Unlock() maxVer := goose.MaxVersion minVer := int64(0) migrations, err := m.collect(minVer, maxVer) diff --git a/keyserver/storage/storage_test.go b/keyserver/storage/storage_test.go index 44cfb5f2a..725d40b4b 100644 --- a/keyserver/storage/storage_test.go +++ b/keyserver/storage/storage_test.go @@ -10,6 +10,7 @@ import ( "github.com/matrix-org/dendrite/keyserver/types" "github.com/matrix-org/dendrite/test" "github.com/matrix-org/dendrite/test/testrig" + "github.com/sasha-s/go-deadlock" ) var ctx = context.Background() @@ -103,6 +104,9 @@ func TestKeyChangesUpperLimit(t *testing.T) { }) } +var dbLock deadlock.Mutex +var deviceArray = []string{"AAA", "another_device"} + // The purpose of this test is to make sure that the storage layer is generating sequential stream IDs per user, // and that they are returned correctly when querying for device keys. func TestDeviceKeysStreamIDGeneration(t *testing.T) { @@ -169,8 +173,12 @@ func TestDeviceKeysStreamIDGeneration(t *testing.T) { t.Fatalf("Expected StoreLocalDeviceKeys to set StreamID=3 (new key same device) but got %d", msgs[0].StreamID) } + dbLock.Lock() + defer dbLock.Unlock() // Querying for device keys returns the latest stream IDs - msgs, err = db.DeviceKeysForUser(ctx, alice, []string{"AAA", "another_device"}, false) + msgs, err = func() ([]api.DeviceMessage, error) { + return db.DeviceKeysForUser(ctx, alice, deviceArray, false) + }() if err != nil { t.Fatalf("DeviceKeysForUser returned error: %s", err) } diff --git a/setup/jetstream/nats.go b/setup/jetstream/nats.go index 248b0e656..0c03fb6b8 100644 --- a/setup/jetstream/nats.go +++ b/setup/jetstream/nats.go @@ -4,25 +4,25 @@ import ( "fmt" "reflect" "strings" - "sync" "time" "github.com/getsentry/sentry-go" "github.com/matrix-org/dendrite/setup/config" "github.com/matrix-org/dendrite/setup/process" + "github.com/sasha-s/go-deadlock" "github.com/sirupsen/logrus" natsserver "github.com/nats-io/nats-server/v2/server" - "github.com/nats-io/nats.go" natsclient "github.com/nats-io/nats.go" ) type NATSInstance struct { *natsserver.Server - sync.Mutex } -func DeleteAllStreams(js nats.JetStreamContext, cfg *config.JetStream) { +var natsLock deadlock.Mutex + +func DeleteAllStreams(js natsclient.JetStreamContext, cfg *config.JetStream) { for _, stream := range streams { // streams are defined in streams.go name := cfg.Prefixed(stream.Name) _ = js.DeleteStream(name) @@ -30,11 +30,12 @@ func DeleteAllStreams(js nats.JetStreamContext, cfg *config.JetStream) { } func (s *NATSInstance) Prepare(process *process.ProcessContext, cfg *config.JetStream) (natsclient.JetStreamContext, *natsclient.Conn) { + natsLock.Lock() + defer natsLock.Unlock() // check if we need an in-process NATS Server if len(cfg.Addresses) != 0 { return setupNATS(process, cfg, nil) } - s.Lock() if s.Server == nil { var err error s.Server, err = natsserver.NewServer(&natsserver.Options{ @@ -61,7 +62,6 @@ func (s *NATSInstance) Prepare(process *process.ProcessContext, cfg *config.JetS process.ComponentFinished() }() } - s.Unlock() if !s.ReadyForConnections(time.Second * 10) { logrus.Fatalln("NATS did not start in time") } diff --git a/syncapi/sync/requestpool_test.go b/syncapi/sync/requestpool_test.go index 48e6c6c7a..a60d11fa4 100644 --- a/syncapi/sync/requestpool_test.go +++ b/syncapi/sync/requestpool_test.go @@ -9,13 +9,17 @@ import ( "github.com/matrix-org/dendrite/setup/config" "github.com/matrix-org/dendrite/syncapi/types" "github.com/matrix-org/gomatrixserverlib" + "github.com/sasha-s/go-deadlock" ) type dummyPublisher struct { + lock deadlock.Mutex count int } func (d *dummyPublisher) SendPresence(userID string, presence types.Presence, statusMsg *string) error { + d.lock.Lock() + defer d.lock.Unlock() d.count++ return nil } @@ -125,11 +129,19 @@ func TestRequestPool_updatePresence(t *testing.T) { go rp.cleanPresence(db, time.Millisecond*50) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - beforeCount := publisher.count + beforeCount := func() int { + publisher.lock.Lock() + defer publisher.lock.Unlock() + return publisher.count + }() rp.updatePresence(db, tt.args.presence, tt.args.userID) - if tt.wantIncrease && publisher.count <= beforeCount { - t.Fatalf("expected count to increase: %d <= %d", publisher.count, beforeCount) - } + func() { + publisher.lock.Lock() + defer publisher.lock.Unlock() + if tt.wantIncrease && publisher.count <= beforeCount { + t.Fatalf("expected count to increase: %d <= %d", publisher.count, beforeCount) + } + }() time.Sleep(tt.args.sleep) }) }