From 3d1341ce2c22580f905a6422decdffc688ffbb08 Mon Sep 17 00:00:00 2001 From: Aditya Mukerjee Date: Fri, 4 Aug 2017 11:24:22 -0400 Subject: [PATCH 1/4] Add AddHook method for logger --- logger.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/logger.go b/logger.go index 2acab05..fdaf8a6 100644 --- a/logger.go +++ b/logger.go @@ -315,3 +315,9 @@ func (logger *Logger) level() Level { func (logger *Logger) SetLevel(level Level) { atomic.StoreUint32((*uint32)(&logger.Level), uint32(level)) } + +func (logger *Logger) AddHook(hook Hook) { + logger.mu.Lock() + defer logger.mu.Unlock() + logger.Hooks.Add(hook) +} From 66230b287179d93b8a9ca65d5e08495b24163535 Mon Sep 17 00:00:00 2001 From: Aditya Mukerjee Date: Fri, 4 Aug 2017 13:00:10 -0400 Subject: [PATCH 2/4] Add test for race condition in hooks --- hook_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/hook_test.go b/hook_test.go index 13f34cb..d14d1e3 100644 --- a/hook_test.go +++ b/hook_test.go @@ -1,6 +1,7 @@ package logrus import ( + "sync" "testing" "github.com/stretchr/testify/assert" @@ -120,3 +121,22 @@ func TestErrorHookShouldFireOnError(t *testing.T) { assert.Equal(t, hook.Fired, true) }) } + +func TestAddHookRace(t *testing.T) { + var wg sync.WaitGroup + wg.Add(2) + hook := new(ErrorHook) + LogAndAssertJSON(t, func(log *Logger) { + go func() { + defer wg.Done() + log.AddHook(hook) + }() + go func() { + defer wg.Done() + log.Error("test") + }() + wg.Wait() + }, func(fields Fields) { + assert.Equal(t, hook.Fired, true) + }) +} From c830992a61feb7c379da5708645b44cf27041a02 Mon Sep 17 00:00:00 2001 From: Aditya Mukerjee Date: Fri, 4 Aug 2017 13:00:43 -0400 Subject: [PATCH 3/4] Take lock on mutex when firing hooks --- entry.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/entry.go b/entry.go index 5bf582e..1fad45e 100644 --- a/entry.go +++ b/entry.go @@ -94,7 +94,10 @@ func (entry Entry) log(level Level, msg string) { entry.Level = level entry.Message = msg - if err := entry.Logger.Hooks.Fire(level, &entry); err != nil { + entry.Logger.mu.Lock() + err := entry.Logger.Hooks.Fire(level, &entry) + entry.Logger.mu.Unlock() + if err != nil { entry.Logger.mu.Lock() fmt.Fprintf(os.Stderr, "Failed to fire hook: %v\n", err) entry.Logger.mu.Unlock() From 9bc52e39813590fd3b61e23badfc85c3bbee2a73 Mon Sep 17 00:00:00 2001 From: Aditya Mukerjee Date: Fri, 4 Aug 2017 13:01:04 -0400 Subject: [PATCH 4/4] Fix test assertion --- hook_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hook_test.go b/hook_test.go index d14d1e3..4fea751 100644 --- a/hook_test.go +++ b/hook_test.go @@ -137,6 +137,8 @@ func TestAddHookRace(t *testing.T) { }() wg.Wait() }, func(fields Fields) { - assert.Equal(t, hook.Fired, true) + // the line may have been logged + // before the hook was added, so we can't + // actually assert on the hook }) }