Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent default ErrorHandler self-delegation #5137

Merged
merged 4 commits into from
Apr 4, 2024
Merged

Prevent default ErrorHandler self-delegation #5137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cedbe07
Prevent default ErrorHandler self-delegation
MrAlias Apr 3, 2024
d79d492
Add changelog entry
MrAlias Apr 3, 2024
e8fa2f9
Merge branch 'main' into fix-5135
MrAlias Apr 4, 2024
57bd075
Merge branch 'main' into fix-5135
MrAlias Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Fixed

- Clarify the documentation about equivalence guarantees for the `Set` and `Distinct` types in `go.opentelemetry.io/otel/attribute`. (#5027)
- Prevent default `ErrorHandler` self-delegation. (#5137)
- Update all dependencies to address [GO-2024-2687]. (#5139)

### Removed
Expand Down
12 changes: 4 additions & 8 deletions handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,8 @@ import (
"go.opentelemetry.io/otel/internal/global"
)

var (
// Compile-time check global.ErrDelegator implements ErrorHandler.
_ ErrorHandler = (*global.ErrDelegator)(nil)
// Compile-time check global.ErrLogger implements ErrorHandler.
_ ErrorHandler = (*global.ErrLogger)(nil)
)
// Compile-time check global.ErrDelegator implements ErrorHandler.
var _ ErrorHandler = (*global.ErrDelegator)(nil)

// GetErrorHandler returns the global ErrorHandler instance.
//
Expand All @@ -33,5 +29,5 @@ func GetErrorHandler() ErrorHandler { return global.GetErrorHandler() }
// delegate errors to h.
func SetErrorHandler(h ErrorHandler) { global.SetErrorHandler(h) }

// Handle is a convenience function for ErrorHandler().Handle(err).
func Handle(err error) { global.Handle(err) }
// Handle is a convenience function for GetErrorHandler().Handle(err).
func Handle(err error) { global.GetErrorHandler().Handle(err) }
3 changes: 3 additions & 0 deletions handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ var _ ErrorHandler = &testErrHandler{}
func (eh *testErrHandler) Handle(err error) { eh.err = err }

func TestGlobalErrorHandler(t *testing.T) {
SetErrorHandler(GetErrorHandler())
assert.NotPanics(t, func() { Handle(assert.AnError) }, "Default assignment")

e1 := &testErrHandler{}
SetErrorHandler(e1)
Handle(assert.AnError)
Expand Down
71 changes: 8 additions & 63 deletions internal/global/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,9 @@ package global // import "go.opentelemetry.io/otel/internal/global"

import (
"log"
"os"
"sync/atomic"
)

var (
// GlobalErrorHandler provides an ErrorHandler that can be used
// throughout an OpenTelemetry instrumented project. When a user
// specified ErrorHandler is registered (`SetErrorHandler`) all calls to
// `Handle` and will be delegated to the registered ErrorHandler.
GlobalErrorHandler = defaultErrorHandler()

// Compile-time check that delegator implements ErrorHandler.
_ ErrorHandler = (*ErrDelegator)(nil)
// Compile-time check that errLogger implements ErrorHandler.
_ ErrorHandler = (*ErrLogger)(nil)
)

// ErrorHandler handles irremediable events.
type ErrorHandler interface {
// Handle handles any error deemed irremediable by an OpenTelemetry
Expand All @@ -33,59 +19,18 @@ type ErrDelegator struct {
delegate atomic.Pointer[ErrorHandler]
}

func (d *ErrDelegator) Handle(err error) {
d.getDelegate().Handle(err)
}
// Compile-time check that delegator implements ErrorHandler.
var _ ErrorHandler = (*ErrDelegator)(nil)

func (d *ErrDelegator) getDelegate() ErrorHandler {
return *d.delegate.Load()
func (d *ErrDelegator) Handle(err error) {
if eh := d.delegate.Load(); eh != nil {
(*eh).Handle(err)
return
}
log.Print(err)
}

// setDelegate sets the ErrorHandler delegate.
func (d *ErrDelegator) setDelegate(eh ErrorHandler) {
d.delegate.Store(&eh)
}

func defaultErrorHandler() *ErrDelegator {
d := &ErrDelegator{}
d.setDelegate(&ErrLogger{l: log.New(os.Stderr, "", log.LstdFlags)})
return 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 *ErrLogger) Handle(err error) {
h.l.Print(err)
}

// GetErrorHandler returns the global ErrorHandler instance.
//
// The default ErrorHandler instance returned will log all errors to STDERR
// until an override ErrorHandler is set with SetErrorHandler. All
// ErrorHandler returned prior to this will automatically forward errors to
// the set instance instead of logging.
//
// Subsequent calls to SetErrorHandler after the first will not forward errors
// to the new ErrorHandler for prior returned instances.
func GetErrorHandler() ErrorHandler {
return GlobalErrorHandler
}

// SetErrorHandler sets the global ErrorHandler to h.
//
// The first time this is called all ErrorHandler previously returned from
// GetErrorHandler will send errors to h instead of the default logging
// ErrorHandler. Subsequent calls will set the global ErrorHandler, but not
// delegate errors to h.
func SetErrorHandler(h ErrorHandler) {
GlobalErrorHandler.setDelegate(h)
}

// Handle is a convenience function for ErrorHandler().Handle(err).
func Handle(err error) {
GetErrorHandler().Handle(err)
}
230 changes: 20 additions & 210 deletions internal/global/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,225 +6,35 @@ package global
import (
"bytes"
"errors"
"io"
"log"
"sync"
"os"
"strings"
"testing"

"github.com/stretchr/testify/suite"
)

type testErrCatcher []string

func (l *testErrCatcher) Write(p []byte) (int, error) {
msg := bytes.TrimRight(p, "\n")
(*l) = append(*l, string(msg))
return len(msg), nil
}

func (l *testErrCatcher) Reset() {
*l = testErrCatcher([]string{})
}

func (l *testErrCatcher) Got() []string {
return []string(*l)
}

func causeErr(text string) {
Handle(errors.New(text))
}

type HandlerTestSuite struct {
suite.Suite

origHandler ErrorHandler
errCatcher *testErrCatcher
}

func (s *HandlerTestSuite) SetupSuite() {
s.errCatcher = new(testErrCatcher)
s.origHandler = GlobalErrorHandler.getDelegate()

GlobalErrorHandler.setDelegate(&ErrLogger{l: log.New(s.errCatcher, "", 0)})
}

func (s *HandlerTestSuite) TearDownSuite() {
GlobalErrorHandler.setDelegate(s.origHandler)
}

func (s *HandlerTestSuite) SetupTest() {
s.errCatcher.Reset()
}

func (s *HandlerTestSuite) TearDownTest() {
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.errCatcher.Got())
}
func TestErrDelegator(t *testing.T) {
buf := new(bytes.Buffer)
log.Default().SetOutput(buf)
t.Cleanup(func() { log.Default().SetOutput(os.Stderr) })

func (s *HandlerTestSuite) TestDelegatedHandler() {
eh := GetErrorHandler()
e := &ErrDelegator{}

newErrLogger := new(testErrCatcher)
SetErrorHandler(&ErrLogger{l: log.New(newErrLogger, "", 0)})
err := errors.New("testing")
e.Handle(err)

errs := []string{"TestDelegatedHandler"}
eh.Handle(errors.New(errs[0]))
s.Assert().Equal(errs, newErrLogger.Got())
}

func (s *HandlerTestSuite) TestNoDropsOnDelegate() {
causeErr("")
s.Require().Len(s.errCatcher.Got(), 1)

// Change to another Handler. We are testing this is loss-less.
newErrLogger := new(testErrCatcher)
secondary := &ErrLogger{
l: log.New(newErrLogger, "", 0),
got := buf.String()
if !strings.Contains(got, err.Error()) {
t.Error("default handler did not log")
}
SetErrorHandler(secondary)

causeErr("")
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(testErrCatcher)
buf.Reset()

secondary := &ErrLogger{l: log.New(notUsed, "", 0)}
SetErrorHandler(secondary)
s.Require().Same(GetErrorHandler(), GlobalErrorHandler, "set changed globalErrorHandler")
s.Require().Same(GlobalErrorHandler.getDelegate(), secondary, "new Handler not set")
var gotErr error
e.setDelegate(fnErrHandler(func(e error) { gotErr = e }))
e.Handle(err)

tertiary := &ErrLogger{l: log.New(notUsed, "", 0)}
SetErrorHandler(tertiary)
s.Require().Same(GetErrorHandler(), GlobalErrorHandler, "set changed globalErrorHandler")
s.Assert().Same(GlobalErrorHandler.getDelegate(), tertiary, "user Handler not overridden")
}

func TestHandlerTestSuite(t *testing.T) {
suite.Run(t, new(HandlerTestSuite))
}

func TestHandlerConcurrentSafe(t *testing.T) {
// In order not to pollute the test output.
SetErrorHandler(&ErrLogger{log.New(io.Discard, "", 0)})

var wg sync.WaitGroup
wg.Add(1)
go func() {
defer wg.Done()
SetErrorHandler(&ErrLogger{log.New(io.Discard, "", 0)})
}()
wg.Add(1)
go func() {
defer wg.Done()
Handle(errors.New("error"))
}()

wg.Wait()
reset()
}

func BenchmarkErrorHandler(b *testing.B) {
primary := &ErrLogger{l: log.New(io.Discard, "", 0)}
secondary := &ErrLogger{l: log.New(io.Discard, "", 0)}
tertiary := &ErrLogger{l: log.New(io.Discard, "", 0)}

GlobalErrorHandler.setDelegate(primary)

err := errors.New("benchmark error handler")

b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
GetErrorHandler().Handle(err)
Handle(err)

SetErrorHandler(secondary)
GetErrorHandler().Handle(err)
Handle(err)

SetErrorHandler(tertiary)
GetErrorHandler().Handle(err)
Handle(err)

GlobalErrorHandler.setDelegate(primary)
}

reset()
}

var eh ErrorHandler

func BenchmarkGetDefaultErrorHandler(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
eh = GetErrorHandler()
if buf.String() != "" {
t.Error("delegate not set")
} else if !errors.Is(gotErr, err) {
t.Error("error not passed to delegate")
}
}

func BenchmarkGetDelegatedErrorHandler(b *testing.B) {
SetErrorHandler(&ErrLogger{l: log.New(io.Discard, "", 0)})

b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
eh = GetErrorHandler()
}

reset()
}

func BenchmarkDefaultErrorHandlerHandle(b *testing.B) {
GlobalErrorHandler.setDelegate(
&ErrLogger{l: log.New(io.Discard, "", 0)},
)

eh := GetErrorHandler()
err := errors.New("benchmark default error handler handle")

b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
eh.Handle(err)
}

reset()
}

func BenchmarkDelegatedErrorHandlerHandle(b *testing.B) {
eh := GetErrorHandler()
SetErrorHandler(&ErrLogger{l: log.New(io.Discard, "", 0)})
err := errors.New("benchmark delegated error handler handle")

b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
eh.Handle(err)
}

reset()
}

func BenchmarkSetErrorHandlerDelegation(b *testing.B) {
alt := &ErrLogger{l: log.New(io.Discard, "", 0)}

b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
SetErrorHandler(alt)

reset()
}
}

func reset() {
GlobalErrorHandler = defaultErrorHandler()
}
2 changes: 1 addition & 1 deletion internal/global/internal_logging_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func TestLoggerConcurrentSafe(t *testing.T) {
}()

wg.Wait()
reset()
ResetForTest(t)
}

func TestLogLevel(t *testing.T) {
Expand Down
Loading
Loading