mirror of
https://github.com/mattermost/mattermost.git
synced 2026-08-30 17:06:34 +08:00
429e03cddc
* [MM-69464] Fix data race in pluginapi.ConfigureLogrus ConfigureLogrus registered the hook via logger.Hooks.Add, which mutates the hook map without holding the logger mutex. logrus copies that map under the mutex when firing hooks, so configuring the logger while another goroutine logs through it is a data race that can panic with "concurrent map read and map write". This is reachable when a plugin re-runs its activation while goroutines from a prior activation still log through logrus.StandardLogger() (originally reported as MM-52096). Use logger.AddHook, which acquires the logger mutex. Also configure the passed-in logger instead of the global standard logger: SetReportCaller and SetLevel previously called the package-level logrus functions, silently mutating logrus.StandardLogger() regardless of the logger argument. Adds a -race regression test that configures the logger concurrently with logging. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Use WaitGroup.Go in the regression test (golangci modernize) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Remove obvious comment on SetReportCaller Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
119 lines
2.9 KiB
Go
119 lines
2.9 KiB
Go
package pluginapi_test
|
|
|
|
import (
|
|
"sync"
|
|
"testing"
|
|
|
|
"github.com/mattermost/mattermost/server/public/plugin/plugintest"
|
|
"github.com/sirupsen/logrus"
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/mock"
|
|
|
|
"github.com/mattermost/mattermost/server/public/pluginapi"
|
|
)
|
|
|
|
func TestLogrus(t *testing.T) {
|
|
testCases := []struct {
|
|
Level logrus.Level
|
|
APICall string
|
|
}{
|
|
{logrus.PanicLevel, "LogError"},
|
|
{logrus.FatalLevel, "LogError"},
|
|
{logrus.ErrorLevel, "LogError"},
|
|
{logrus.WarnLevel, "LogWarn"},
|
|
{logrus.InfoLevel, "LogInfo"},
|
|
{logrus.DebugLevel, "LogDebug"},
|
|
{logrus.TraceLevel, "LogDebug"},
|
|
}
|
|
|
|
for _, testCase := range testCases {
|
|
t.Run(testCase.Level.String(), func(t *testing.T) {
|
|
logger := logrus.New()
|
|
logger.SetLevel(logrus.TraceLevel) // not testing logrus filtering
|
|
logger.ReportCaller = true
|
|
|
|
api := &plugintest.API{}
|
|
defer api.AssertExpectations(t)
|
|
client := pluginapi.NewClient(api, &plugintest.Driver{})
|
|
|
|
pluginapi.ConfigureLogrus(logger, client)
|
|
|
|
// Parameter order of map is non-deterministic, so expect either.
|
|
api.On(testCase.APICall, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
|
|
|
|
entry := logger.WithFields(logrus.Fields{
|
|
"a": "a",
|
|
"b": 1,
|
|
})
|
|
|
|
if testCase.Level == logrus.PanicLevel {
|
|
done := make(chan bool)
|
|
go func() {
|
|
defer func() {
|
|
r := recover()
|
|
assert.NotNil(t, r, "expected panic")
|
|
close(done)
|
|
}()
|
|
|
|
entry.Panic("message")
|
|
}()
|
|
<-done
|
|
} else {
|
|
entry.Log(testCase.Level, "message")
|
|
}
|
|
|
|
// Assert the required API call was executed at most once.
|
|
if api.AssertNumberOfCalls(t, testCase.APICall, 1) {
|
|
call := api.Calls[0]
|
|
for i := 1; i < len(call.Arguments)-1; i += 2 {
|
|
argument := call.Arguments[i]
|
|
value := call.Arguments[i+1]
|
|
|
|
switch argument {
|
|
case "a":
|
|
assert.Equal(t, "a", value, "unexpected value for a")
|
|
case "b":
|
|
assert.Equal(t, "1", value, "unexpected value for b")
|
|
case "plugin_caller":
|
|
assert.IsType(t, "string", value)
|
|
default:
|
|
assert.Fail(t, "unexpected argument and value", "%v: %v", argument, value)
|
|
}
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// TestConfigureLogrusConcurrentWithLogging guards against a data race between
|
|
// registering the hook and logging through the same logger. Run with -race.
|
|
func TestConfigureLogrusConcurrentWithLogging(t *testing.T) {
|
|
api := &plugintest.API{}
|
|
api.On("LogDebug", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe()
|
|
client := pluginapi.NewClient(api, &plugintest.Driver{})
|
|
|
|
logger := logrus.New()
|
|
|
|
var wg sync.WaitGroup
|
|
start := make(chan struct{})
|
|
|
|
for range 4 {
|
|
wg.Go(func() {
|
|
<-start
|
|
for range 500 {
|
|
logger.WithField("k", "v").Debug("message")
|
|
}
|
|
})
|
|
}
|
|
|
|
wg.Go(func() {
|
|
<-start
|
|
for range 50 {
|
|
pluginapi.ConfigureLogrus(logger, client)
|
|
}
|
|
})
|
|
|
|
close(start)
|
|
wg.Wait()
|
|
}
|