Return unwrapping error when executing Do func

[zacscoding]

Changed returned error retryableError to unwrapping error in retry.Do().

In my case, i use retry.Do() func like below to get origin error.

err := retry.Do(context.Background(), b, func(ctx context.Context) error {
  // my logic
})
if err != nil {
  if err2 := errors.Unwrap(err); err2 != nil {
    err = err2
  }
}

I think it is also helpful if u provide unwrapping errors in retry packages :)

[sethvargo]

I think the proper use here would be:

var targetError *MyCustomError
err := retry.Do(...)
if err != nil {
  if errors.As(err, &targetError) {
    // use custom error
  }
  return fmt.Errorf("unknown error: %w", err)
}

[zacscoding]

@sethvargo thx for reply :)
I think that is proper use if i know target error.
But what is proper use if i don't know target error?

[sethvargo]

If you don't know the error target, what's the point in unwrapping?

[zacscoding]

I just want to return origin error :)

For example, define http client's wrapping function for retry like below.
In this case, i just want to return a fasthttp's error, and hide retry package.

func (c *HttpClient) Get(ctx context.Context, url string, result interface{}) error {
	var (
		statusCode int
		body       []byte
	)
	backoff := c.NewBackoff()
	if err := retry.Do(ctx, backoff, func(ctx context.Context) error {
		var err error
		statusCode, body, err = fasthttp.Get(nil, url)
		if err != nil {
			// we don't know target error
			return retry.RetryableError(err)
		}
		return nil
	}); err != nil {
		if err2 := errors.Unwrap(err); err2 != nil {
			return err2
		}
		return err
	}
	// extract result
}

[sethvargo]

You have no guarantee that the error you're unwrapping is a fasthttp error though.

[loicalbertin]

Hi @sethvargo & @zacscoding

As @zacscoding I dislike to return to my caller an error that may or not be a retryable error. I mean that this caller should not know about my internal implementation details.
So what about adding a IsRetryableError(err error) bool function that check if the error returned by Do is retryable or not and let me decide if I want to unwrap it?

[sethvargo]

If you have a particular type of error you want to return to the caller, you should use errors.Is and errors.As to unwrap it.

I think you may be mis-using Go errors. Sentinel errors are almost always a bad idea, also this.

[loicalbertin]

Well I agree that sentinel errors are to be avoided. I would prefer @zacscoding solution.

Anyway, to clarify my point is not about returning a particular error but masking the retry.retryableError.

In your example:

// This example demonstrates selectively retrying specific errors. Only errors
// wrapped with RetryableError are eligible to be retried.
if err := retry.Do(ctx, retry.WithMaxRetries(3, b), func(ctx context.Context) error {
	resp, err := http.Get("https://google.com/")
	if err != nil {
		return err
	}
	defer resp.Body.Close()

	switch resp.StatusCode / 100 {
	case 4:
		return fmt.Errorf("bad response: %v", resp.StatusCode)
	case 5:
		return retry.RetryableError(fmt.Errorf("bad response: %v", resp.StatusCode))
	default:
		return nil
	}
}); err != nil {
	// handle error
}

In this example Do() may returns an error from http.Get(), an error created with fmt.Errorf() or a retry.retryableError.
If in // handle error I simply return it, my caller will sometime see errors retryable: bad response: xxx I think knowing that an error is retryable or not is not interesting and may even be confusing. I don't want my caller to be able make decisions based on that. I don't get the added value of returning an retry.retryableError.

But maybe I indeed do not understand how I should properly handle errors returned by this package.

[sethvargo]

IFF you want to use sentinel errors, you need to use them throughout. Here's an example of how I would do the HTTP example above if I was required to use sentinel errors:

type StatusError struct {
  code int
  msg string
}

func (e *StatusError) Error() string {
  return fmt.Sprintf("%d - %s", e.code, e.msg)
}

func TODO() {
  // This example demonstrates selectively retrying specific errors. Only errors
  // wrapped with RetryableError are eligible to be retried.
  if err := retry.Do(ctx, retry.WithMaxRetries(3, b), func(ctx context.Context) error {
    resp, err := http.Get("https://google.com/")
    if err != nil {
      return err
    }
    defer resp.Body.Close()

    switch resp.StatusCode / 100 {
    case 4:
      return &StatusError{resp.StatusCode, "user error"}
    case 5:
      return &StatusError{resp.StatusCode, "server error"}
    default:
      return nil
    }
  }); err != nil {
    var e *StatusError
    if errors.As(err, &e) {
      return e // e is *StatusError
    }
    return err
  }
}

[loicalbertin]

Thanks for your reply I do not particularly want to use sentinel errors (It was just another proposal as you seems to disagree with @zacscoding PR).

If we get back to your original example.

// This example demonstrates selectively retrying specific errors. Only errors
// wrapped with RetryableError are eligible to be retried.
if err := retry.Do(ctx, retry.WithMaxRetries(3, b), func(ctx context.Context) error {
	resp, err := http.Get("https://google.com/")
	if err != nil {
		return err
	}
	defer resp.Body.Close()

	switch resp.StatusCode / 100 {
	case 4:
		return fmt.Errorf("bad response: %v", resp.StatusCode)
	case 5:
		return retry.RetryableError(fmt.Errorf("bad response: %v", resp.StatusCode))
	default:
		return nil
	}
}); err != nil {
	// handle error
}

How should a handle error to not expose to my callers that an error is or is not retryable?

[sethvargo]

If you or your callers care about the specific error that's returned (sentinel errors), you have to call all-in. That means you wrap all the errors that you return and then unwrap them before passing them onto the caller.

[loicalbertin]

OK thanks, and just to understand how to properly use this lib, what is the benefit of returning a retry.retryableError in Do() function?

[sethvargo]

It tells the function that the error is retryable. In the example above, only the 500-level errors are retried. The others are immediately returned and execution stops.

[github-actions]

This Pull Request is stale because it has been open for 14 days with
no activity. It will automatically close after 7 more days of
inactivity.

[zacscoding]

@loicalbertin thx for additional opinion :)

@sethvargo thx for proper usage of errors :) I learned many things from reference and comments!
please close this pr :)

[github-actions]

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