Should the caller deal with *retryableError

[Deleplace]

Hello Seth, thanks for the nice facility that I'm trying to use for the first time.

It seems that:

• the Do signature says it returns an error, which is idiomatic (instead of a more specific and error-prone error type);
• there is currently no promise about the exact form of the returned error;
• on success (regardless the number of attempts), Do returns nil;
• on failure, Do ignores the results of the first failed attempts, and returns the error of the very last failed attempt, wrapped in a *retryableError.

If my understanding (the above bullets) is correct, then it seems that it would be even nicer to return the "root cause" error to the caller of Do, i.e. not wrapped in a *retryableError. Shall I try a PR?

Caveat: maybe I'm not familiar enough with errors.As and interface {Unwrap() error}, which means that it would be, in fact, more idiomatic to keep the *retryableError as a visible trace of the retry device, and let the caller unwrap/test the returned error.

[sethvargo]

Hey @Deleplace - thanks for opening an issue.

If my understanding (the above bullets) is correct, then it seems that it would be even nicer to return the "root cause" error to the caller of Do, i.e. not wrapped in a *retryableError. Shall I try a PR?

My thinking (and I'm open to being challenged here) is that the developer has control over whether a particular error should be "retryable" or not (which is why you have to explicitly wrap an error as retryable with retry.RetryableError - normal errors are returned immediately). Thus I assumed the developer would explicitly call errors.Unwrap on the result, if they wanted a specific error.

Now, it does get a little confusing when you have multiple possible returned errors:

retry.Do(ctx, b, func(ctx context.Context) error {
  if err := thing1(); err != nil {
    return err // don't retry
  }

  if err := thing2(); err != nil {
    return retry.RetryableError(err)
  }
})

In this case, I agree it's difficult for the caller to know whether it's "safe" to unwrap the error, since errors from thing1 should not be unwrapped, but errors from thing2 should be unwrapped. Is #6 what you're thinking?

[Deleplace]

Thank you Seth for this quick action, yes #6 is exactly what I was thinking.

My code at call site was doing this safe but boilerplaty check:

	if rerr, ok := err.(interface{ Unwrap() error }); ok {
		// The non-nil error returned by retry.Do (as of 2021-12) may have type *retry.retryableError
		err = rerr.Unwrap()
	}

Indeed for a moment I forgot that my own client code was explicitly doing the wrapping retry.RetryableError(err). It's still nice to not having to do the unwrapping myself from v0.2.0 on.

[Deleplace]

There's something weird going on with the release v0.2.0, I'll open a distinct issue for this: #7

[github-actions]

This issue has been automatically locked since there has not been any
recent activity after it was closed. Please open a new issue for
related bugs.