This was added in DefaultPolicy and DefaultBackoff. The latter will examine the Retry-After response header (if existant) and do a backoff for the specified amount of time. Otherwise it will default to default backoff behaviour.

This addresses Issue #99.

Summary

• Allow RequestLogHook and ResponseLogHook to be accessed even if the client.Logger is nil
• Add tests for ^

Use Case

We're using a logging package that doesn't conform to Logger interface, but still want access to values available through LogHooks. Specifically, we're using RequestLogHook to access the retry count and pass it to our own internal structured JSON logging package.

However, setting Client.Logger = nil (in order to suppress logging to STDOUT) skips the block of code where RequestLogHook accesses the retry count (here). The presence of the default case in the switch here makes me think that this behavior (suppressed logging, but still able to access values in LogHooks) was meant to be supported.

Considered Alternatives

The alternative of disabling the logger with a noop logger or other similar approach has a small performance cost (see here, tho the benchmarks are probably out of date, the idea that it will cost something to have a noop logger is what is most important)

rand.NewSource doc clearly says: Unlike the default Source used by top-level functions, this source is not safe for concurrent use by multiple goroutines.

This fixes #22 in a different way to the current proposed fix (#21):

• Do() can now take any http.Request as input. If the request body is not seekable then the contents of the request are read to a buffer, and bytes.Reader is used, which is seekable.
  • but if the request body is already seekable then the copy is not made
  • and in my experience almost all request bodies are already in-memory buffers created by marshaling json / xml / text / etc, so I don't think this is a big loss
  • and realistically, if the user passes in a request body that was not seekable, then under the old API the user just would have done the in-memory copy trick anyway
  • so it really does not seem like we're losing a lot here
• drops the Request wrapper struct (converts it to a type alias for http.Request for backwards compatbility)
• the NewRequest function is now just a direct passthrough to http.newRequest

One of the nice things about this approach is that it allows users of this library to define interfaces that can match both http.Client and retryablehttp.Client, so e.g. that retryable HTTP requests can be dependency-injected into applications:

type MyHttpClient interface {
  Do(req *http.Request) (*http.Response, error)
}

func doSomething(client MyHttpClient) { ... }

func main() {
  if (shouldBeRetryable) {
    doSomething(&retryablehttp.Client{...})
  } else {
   doSomething(&http.Client{...})
  }
}

When you're using a http.Client with a custom RoundTripper that actually facilitates the retries (it maintains its own internal http.Client object); any errors returned from the original http.Client are re-wrapped.

For instance, trying to do a Get request that returns an error response, and using the PassthroughErrorHandler to return the real failure would result in an error like this:

Get "http://this-url-does-not-exist.com/": Get "http://this-url-does-not-exist.com/": dial tcp: lookup this-url-does-not-exist.com: no such host

This is basically an *url.Error object nested within another *url.Error's Err field.

We can just .Unwrap() it to make sure that we only get what we need. Open to other suggestions though.

Hi, thanks for developing such wonderful library. I was trying to use it in my project and I have a question on below line.

https://github.com/hashicorp/go-retryablehttp/blob/4af2e4b1970397d927d9e8d74c97e8659c0350c3/client.go#L575-L581

May I know why need to do c.HTTPClient.CloseIdleConnections() when do not need retry? I found this will send a FIN to server side and start to close connection. But In my understanding, the connection can be still reused. Actually in my project, I found the connection number was keep growing after my service starts and accepts API requests. Then I removed this line in my project, the function is working and connections stop to keep growing.

As a followup to: https://github.com/hashicorp/go-retryablehttp/pull/57#pullrequestreview-234625048

If I'm reading this correctly, the library currently seems to close all happy-case connections, which makes it unsuitable for use in a micro-service architecture where many requests go to the same few hosts. I'm currently observing this behavior in one of my services. This is not just a connection-time issue, but in my case the closed requests eat up available sockets in TIME_WAIT state to the point of machine failure.

@jefferai Could you please revisit this review request above with this consideration in mind? I'm happy to PR this is you concur it's an issue.

This change makes it easier to use a different log implementation and use the log level from these loggers. I left out other additional ones (Infof, etc) since they are not used here

what was done

• Changing the hclog.Logger interface to a smaller one that will be easier to get implemented from other logger libraries. As we are just reducing the required methods on the interface this will not break the API.

alternative:

As proposed on #74 we can also break the API by changing the existing Logger interface.

Closes #74

Add propagation of the HTTP request/response error up the call stack when the number of retries are exceeded. Because the ErrorHandler does not receive a copy of the request, it's impossible to use the ErrorHandler to generate an enhanced version of the default Retries Exceeded error message.

This change takes the non-API breaking approach to solving this, by including the error string in the returned error (if an error has occured).

Fixes #69

This reverts #33, fixing the data race, and removes the excess allocations by using the global random source which is safe for concurrent use.

An alternative PR to #35

Change ERR to ERROR for consistency in our libraries.

To Do:

• Find out context for the different loggers. When running a terraform apply I saw the following log output from this library:

2022-12-23T18:19:35.431-0700 [INFO]  CLI command args: []string{"apply"}
2022/12/23 18:19:35 [DEBUG] GET https://app.terraform.io/.well-known/terraform.json

The DEBUG prints even if TF_LOG is set to info (which I did in this case). Also would be nice to have consistent date/time stamps.

Certificate retry inhibiting would be broken in Go 1.20.

The upstream CL http://go.dev/cl/449336 changed the error type returned when there's a certificate verification failure. The following line no longer hits:

https://github.com/hashicorp/go-retryablehttp/blob/493aa4cf372e47ec00228558d9d91a9517b045a5/client.go#L476-L478

I believe it would resume working (and also work for older Go versions) if this was changed to:

			// Don't retry if the error was due to TLS cert verification failure.
-			if _, ok := v.Err.(x509.UnknownAuthorityError); ok {
+			if _, ok := errors.As(v.Err, &x509.UnknownAuthorityError{}); ok {
				return false, nil
			}

In addition to 501, these errors seem permanent as well:

http.StatusHTTPVersionNotSupported       // 505 HTTP Version Not Supported
http.StatusVariantAlsoNegotiates         // 506 Variant Also Negotiates
http.StatusNotExtended                   // 510 Not Extended
http.StatusNetworkAuthenticationRequired // 511 Network Authentication Required

Perhaps we should skip the retry logic when we see one of them?

PS: I'm not sure about http.StatusLoopDetected (508 Loop Detected). Maybe we should include that as well.

Hi there 👋

This PR was auto-generated as part of an internal review of public repositories that are not in compliance with HashiCorp's licensing standards.

Frequently Asked Questions

Please approve and merge this PR in a timely manner to keep this source code compliant with our OSS license agreement. If you have any questions or feedback, reach out to #proj-software-copyright.

Thank you!

Made with :heart: @HashiCorp

For lines

https://github.com/hashicorp/go-retryablehttp/blob/493aa4cf372e47ec00228558d9d91a9517b045a5/client.go#L589

and

https://github.com/hashicorp/go-retryablehttp/blob/493aa4cf372e47ec00228558d9d91a9517b045a5/client.go#L646

and

https://github.com/hashicorp/go-retryablehttp/blob/493aa4cf372e47ec00228558d9d91a9517b045a5/client.go#L690

CodeQL analysis is throwing following error

Log entry depends on a user-provided value.