From 4408ece955b946056cc0cb4d02d7538167842ac3 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Mon, 16 Oct 2023 22:40:19 +0200 Subject: [PATCH] Fix data race in bulk import tests (#24897) --- server/channels/app/import.go | 2 +- .../channels/store/sqlstore/context_test.go | 46 +++++++++++++++++++ server/public/shared/request/context.go | 7 +++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/server/channels/app/import.go b/server/channels/app/import.go index c877581f52..1997862db2 100644 --- a/server/channels/app/import.go +++ b/server/channels/app/import.go @@ -268,7 +268,7 @@ func (a *App) bulkImport(c request.CTX, jsonlReader io.Reader, attachmentsReader linesChan = make(chan imports.LineImportWorkerData, workers) for i := 0; i < workers; i++ { wg.Add(1) - go a.bulkImportWorker(c, dryRun, &wg, linesChan, errorsChan) + go a.bulkImportWorker(c.Clone(), dryRun, &wg, linesChan, errorsChan) } } diff --git a/server/channels/store/sqlstore/context_test.go b/server/channels/store/sqlstore/context_test.go index 43d3332578..e477cb3c56 100644 --- a/server/channels/store/sqlstore/context_test.go +++ b/server/channels/store/sqlstore/context_test.go @@ -7,6 +7,7 @@ import ( "context" "testing" + "github.com/mattermost/mattermost/server/public/shared/request" "github.com/stretchr/testify/assert" ) @@ -16,3 +17,48 @@ func TestContextMaster(t *testing.T) { m := WithMaster(ctx) assert.True(t, HasMaster(m)) } + +func TestRequestContextWithMaster(t *testing.T) { + t.Run("set and get", func(t *testing.T) { + var rctx request.CTX = request.TestContext(t) + + rctx = RequestContextWithMaster(rctx) + assert.True(t, HasMaster(rctx.Context())) + }) + + t.Run("directly assigning does cause the child to alter the parent", func(t *testing.T) { + var rctx request.CTX = request.TestContext(t) + rctxClone := rctx + rctxClone = RequestContextWithMaster(rctxClone) + + assert.True(t, HasMaster(rctx.Context())) + assert.True(t, HasMaster(rctxClone.Context())) + }) + + t.Run("values get copied from parent", func(t *testing.T) { + var rctx request.CTX = request.TestContext(t) + rctx = RequestContextWithMaster(rctx) + rctxClone := rctx.Clone() + + assert.True(t, HasMaster(rctx.Context())) + assert.True(t, HasMaster(rctxClone.Context())) + }) + + t.Run("changing the child does not alter the parent", func(t *testing.T) { + var rctx request.CTX = request.TestContext(t) + rctxClone := rctx.Clone() + rctxClone = RequestContextWithMaster(rctxClone) + + assert.False(t, HasMaster(rctx.Context())) + assert.True(t, HasMaster(rctxClone.Context())) + }) + + t.Run("changing the parent does not alter the child", func(t *testing.T) { + var rctx request.CTX = request.TestContext(t) + rctxClone := rctx.Clone() + rctx = RequestContextWithMaster(rctx) + + assert.True(t, HasMaster(rctx.Context())) + assert.False(t, HasMaster(rctxClone.Context())) + }) +} diff --git a/server/public/shared/request/context.go b/server/public/shared/request/context.go index 3e43611287..bd073794cd 100644 --- a/server/public/shared/request/context.go +++ b/server/public/shared/request/context.go @@ -54,6 +54,12 @@ func TestContext(t testing.TB) *Context { return EmptyContext(logger) } +// Clone creates a shallow copy of Context, allowing clones to apply per-request changes. +func (c *Context) Clone() CTX { + cCopy := *c + return &cCopy +} + func (c *Context) T(translationID string, args ...any) string { return c.t(translationID, args...) } @@ -125,6 +131,7 @@ func (c *Context) Logger() mlog.LoggerIFace { } type CTX interface { + Clone() CTX T(string, ...interface{}) string Session() *model.Session RequestId() string