From 3090ceacdc64be58405ec549ead0b0dc66c9b5bb Mon Sep 17 00:00:00 2001 From: Aaron Clawson Date: Mon, 29 Nov 2021 16:27:59 +0000 Subject: [PATCH 1/3] Migrated to a global errorHandler delegate --- handler.go | 63 +++++++++++++----------------- handler_test.go | 100 ++++++++++++++++++++++-------------------------- 2 files changed, 72 insertions(+), 91 deletions(-) diff --git a/handler.go b/handler.go index 238f9de12fb..35263e01ac2 100644 --- a/handler.go +++ b/handler.go @@ -18,7 +18,6 @@ import ( "log" "os" "sync" - "sync/atomic" ) var ( @@ -28,44 +27,45 @@ var ( // `Handle` and will be delegated to the registered ErrorHandler. globalErrorHandler = defaultErrorHandler() - // delegateErrorHandlerOnce ensures that a user provided ErrorHandler is - // only ever registered once. - delegateErrorHandlerOnce sync.Once - // Compile-time check that delegator implements ErrorHandler. _ ErrorHandler = (*delegator)(nil) + // Compile-time check that errLogger implements ErrorHandler. + _ ErrorHandler = (*errLogger)(nil) ) -type holder struct { - eh ErrorHandler +type delegator struct { + lock *sync.RWMutex + eh ErrorHandler } -func defaultErrorHandler() *atomic.Value { - v := &atomic.Value{} - v.Store(holder{eh: &delegator{l: log.New(os.Stderr, "", log.LstdFlags)}}) - return v +func (d *delegator) Handle(err error) { + d.lock.RLock() + defer d.lock.RUnlock() + d.eh.Handle(err) } -// delegator logs errors if no delegate is set, otherwise they are delegated. -type delegator struct { - delegate atomic.Value +// setDelegate sets the ErrorHandler delegate. +func (d *delegator) setDelegate(eh ErrorHandler) { + d.lock.Lock() + defer d.lock.Unlock() + d.eh = eh +} + +func defaultErrorHandler() *delegator { + return &delegator{ + lock: &sync.RWMutex{}, + eh: &errLogger{l: log.New(os.Stderr, "", log.LstdFlags)}, + } - l *log.Logger } -// setDelegate sets the ErrorHandler delegate. -func (h *delegator) setDelegate(d ErrorHandler) { - // It is critical this is guarded with delegateErrorHandlerOnce, if it is - // called again with a different concrete type it will panic. - h.delegate.Store(d) +// errLogger logs errors if no delegate is set, otherwise they are delegated. +type errLogger struct { + l *log.Logger } // Handle logs err if no delegate is set, otherwise it is delegated. -func (h *delegator) Handle(err error) { - if d := h.delegate.Load(); d != nil { - d.(ErrorHandler).Handle(err) - return - } +func (h *errLogger) Handle(err error) { h.l.Print(err) } @@ -79,7 +79,7 @@ func (h *delegator) Handle(err error) { // Subsequent calls to SetErrorHandler after the first will not forward errors // to the new ErrorHandler for prior returned instances. func GetErrorHandler() ErrorHandler { - return globalErrorHandler.Load().(holder).eh + return globalErrorHandler } // SetErrorHandler sets the global ErrorHandler to h. @@ -89,16 +89,7 @@ func GetErrorHandler() ErrorHandler { // ErrorHandler. Subsequent calls will set the global ErrorHandler, but not // delegate errors to h. func SetErrorHandler(h ErrorHandler) { - delegateErrorHandlerOnce.Do(func() { - current := GetErrorHandler() - if current == h { - return - } - if internalHandler, ok := current.(*delegator); ok { - internalHandler.setDelegate(h) - } - }) - globalErrorHandler.Store(holder{eh: h}) + globalErrorHandler.setDelegate(h) } // Handle is a convenience function for ErrorHandler().Handle(err) diff --git a/handler_test.go b/handler_test.go index 531ec64c195..b749d2b048b 100644 --- a/handler_test.go +++ b/handler_test.go @@ -19,26 +19,25 @@ import ( "errors" "io/ioutil" "log" - "sync" - "sync/atomic" + "os" "testing" "github.com/stretchr/testify/suite" ) -type errLogger []string +type testErrCatcher []string -func (l *errLogger) Write(p []byte) (int, error) { +func (l *testErrCatcher) Write(p []byte) (int, error) { msg := bytes.TrimRight(p, "\n") (*l) = append(*l, string(msg)) return len(msg), nil } -func (l *errLogger) Reset() { - *l = errLogger([]string{}) +func (l *testErrCatcher) Reset() { + *l = testErrCatcher([]string{}) } -func (l *errLogger) Got() []string { +func (l *testErrCatcher) Got() []string { return []string(*l) } @@ -58,92 +57,91 @@ type HandlerTestSuite struct { suite.Suite origHandler ErrorHandler - errLogger *errLogger + errCatcher *testErrCatcher } func (s *HandlerTestSuite) SetupSuite() { - s.errLogger = new(errLogger) - s.origHandler = globalErrorHandler.Load().(holder).eh + s.errCatcher = new(testErrCatcher) + s.origHandler = globalErrorHandler.eh - globalErrorHandler.Store(holder{eh: &delegator{l: log.New(s.errLogger, "", 0)}}) + globalErrorHandler.setDelegate(&errLogger{l: log.New(s.errCatcher, "", 0)}) } func (s *HandlerTestSuite) TearDownSuite() { - globalErrorHandler.Store(holder{eh: s.origHandler}) - delegateErrorHandlerOnce = sync.Once{} + globalErrorHandler.setDelegate(s.origHandler) } func (s *HandlerTestSuite) SetupTest() { - s.errLogger.Reset() + s.errCatcher.Reset() } func (s *HandlerTestSuite) TearDownTest() { - globalErrorHandler.Store(holder{eh: &delegator{l: log.New(s.errLogger, "", 0)}}) - delegateErrorHandlerOnce = sync.Once{} + globalErrorHandler.setDelegate(&errLogger{l: log.New(s.errCatcher, "", 0)}) } func (s *HandlerTestSuite) TestGlobalHandler() { errs := []string{"one", "two"} GetErrorHandler().Handle(errors.New(errs[0])) Handle(errors.New(errs[1])) - s.Assert().Equal(errs, s.errLogger.Got()) + s.Assert().Equal(errs, s.errCatcher.Got()) } func (s *HandlerTestSuite) TestDelegatedHandler() { eh := GetErrorHandler() - newErrLogger := new(errLogger) - SetErrorHandler(&logger{l: log.New(newErrLogger, "", 0)}) + newErrLogger := new(testErrCatcher) + SetErrorHandler(&errLogger{l: log.New(newErrLogger, "", 0)}) errs := []string{"TestDelegatedHandler"} eh.Handle(errors.New(errs[0])) s.Assert().Equal(errs, newErrLogger.Got()) } -func (s *HandlerTestSuite) TestSettingDefaultIsANoOp() { - SetErrorHandler(GetErrorHandler()) - d := globalErrorHandler.Load().(holder).eh.(*delegator) - s.Assert().Nil(d.delegate.Load()) -} - func (s *HandlerTestSuite) TestNoDropsOnDelegate() { causeErr("") - s.Require().Len(s.errLogger.Got(), 1) + s.Require().Len(s.errCatcher.Got(), 1) // Change to another Handler. We are testing this is loss-less. - newErrLogger := new(errLogger) - secondary := &logger{ + newErrLogger := new(testErrCatcher) + secondary := &errLogger{ l: log.New(newErrLogger, "", 0), } SetErrorHandler(secondary) causeErr("") - s.Assert().Len(s.errLogger.Got(), 1, "original Handler used after delegation") + s.Assert().Len(s.errCatcher.Got(), 1, "original Handler used after delegation") s.Assert().Len(newErrLogger.Got(), 1, "new Handler not used after delegation") } func (s *HandlerTestSuite) TestAllowMultipleSets() { - notUsed := new(errLogger) + notUsed := new(testErrCatcher) - secondary := &logger{l: log.New(notUsed, "", 0)} + secondary := &errLogger{l: log.New(notUsed, "", 0)} SetErrorHandler(secondary) - s.Require().Same(GetErrorHandler(), secondary, "new Handler not set") + s.Require().Same(GetErrorHandler(), globalErrorHandler, "set changed globalErrorHandler") + s.Require().Same(globalErrorHandler.eh, secondary, "new Handler not set") - tertiary := &logger{l: log.New(notUsed, "", 0)} + tertiary := &errLogger{l: log.New(notUsed, "", 0)} SetErrorHandler(tertiary) - s.Assert().Same(GetErrorHandler(), tertiary, "user Handler not overridden") + s.Require().Same(GetErrorHandler(), globalErrorHandler, "set changed globalErrorHandler") + s.Assert().Same(globalErrorHandler.eh, tertiary, "user Handler not overridden") } func TestHandlerTestSuite(t *testing.T) { suite.Run(t, new(HandlerTestSuite)) } +func TestHandlerRace(t *testing.T) { + go SetErrorHandler(&errLogger{log.New(os.Stderr, "", 0)}) + go Handle(errors.New("Error")) +} + func BenchmarkErrorHandler(b *testing.B) { - primary := &delegator{l: log.New(ioutil.Discard, "", 0)} - secondary := &logger{l: log.New(ioutil.Discard, "", 0)} - tertiary := &logger{l: log.New(ioutil.Discard, "", 0)} + primary := &errLogger{l: log.New(ioutil.Discard, "", 0)} + secondary := &errLogger{l: log.New(ioutil.Discard, "", 0)} + tertiary := &errLogger{l: log.New(ioutil.Discard, "", 0)} - globalErrorHandler.Store(holder{eh: primary}) + globalErrorHandler.setDelegate(primary) err := errors.New("BenchmarkErrorHandler") @@ -161,14 +159,9 @@ func BenchmarkErrorHandler(b *testing.B) { GetErrorHandler().Handle(err) Handle(err) - b.StopTimer() - primary.delegate = atomic.Value{} - globalErrorHandler.Store(holder{eh: primary}) - delegateErrorHandlerOnce = sync.Once{} - b.StartTimer() + globalErrorHandler.setDelegate(primary) } - b.StopTimer() reset() } @@ -182,7 +175,7 @@ func BenchmarkGetDefaultErrorHandler(b *testing.B) { } func BenchmarkGetDelegatedErrorHandler(b *testing.B) { - SetErrorHandler(&logger{l: log.New(ioutil.Discard, "", 0)}) + SetErrorHandler(&errLogger{l: log.New(ioutil.Discard, "", 0)}) b.ReportAllocs() b.ResetTimer() @@ -195,9 +188,9 @@ func BenchmarkGetDelegatedErrorHandler(b *testing.B) { } func BenchmarkDefaultErrorHandlerHandle(b *testing.B) { - globalErrorHandler.Store(holder{ - eh: &delegator{l: log.New(ioutil.Discard, "", 0)}, - }) + globalErrorHandler.setDelegate( + &errLogger{l: log.New(ioutil.Discard, "", 0)}, + ) eh := GetErrorHandler() err := errors.New("BenchmarkDefaultErrorHandlerHandle") @@ -214,7 +207,7 @@ func BenchmarkDefaultErrorHandlerHandle(b *testing.B) { func BenchmarkDelegatedErrorHandlerHandle(b *testing.B) { eh := GetErrorHandler() - SetErrorHandler(&logger{l: log.New(ioutil.Discard, "", 0)}) + SetErrorHandler(&errLogger{l: log.New(ioutil.Discard, "", 0)}) err := errors.New("BenchmarkDelegatedErrorHandlerHandle") b.ReportAllocs() @@ -228,23 +221,21 @@ func BenchmarkDelegatedErrorHandlerHandle(b *testing.B) { } func BenchmarkSetErrorHandlerDelegation(b *testing.B) { - alt := &logger{l: log.New(ioutil.Discard, "", 0)} + alt := &errLogger{l: log.New(ioutil.Discard, "", 0)} b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { SetErrorHandler(alt) - b.StopTimer() reset() - b.StartTimer() } } func BenchmarkSetErrorHandlerNoDelegation(b *testing.B) { eh := []ErrorHandler{ - &logger{l: log.New(ioutil.Discard, "", 0)}, - &logger{l: log.New(ioutil.Discard, "", 0)}, + &errLogger{l: log.New(ioutil.Discard, "", 0)}, + &errLogger{l: log.New(ioutil.Discard, "", 0)}, } mod := len(eh) // Do not measure delegation. @@ -262,5 +253,4 @@ func BenchmarkSetErrorHandlerNoDelegation(b *testing.B) { func reset() { globalErrorHandler = defaultErrorHandler() - delegateErrorHandlerOnce = sync.Once{} } From cbf8bad66cc0164c3d3b37f41a4cd67a46858a67 Mon Sep 17 00:00:00 2001 From: Aaron Clawson Date: Mon, 29 Nov 2021 17:09:56 +0000 Subject: [PATCH 2/3] Removed benchmark that doesn't test anything anymore --- handler_test.go | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/handler_test.go b/handler_test.go index b749d2b048b..720f1ee076e 100644 --- a/handler_test.go +++ b/handler_test.go @@ -183,7 +183,6 @@ func BenchmarkGetDelegatedErrorHandler(b *testing.B) { eh = GetErrorHandler() } - b.StopTimer() reset() } @@ -201,7 +200,6 @@ func BenchmarkDefaultErrorHandlerHandle(b *testing.B) { eh.Handle(err) } - b.StopTimer() reset() } @@ -216,7 +214,6 @@ func BenchmarkDelegatedErrorHandlerHandle(b *testing.B) { eh.Handle(err) } - b.StopTimer() reset() } @@ -232,25 +229,6 @@ func BenchmarkSetErrorHandlerDelegation(b *testing.B) { } } -func BenchmarkSetErrorHandlerNoDelegation(b *testing.B) { - eh := []ErrorHandler{ - &errLogger{l: log.New(ioutil.Discard, "", 0)}, - &errLogger{l: log.New(ioutil.Discard, "", 0)}, - } - mod := len(eh) - // Do not measure delegation. - SetErrorHandler(eh[1]) - - b.ReportAllocs() - b.ResetTimer() - for i := 0; i < b.N; i++ { - SetErrorHandler(eh[i%mod]) - } - - b.StopTimer() - reset() -} - func reset() { globalErrorHandler = defaultErrorHandler() } From bd3c3b20b7012c50cb3a22c86f0da3feb8a13bb6 Mon Sep 17 00:00:00 2001 From: Aaron Clawson Date: Wed, 1 Dec 2021 17:05:18 +0000 Subject: [PATCH 3/3] Removed unsued test structure --- handler_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/handler_test.go b/handler_test.go index 720f1ee076e..b06fea071eb 100644 --- a/handler_test.go +++ b/handler_test.go @@ -41,14 +41,6 @@ func (l *testErrCatcher) Got() []string { return []string(*l) } -type logger struct { - l *log.Logger -} - -func (l *logger) Handle(err error) { - l.l.Print(err) -} - func causeErr(text string) { Handle(errors.New(text)) }