Migrated to a global errorHandler delegate

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
-}
-
-func defaultErrorHandler() *atomic.Value {
-	v := &atomic.Value{}
-	v.Store(holder{eh: &delegator{l: log.New(os.Stderr, "", log.LstdFlags)}})
-	return v
-}
-
-// delegator logs errors if no delegate is set, otherwise they are delegated.
 type delegator struct {
-	delegate atomic.Value
+	lock *sync.RWMutex
+	eh   ErrorHandler
+}
 
-	l *log.Logger
+func (d *delegator) Handle(err error) {
+	d.lock.RLock()
+	defer d.lock.RUnlock()
+	d.eh.Handle(err)
 }
 
 // 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)
+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)},
+	}
+
+}
+
+// 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)

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 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)}
+func TestHandlerRace(t *testing.T) {
+	go SetErrorHandler(&errLogger{log.New(os.Stderr, "", 0)})
+	go Handle(errors.New("Error"))
+}
 
-	globalErrorHandler.Store(holder{eh: primary})
+func BenchmarkErrorHandler(b *testing.B) {
+	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.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{}
 }