From 4e1824e1434ec0f49e0d8413d65c061a977614d8 Mon Sep 17 00:00:00 2001 From: Claudio Costa Date: Thu, 10 Sep 2020 09:57:00 +0200 Subject: [PATCH] [MM-28397] Fix data race in SqlSupplier (#15419) * Fix data race in SqlSupplier * Add comment Co-authored-by: Mattermod --- store/sqlstore/supplier.go | 12 +++++++++--- store/sqlstore/supplier_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/store/sqlstore/supplier.go b/store/sqlstore/supplier.go index 05d71f1bcb..1d4f3035d2 100644 --- a/store/sqlstore/supplier.go +++ b/store/sqlstore/supplier.go @@ -115,7 +115,7 @@ type SqlSupplier struct { lockedToMaster bool context context.Context license *model.License - licenseMutex sync.Mutex + licenseMutex sync.RWMutex } type TraceOnAdapter struct{} @@ -330,7 +330,10 @@ func (ss *SqlSupplier) GetMaster() *gorp.DbMap { } func (ss *SqlSupplier) GetSearchReplica() *gorp.DbMap { - if ss.license == nil { + ss.licenseMutex.RLock() + license := ss.license + ss.licenseMutex.RUnlock() + if license == nil { return ss.GetMaster() } @@ -343,7 +346,10 @@ func (ss *SqlSupplier) GetSearchReplica() *gorp.DbMap { } func (ss *SqlSupplier) GetReplica() *gorp.DbMap { - if len(ss.settings.DataSourceReplicas) == 0 || ss.lockedToMaster || ss.license == nil { + ss.licenseMutex.RLock() + license := ss.license + ss.licenseMutex.RUnlock() + if len(ss.settings.DataSourceReplicas) == 0 || ss.lockedToMaster || license == nil { return ss.GetMaster() } diff --git a/store/sqlstore/supplier_test.go b/store/sqlstore/supplier_test.go index 85fbcb6abf..69d4cea6d6 100644 --- a/store/sqlstore/supplier_test.go +++ b/store/sqlstore/supplier_test.go @@ -19,6 +19,36 @@ import ( "github.com/mattermost/mattermost-server/v5/store/storetest" ) +// This test was used to consistently reproduce the race +// before the fix in MM-28397. +// Keeping it here to help avoiding future regressions. +func TestSupplierLicenseRace(t *testing.T) { + settings := makeSqlSettings(model.DATABASE_DRIVER_SQLITE) + settings.DataSourceReplicas = []string{":memory:"} + settings.DataSourceSearchReplicas = []string{":memory:"} + supplier := sqlstore.NewSqlSupplier(*settings, nil) + + wg := sync.WaitGroup{} + wg.Add(3) + + go func() { + supplier.UpdateLicense(&model.License{}) + wg.Done() + }() + + go func() { + supplier.GetReplica() + wg.Done() + }() + + go func() { + supplier.GetSearchReplica() + wg.Done() + }() + + wg.Wait() +} + func TestGetReplica(t *testing.T) { t.Parallel() testCases := []struct {