Add race testing to tests, and fix a few small race conditions in the tests

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 ./...

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

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")
 	}

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

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=

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)

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)
 		}

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")
 	}

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)
 		})
 	}