baseRetryPolicy not closing response

[fantapop]

I noticed in the docs for the CheckRetry function that its responsible for closing the response if the function determines retry should not be made:

go-retryablehttp/client.go

Lines 358 to 361 in 2bcd5fe

	// Client will close any response body when retrying, but if the retry is
	// aborted it is up to the CheckRetry callback to properly close any
	// response body before returning.
	type CheckRetry func(ctx context.Context, resp *http.Response, err error) (bool, error)

I went looking for an example of that in the default implementation but it looks like its not being done?

go-retryablehttp/client.go

Lines 462 to 486 in 2bcd5fe

	func baseRetryPolicy(resp *http.Response, err error) (bool, error) {
	if err != nil {
	if v, ok := err.(*url.Error); ok {
	// Don't retry if the error was due to too many redirects.
	if redirectsErrorRe.MatchString(v.Error()) {
	return false, v
	}
	// Don't retry if the error was due to an invalid protocol scheme.
	if schemeErrorRe.MatchString(v.Error()) {
	return false, v
	}
	// Don't retry if the error was due to TLS cert verification failure.
	if notTrustedErrorRe.MatchString(v.Error()) {
	return false, v
	}
	if _, ok := v.Err.(x509.UnknownAuthorityError); ok {
	return false, v
	}
	}
	// The error is likely recoverable so retry.
	return true, nil
	}

Is this just an oversight?

thanks

[bmendric]

I just noticed this as well -- I actually think that this is an error within the CheckRetry documentation as the originating caller of the request should still be able to introspect the body of their response (e.g. if they want to check some JSON object the server returns that gives more information on the error). So the Client closing the response body on retries makes sense, but for non-retry returns you should ideally NOT close the response body (and further, leave it in an unmodified state).

So, if you are looking to write some custom CheckRetry function that needs to introspect the response body, I believe something like this is the correct way to handle it --

client := retryablehttp.NewClient()
client.CheckRetry = func(ctx context.Context, resp *http.Response, err error) (bool, error) {
  // ... -- initial checks/validation

  defer resp.Body.Close()
  body, rErr := io.ReadAll(resp.Body)
  if rErr != nil {
    return false, rErr
  }
  resp.Body = io.NopCloser(bytes.NewBuffer(body))

  // ... -- using `body` to do w/e
}