From d1bbd8c9059657ca2fed55ed32ae3e910b607eb6 Mon Sep 17 00:00:00 2001 From: dillonstreator Date: Tue, 21 Mar 2023 20:54:59 -0600 Subject: [PATCH 1/2] remove error log pre-allocation and add benchmark --- retry.go | 83 +++++++++++++++++++-------------------------------- retry_test.go | 26 ++++++++++++++++ 2 files changed, 57 insertions(+), 52 deletions(-) diff --git a/retry.go b/retry.go index 13bee07..dccf1ec 100644 --- a/retry.go +++ b/retry.go @@ -114,72 +114,61 @@ func Do(retryableFunc RetryableFunc, opts ...Option) error { return nil } - var errorLog Error - if !config.lastErrorOnly { - errorLog = make(Error, config.attempts) - } else { - errorLog = make(Error, 1) - } + errorLog := Error{} attemptsForError := make(map[error]uint, len(config.attemptsForError)) for err, attempts := range config.attemptsForError { attemptsForError[err] = attempts } - lastErrIndex := n shouldRetry := true for shouldRetry { err := retryableFunc() + if err == nil { + return nil + } - if err != nil { - errorLog[lastErrIndex] = unpackUnrecoverable(err) + errorLog = append(errorLog, unpackUnrecoverable(err)) - if !config.retryIf(err) { - break - } + if !config.retryIf(err) { + break + } - config.onRetry(n, err) + config.onRetry(n, err) - for errToCheck, attempts := range attemptsForError { - if errors.Is(err, errToCheck) { - attempts-- - attemptsForError[errToCheck] = attempts - shouldRetry = shouldRetry && attempts > 0 - } + for errToCheck, attempts := range attemptsForError { + if errors.Is(err, errToCheck) { + attempts-- + attemptsForError[errToCheck] = attempts + shouldRetry = shouldRetry && attempts > 0 } + } - // if this is last attempt - don't wait - if n == config.attempts-1 { - break - } + // if this is last attempt - don't wait + if n == config.attempts-1 { + break + } - select { - case <-config.timer.After(delay(config, n, err)): - case <-config.context.Done(): - if config.lastErrorOnly { - return config.context.Err() - } - n++ - errorLog[n] = config.context.Err() - return errorLog[:lenWithoutNil(errorLog)] + select { + case <-config.timer.After(delay(config, n, err)): + case <-config.context.Done(): + if config.lastErrorOnly { + return config.context.Err() } + n++ - } else { - return nil + return append(errorLog, config.context.Err()) } n++ shouldRetry = shouldRetry && n < config.attempts - - if !config.lastErrorOnly { - lastErrIndex = n - } } if config.lastErrorOnly { - return errorLog[lastErrIndex] + return errorLog.Unwrap() } - return errorLog[:lenWithoutNil(errorLog)] + + return errorLog } func newDefaultRetryConfig() *Config { @@ -203,7 +192,7 @@ type Error []error // Error method return string representation of Error // It is an implementation of error interface func (e Error) Error() string { - logWithNumber := make([]string, lenWithoutNil(e)) + logWithNumber := make([]string, len(e)) for i, l := range e { if l != nil { logWithNumber[i] = fmt.Sprintf("#%d: %s", i+1, l.Error()) @@ -247,17 +236,7 @@ When you need to unwrap all errors, you should use `WrappedErrors()` instead. Added in version 4.2.0. */ func (e Error) Unwrap() error { - return e[lenWithoutNil(e)-1] -} - -func lenWithoutNil(e Error) (count int) { - for _, v := range e { - if v != nil { - count++ - } - } - - return + return e[len(e)-1] } // WrappedErrors returns the list of errors that this Error is wrapping. diff --git a/retry_test.go b/retry_test.go index 57f2a87..9b7b23d 100644 --- a/retry_test.go +++ b/retry_test.go @@ -503,3 +503,29 @@ func TestUnwrap(t *testing.T) { assert.Error(t, err) assert.Equal(t, testError, errors.Unwrap(err)) } + +func BenchmarkDo(b *testing.B) { + testError := errors.New("test error") + + for i := 0; i < b.N; i++ { + Do( + func() error { + return testError + }, + Attempts(10), + Delay(0), + ) + } +} + +func BenchmarkDoNoErrors(b *testing.B) { + for i := 0; i < b.N; i++ { + Do( + func() error { + return nil + }, + Attempts(10), + Delay(0), + ) + } +} From 8f58a1e54b3106398f477397508732e67b0a2c52 Mon Sep 17 00:00:00 2001 From: dillon Date: Sat, 22 Apr 2023 12:09:14 -0500 Subject: [PATCH 2/2] bring EOF newline back --- retry_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/retry_test.go b/retry_test.go index e006c7d..35af7b9 100644 --- a/retry_test.go +++ b/retry_test.go @@ -526,7 +526,6 @@ func TestUnwrap(t *testing.T) { assert.Equal(t, testError, errors.Unwrap(err)) } - func BenchmarkDo(b *testing.B) { testError := errors.New("test error") @@ -562,4 +561,4 @@ func TestIsRecoverable(t *testing.T) { err = fmt.Errorf("wrapping: %w", err) assert.False(t, IsRecoverable(err)) -} \ No newline at end of file +}