From 95439df23bb38096baf56eb4817080292ffab383 Mon Sep 17 00:00:00 2001 From: Marco Pracucci Date: Wed, 3 Jul 2024 09:45:46 +0200 Subject: [PATCH 1/3] Backoff: change Err() to return the context cause when available Signed-off-by: Marco Pracucci --- backoff/backoff.go | 6 ++-- backoff/backoff_test.go | 62 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/backoff/backoff.go b/backoff/backoff.go index 7ce556472..5468929bc 100644 --- a/backoff/backoff.go +++ b/backoff/backoff.go @@ -54,10 +54,12 @@ func (b *Backoff) Ongoing() bool { return b.ctx.Err() == nil && (b.cfg.MaxRetries == 0 || b.numRetries < b.cfg.MaxRetries) } -// Err returns the reason for terminating the backoff, or nil if it didn't terminate +// Err returns the reason for terminating the backoff, or nil if it didn't terminate. +// If backoff is terminated because the context has been canceled, then this function +// returns the context cancellation cause. func (b *Backoff) Err() error { if b.ctx.Err() != nil { - return b.ctx.Err() + return context.Cause(b.ctx) } if b.cfg.MaxRetries != 0 && b.numRetries >= b.cfg.MaxRetries { return fmt.Errorf("terminated after %d retries", b.numRetries) diff --git a/backoff/backoff_test.go b/backoff/backoff_test.go index dff6432c0..09379908d 100644 --- a/backoff/backoff_test.go +++ b/backoff/backoff_test.go @@ -4,6 +4,9 @@ import ( "context" "testing" "time" + + "github.com/pkg/errors" + "github.com/stretchr/testify/require" ) func TestBackoff_NextDelay(t *testing.T) { @@ -101,3 +104,62 @@ func TestBackoff_NextDelay(t *testing.T) { }) } } + +func TestBackoff_Err(t *testing.T) { + cause := errors.New("my cause") + + tests := map[string]struct { + ctx func(*testing.T) context.Context + expectedErr error + }{ + "should return context.DeadlineExceeded when context deadline exceeded without cause": { + ctx: func(t *testing.T) context.Context { + ctx, cancel := context.WithDeadline(context.Background(), time.Now()) + t.Cleanup(cancel) + + return ctx + }, + expectedErr: context.DeadlineExceeded, + }, + "should return cause when context deadline exceeded with cause": { + ctx: func(t *testing.T) context.Context { + ctx, cancel := context.WithDeadlineCause(context.Background(), time.Now(), cause) + t.Cleanup(cancel) + + return ctx + }, + expectedErr: cause, + }, + "should return context.Canceled when context is canceled without cause": { + ctx: func(_ *testing.T) context.Context { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + return ctx + }, + expectedErr: context.Canceled, + }, + "should return cause when context is canceled with cause": { + ctx: func(_ *testing.T) context.Context { + ctx, cancel := context.WithCancelCause(context.Background()) + cancel(cause) + + return ctx + }, + expectedErr: cause, + }, + } + + for testName, testData := range tests { + t.Run(testName, func(t *testing.T) { + b := New(testData.ctx(t), Config{}) + + // Wait until the backoff returns error. + require.Eventually(t, func() bool { + return b.Err() != nil + }, time.Second, 10*time.Millisecond) + + require.Equal(t, testData.expectedErr, b.Err()) + }) + } +} From 2ee254931638da3a9347e6b73bf233fddb725efb Mon Sep 17 00:00:00 2001 From: Marco Pracucci Date: Wed, 3 Jul 2024 18:44:18 +0200 Subject: [PATCH 2/3] Add Backoff.ErrCause() instead Signed-off-by: Marco Pracucci --- CHANGELOG.md | 1 + backoff/backoff.go | 13 ++++++++++--- backoff/backoff_test.go | 26 ++++++++++++++++---------- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index daff63f43..fbdb40643 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -214,6 +214,7 @@ * [ENHANCEMENT] SpanProfiler: do less work on unsampled traces. #528 * [ENHANCEMENT] Log Middleware: if the trace is not sampled, log its ID as `trace_id_unsampled` instead of `trace_id`. #529 * [EHNANCEMENT] httpgrpc: httpgrpc Server can now use error message from special HTTP header when converting HTTP response to an error. This is useful when HTTP response body contains binary data that doesn't form valid utf-8 string, otherwise grpc would fail to marshal returned error. #531 +* [CHANGE] Backoff: added `Backoff.ErrCause()` which is like `Backoff.Err()` but returns the context cause if backoff is terminated because the context has been canceled. #538 * [BUGFIX] spanlogger: Support multiple tenant IDs. #59 * [BUGFIX] Memberlist: fixed corrupted packets when sending compound messages with more than 255 messages or messages bigger than 64KB. #85 * [BUGFIX] Ring: `ring_member_ownership_percent` and `ring_tokens_owned` metrics are not updated on scale down. #109 diff --git a/backoff/backoff.go b/backoff/backoff.go index 5468929bc..419af80e1 100644 --- a/backoff/backoff.go +++ b/backoff/backoff.go @@ -55,11 +55,9 @@ func (b *Backoff) Ongoing() bool { } // Err returns the reason for terminating the backoff, or nil if it didn't terminate. -// If backoff is terminated because the context has been canceled, then this function -// returns the context cancellation cause. func (b *Backoff) Err() error { if b.ctx.Err() != nil { - return context.Cause(b.ctx) + return b.ctx.Err() } if b.cfg.MaxRetries != 0 && b.numRetries >= b.cfg.MaxRetries { return fmt.Errorf("terminated after %d retries", b.numRetries) @@ -67,6 +65,15 @@ func (b *Backoff) Err() error { return nil } +// ErrCause is like Err() but returns the context cause if backoff is terminated because the +// context has been canceled. +func (b *Backoff) ErrCause() error { + if b.ctx.Err() != nil { + return context.Cause(b.ctx) + } + return b.Err() +} + // NumRetries returns the number of retries so far func (b *Backoff) NumRetries() int { return b.numRetries diff --git a/backoff/backoff_test.go b/backoff/backoff_test.go index 09379908d..b9caaf2fc 100644 --- a/backoff/backoff_test.go +++ b/backoff/backoff_test.go @@ -109,44 +109,49 @@ func TestBackoff_Err(t *testing.T) { cause := errors.New("my cause") tests := map[string]struct { - ctx func(*testing.T) context.Context - expectedErr error + ctx func(*testing.T) context.Context + expectedErr error + expectedErrCause error }{ - "should return context.DeadlineExceeded when context deadline exceeded without cause": { + "context deadline exceeded without cause": { ctx: func(t *testing.T) context.Context { ctx, cancel := context.WithDeadline(context.Background(), time.Now()) t.Cleanup(cancel) return ctx }, - expectedErr: context.DeadlineExceeded, + expectedErr: context.DeadlineExceeded, + expectedErrCause: context.DeadlineExceeded, }, - "should return cause when context deadline exceeded with cause": { + "context deadline exceeded with cause": { ctx: func(t *testing.T) context.Context { ctx, cancel := context.WithDeadlineCause(context.Background(), time.Now(), cause) t.Cleanup(cancel) return ctx }, - expectedErr: cause, + expectedErr: context.DeadlineExceeded, + expectedErrCause: cause, }, - "should return context.Canceled when context is canceled without cause": { + "context is canceled without cause": { ctx: func(_ *testing.T) context.Context { ctx, cancel := context.WithCancel(context.Background()) cancel() return ctx }, - expectedErr: context.Canceled, + expectedErr: context.Canceled, + expectedErrCause: context.Canceled, }, - "should return cause when context is canceled with cause": { + "context is canceled with cause": { ctx: func(_ *testing.T) context.Context { ctx, cancel := context.WithCancelCause(context.Background()) cancel(cause) return ctx }, - expectedErr: cause, + expectedErr: context.Canceled, + expectedErrCause: cause, }, } @@ -160,6 +165,7 @@ func TestBackoff_Err(t *testing.T) { }, time.Second, 10*time.Millisecond) require.Equal(t, testData.expectedErr, b.Err()) + require.Equal(t, testData.expectedErrCause, b.ErrCause()) }) } } From 1d033d605c89628461ed7fc15f2ba9c9ef400873 Mon Sep 17 00:00:00 2001 From: Marco Pracucci Date: Thu, 4 Jul 2024 10:09:52 +0200 Subject: [PATCH 3/3] Renamed unit test Signed-off-by: Marco Pracucci --- backoff/backoff_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backoff/backoff_test.go b/backoff/backoff_test.go index b9caaf2fc..586121415 100644 --- a/backoff/backoff_test.go +++ b/backoff/backoff_test.go @@ -105,7 +105,7 @@ func TestBackoff_NextDelay(t *testing.T) { } } -func TestBackoff_Err(t *testing.T) { +func TestBackoff_ErrAndErrCause(t *testing.T) { cause := errors.New("my cause") tests := map[string]struct {