From 2c8fcf4fb92b5524a7a4563c8b38ba3f19d963d6 Mon Sep 17 00:00:00 2001 From: slizco Date: Tue, 7 May 2024 09:14:01 -0400 Subject: [PATCH 1/3] Add a SurfaceWorkErrors() opt to the retrier --- retrier/retrier.go | 23 +++++++++++++++++------ retrier/retrier_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/retrier/retrier.go b/retrier/retrier.go index 04a116b..4363933 100644 --- a/retrier/retrier.go +++ b/retrier/retrier.go @@ -11,12 +11,13 @@ import ( // Retrier implements the "retriable" resiliency pattern, abstracting out the process of retrying a failed action // a certain number of times with an optional back-off between each retry. type Retrier struct { - backoff []time.Duration - infiniteRetry bool - class Classifier - jitter float64 - rand *rand.Rand - randMu sync.Mutex + backoff []time.Duration + infiniteRetry bool + surfaceWorkErrors bool + class Classifier + jitter float64 + rand *rand.Rand + randMu sync.Mutex } // New constructs a Retrier with the given backoff pattern and classifier. The length of the backoff pattern @@ -43,6 +44,13 @@ func (r *Retrier) WithInfiniteRetry() *Retrier { return r } +// WithSurfaceWorkErrors configures the retrier to always return the last non-nil error received from work() +// even if a context timeout/deadline is hit. +func (r *Retrier) WithSurfaceWorkErrors() *Retrier { + r.surfaceWorkErrors = true + return r +} + // Run executes the given work function by executing RunCtx without context.Context. func (r *Retrier) Run(work func() error) error { return r.RunFn(context.Background(), func(c context.Context, r int) error { @@ -83,6 +91,9 @@ func (r *Retrier) RunFn(ctx context.Context, work func(ctx context.Context, retr timer := time.NewTimer(r.calcSleep(retries)) if err := r.sleep(ctx, timer); err != nil { + if r.surfaceWorkErrors && ret != nil { + return ret + } return err } diff --git a/retrier/retrier_test.go b/retrier/retrier_test.go index c23407d..f1deec0 100644 --- a/retrier/retrier_test.go +++ b/retrier/retrier_test.go @@ -153,6 +153,44 @@ func TestRetrierRunFnWithInfinite(t *testing.T) { } } +func TestRetrierCtxSurfaceWorkErrors(t *testing.T) { + // Will timeout before getting to errBar. + ctx, cancel := context.WithTimeout(context.Background(), 7*time.Millisecond) + defer cancel() + r := New([]time.Duration{0, 10 * time.Millisecond}, nil).WithSurfaceWorkErrors() + errExpected := []error{errFoo, errFoo, errBar, errBaz} + retries := 0 + err := r.RunCtx(ctx, func(ctx context.Context) error { + if retries >= len(errExpected) { + return nil + } + err := errExpected[retries] + retries++ + return err + }) + if err != errFoo { + t.Error(err) + } +} + +func TestRetrierRunFnWithSurfaceWorkErrors(t *testing.T) { + // Will timeout before getting to errBar. + ctx, cancel := context.WithTimeout(context.Background(), 7*time.Millisecond) + defer cancel() + r := New([]time.Duration{0, 10 * time.Millisecond}, nil).WithSurfaceWorkErrors() + errExpected := []error{errFoo, errFoo, errFoo, errBar, errBaz} + + err := r.RunFn(ctx, func(ctx context.Context, retries int) error { + if retries >= len(errExpected) { + return nil + } + return errExpected[retries] + }) + if err != errFoo { + t.Error(err) + } +} + func TestRetrierNone(t *testing.T) { r := New(nil, nil) From 88f48038a38943d4eca6f787e2d0b850031f7584 Mon Sep 17 00:00:00 2001 From: slizco Date: Thu, 18 Jul 2024 11:39:35 -0400 Subject: [PATCH 2/3] Remove nil-check on last received error --- retrier/retrier.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/retrier/retrier.go b/retrier/retrier.go index 4363933..b51c1a7 100644 --- a/retrier/retrier.go +++ b/retrier/retrier.go @@ -44,7 +44,7 @@ func (r *Retrier) WithInfiniteRetry() *Retrier { return r } -// WithSurfaceWorkErrors configures the retrier to always return the last non-nil error received from work() +// WithSurfaceWorkErrors configures the retrier to always return the last error received from work function // even if a context timeout/deadline is hit. func (r *Retrier) WithSurfaceWorkErrors() *Retrier { r.surfaceWorkErrors = true @@ -91,7 +91,7 @@ func (r *Retrier) RunFn(ctx context.Context, work func(ctx context.Context, retr timer := time.NewTimer(r.calcSleep(retries)) if err := r.sleep(ctx, timer); err != nil { - if r.surfaceWorkErrors && ret != nil { + if r.surfaceWorkErrors { return ret } return err From 029d178001d1830089dcabb720c85124ffde90b8 Mon Sep 17 00:00:00 2001 From: slizco Date: Thu, 18 Jul 2024 12:04:02 -0400 Subject: [PATCH 3/3] Simplify WithSurfaceWorkErrors tests, add inverse behavior test --- retrier/retrier_test.go | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/retrier/retrier_test.go b/retrier/retrier_test.go index f1deec0..b71cd26 100644 --- a/retrier/retrier_test.go +++ b/retrier/retrier_test.go @@ -153,40 +153,48 @@ func TestRetrierRunFnWithInfinite(t *testing.T) { } } -func TestRetrierCtxSurfaceWorkErrors(t *testing.T) { - // Will timeout before getting to errBar. - ctx, cancel := context.WithTimeout(context.Background(), 7*time.Millisecond) +func TestRetrierRunFnWithSurfaceWorkErrors(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) defer cancel() r := New([]time.Duration{0, 10 * time.Millisecond}, nil).WithSurfaceWorkErrors() - errExpected := []error{errFoo, errFoo, errBar, errBaz} - retries := 0 - err := r.RunCtx(ctx, func(ctx context.Context) error { + errExpected := []error{errFoo, errBar, errBaz} + + err := r.RunFn(ctx, func(ctx context.Context, retries int) error { if retries >= len(errExpected) { return nil } + if retries == 1 { + // Context canceled inside second call to work function. + cancel() + } err := errExpected[retries] retries++ return err }) - if err != errFoo { + if err != errBar { t.Error(err) } } -func TestRetrierRunFnWithSurfaceWorkErrors(t *testing.T) { - // Will timeout before getting to errBar. - ctx, cancel := context.WithTimeout(context.Background(), 7*time.Millisecond) +func TestRetrierRunFnWithoutSurfaceWorkErrors(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) defer cancel() - r := New([]time.Duration{0, 10 * time.Millisecond}, nil).WithSurfaceWorkErrors() - errExpected := []error{errFoo, errFoo, errFoo, errBar, errBaz} + r := New([]time.Duration{0, 10 * time.Millisecond}, nil) + errExpected := []error{errFoo, errBar, errBaz} err := r.RunFn(ctx, func(ctx context.Context, retries int) error { if retries >= len(errExpected) { return nil } - return errExpected[retries] + if retries == 1 { + // Context canceled inside second call to work function. + cancel() + } + err := errExpected[retries] + retries++ + return err }) - if err != errFoo { + if err != context.Canceled { t.Error(err) } }