OnRetry terminology is ambiguous

[clook]

Hi and first thanks for this useful contribution for go 🙏

From the OnRetry terminology, I expect OnRetry to happen between an failed try and the subsequent try. I would not expect it to happen just after the last try.
I would expect the current behavior from a function OnFailure or OnTryFailure (to distinguish from a failure), and I'm by the way basing on retry.Do return to know if the operation has not succeeded finally (ie exceeded).

I would understand if you want to keep the existing behavior to avoid breaking changes but I would at least add a boolean parameter like lastTry or Exceeded to be able to distinguish if a next try is really going to happen.
Without it, we have to use a wrapper or implement this behavior in our calling code, which should be agnostic to the "retry business" to my mind.

I may provide a PR for any of the suggested changes.

Thanks in advance!

[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.

[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.