From aaf975bff1f9779f0398f791d6d6f475072b26f4 Mon Sep 17 00:00:00 2001 From: Mura Li Date: Tue, 17 Sep 2019 17:40:28 +0800 Subject: [PATCH] Fix data race (#8206) * Fix data race * Fix data race in modules/log * Make the scope of lock finner-grained * Use syc.Map * Fix missing change in the test * Do not export LoggerMap --- integrations/testlogger.go | 17 +++++++++-- modules/log/log.go | 58 +++++++++++++++++++++++++++----------- modules/log/log_test.go | 2 +- 3 files changed, 57 insertions(+), 20 deletions(-) diff --git a/integrations/testlogger.go b/integrations/testlogger.go index 43a1471f66..91b94ac9f3 100644 --- a/integrations/testlogger.go +++ b/integrations/testlogger.go @@ -10,6 +10,7 @@ import ( "os" "runtime" "strings" + "sync" "testing" "code.gitea.io/gitea/modules/log" @@ -25,11 +26,21 @@ type TestLogger struct { var writerCloser = &testLoggerWriterCloser{} type testLoggerWriterCloser struct { + sync.RWMutex t testing.TB } +func (w *testLoggerWriterCloser) setT(t *testing.TB) { + w.Lock() + w.t = *t + w.Unlock() +} + func (w *testLoggerWriterCloser) Write(p []byte) (int, error) { - if w.t != nil { + w.RLock() + t := w.t + w.RUnlock() + if t != nil { if len(p) > 0 && p[len(p)-1] == '\n' { p = p[:len(p)-1] } @@ -54,7 +65,7 @@ func (w *testLoggerWriterCloser) Write(p []byte) (int, error) { } }() - w.t.Log(string(p)) + t.Log(string(p)) return len(p), nil } return len(p), nil @@ -77,7 +88,7 @@ func PrintCurrentTest(t testing.TB, skip ...int) { } else { fmt.Fprintf(os.Stdout, "=== %s (%s:%d)\n", t.Name(), strings.TrimPrefix(filename, prefix), line) } - writerCloser.t = t + writerCloser.setT(&t) } // Printf takes a format and args and prints the string to os.Stdout diff --git a/modules/log/log.go b/modules/log/log.go index 0ca0f3adc5..71e88491f1 100644 --- a/modules/log/log.go +++ b/modules/log/log.go @@ -8,13 +8,35 @@ import ( "os" "runtime" "strings" + "sync" ) +type loggerMap struct { + sync.Map +} + +func (m *loggerMap) Load(k string) (*Logger, bool) { + v, ok := m.Map.Load(k) + if !ok { + return nil, false + } + l, ok := v.(*Logger) + return l, ok +} + +func (m *loggerMap) Store(k string, v *Logger) { + m.Map.Store(k, v) +} + +func (m *loggerMap) Delete(k string) { + m.Map.Delete(k) +} + var ( // DEFAULT is the name of the default logger DEFAULT = "default" // NamedLoggers map of named loggers - NamedLoggers = make(map[string]*Logger) + NamedLoggers loggerMap prefix string ) @@ -25,16 +47,16 @@ func NewLogger(bufLen int64, name, provider, config string) *Logger { CriticalWithSkip(1, "Unable to create default logger: %v", err) panic(err) } - return NamedLoggers[DEFAULT] + l, _ := NamedLoggers.Load(DEFAULT) + return l } // NewNamedLogger creates a new named logger for a given configuration func NewNamedLogger(name string, bufLen int64, subname, provider, config string) error { - logger, ok := NamedLoggers[name] + logger, ok := NamedLoggers.Load(name) if !ok { logger = newLogger(name, bufLen) - - NamedLoggers[name] = logger + NamedLoggers.Store(name, logger) } return logger.SetLogger(subname, provider, config) @@ -42,16 +64,16 @@ func NewNamedLogger(name string, bufLen int64, subname, provider, config string) // DelNamedLogger closes and deletes the named logger func DelNamedLogger(name string) { - l, ok := NamedLoggers[name] + l, ok := NamedLoggers.Load(name) if ok { - delete(NamedLoggers, name) + NamedLoggers.Delete(name) l.Close() } } // DelLogger removes the named sublogger from the default logger func DelLogger(name string) error { - logger := NamedLoggers[DEFAULT] + logger, _ := NamedLoggers.Load(DEFAULT) found, err := logger.DelLogger(name) if !found { Trace("Log %s not found, no need to delete", name) @@ -61,21 +83,24 @@ func DelLogger(name string) error { // GetLogger returns either a named logger or the default logger func GetLogger(name string) *Logger { - logger, ok := NamedLoggers[name] + logger, ok := NamedLoggers.Load(name) if ok { return logger } - return NamedLoggers[DEFAULT] + logger, _ = NamedLoggers.Load(DEFAULT) + return logger } // GetLevel returns the minimum logger level func GetLevel() Level { - return NamedLoggers[DEFAULT].GetLevel() + l, _ := NamedLoggers.Load(DEFAULT) + return l.GetLevel() } // GetStacktraceLevel returns the minimum logger level func GetStacktraceLevel() Level { - return NamedLoggers[DEFAULT].GetStacktraceLevel() + l, _ := NamedLoggers.Load(DEFAULT) + return l.GetStacktraceLevel() } // Trace records trace log @@ -169,18 +194,18 @@ func IsFatal() bool { // Close closes all the loggers func Close() { - l, ok := NamedLoggers[DEFAULT] + l, ok := NamedLoggers.Load(DEFAULT) if !ok { return } - delete(NamedLoggers, DEFAULT) + NamedLoggers.Delete(DEFAULT) l.Close() } // Log a message with defined skip and at logging level // A skip of 0 refers to the caller of this command func Log(skip int, level Level, format string, v ...interface{}) { - l, ok := NamedLoggers[DEFAULT] + l, ok := NamedLoggers.Load(DEFAULT) if ok { l.Log(skip+1, level, format, v...) } @@ -195,7 +220,8 @@ type LoggerAsWriter struct { // NewLoggerAsWriter creates a Writer representation of the logger with setable log level func NewLoggerAsWriter(level string, ourLoggers ...*Logger) *LoggerAsWriter { if len(ourLoggers) == 0 { - ourLoggers = []*Logger{NamedLoggers[DEFAULT]} + l, _ := NamedLoggers.Load(DEFAULT) + ourLoggers = []*Logger{l} } l := &LoggerAsWriter{ ourLoggers: ourLoggers, diff --git a/modules/log/log_test.go b/modules/log/log_test.go index 9e3d7527b2..0e7583081c 100644 --- a/modules/log/log_test.go +++ b/modules/log/log_test.go @@ -143,7 +143,7 @@ func TestNewNamedLogger(t *testing.T) { level := INFO err := NewNamedLogger("test", 0, "console", "console", fmt.Sprintf(`{"level":"%s"}`, level.String())) assert.NoError(t, err) - logger := NamedLoggers["test"] + logger, _ := NamedLoggers.Load("test") assert.Equal(t, level, logger.GetLevel()) written, closed := baseConsoleTest(t, logger)