OnRetry terminology is ambiguous about retry-go

[RaJiska]

Hello @JaSei ,

Here is a minimal example as requested.

package main
import (
	"errors"
	"fmt"
	"time"
	"github.com/avast/retry-go"
)
func main() {
	retry.Do(
		func() error {
			fmt.Println("Do")
			return errors.New("")
		},
		retry.OnRetry(func(n uint, err error) {
			fmt.Println("Attempt: ", n, ": Retrying...")
		}),
		retry.Delay(time.Nanosecond),
		retry.Attempts(3),
	)
	fmt.Println("Done")
}

Which outputs:

Do
Attempt:  0 : Retrying...
Do
Attempt:  1 : Retrying...
Do
Attempt:  2 : Retrying...
Done

As you can see with the output, we fall in the OnRetry function even though the max attempt number has been reached and no further attempt of the Do function will be tried. Logically, the OnRetry function should only be called if another loop of Do is ensured.

[JaSei]

Hi @clook OnRetry happening between a failed try.
Look at the code https://github.com/avast/retry-go/blob/master/retry.go#L123.

[clook]

thanks @JaSei
A few lines after, I see this part to avoid waiting after last attempt: https://github.com/avast/retry-go/blob/master/retry.go#L125
So I expect it to happen just after the last attempt (and I can confirm this behavior from our use).

[JaSei]

Sorry @clook, I don't understand well what's wrong?
If you see to this test https://github.com/avast/retry-go/blob/master/retry_test.go#L13 you can see OnRetry function is called each retry. Can you please send your minimal example?

thanks

[clook]

Hi @JaSei,

Thanks for coming back.
I will come with a minimal example asap.
To my mind, what seems wrong to me may be caused by the attempt index starting at 0.
If I take the for loop starting at https://github.com/avast/retry-go/blob/master/retry.go#L126 with a trivial case:

• config.attempts set to 1
• retryableFunc returning systematic error

I expect a single try to fail (and no "retry"), so no call to onRetry callback function.
But the call will indeed occur at https://github.com/avast/retry-go/blob/master/retry.go#L136 at n = 0

Regarding the test here: https://github.com/avast/retry-go/blob/master/retry_test.go#L34
...it is not covering the number of onRetry calls, the sum is 0 + 1 + 2 + ... + 8 + 9, which is 45 for 10 attempts but also 10 calls to onRetry (the first call has no visible effect on the sum so we can think the sum is from 1 to 9).

A more helpful test to double check the number of onRetry would be:

retryCount += 1

and

assert.Equal(t, uint(9), retryCount, "right count of retry")

which I think would fail with the current implementation.

Tell me if it makes things clearer.