Repository: caddyserver/caddy
File: caddyconfig/httploader.go (Go)

Code before:
	if err != nil {
		return nil, err
	}

	result, warnings, err := adaptByContentType(resp.Header.Get("Content-Type"), body)
	if err != nil {
		return nil, err
	}
	for _, warn := range warnings {
		ctx.Logger().Warn(warn.String())
	}

	return result, nil
}

// Reattempts the http call using exponential back off when maxRetries is greater than 0.
func doHttpCallWithRetries(client *http.Client, request *http.Request, maxRetries int) (*http.Response, error) {
	var resp *http.Response
	var err error

	for retry := 0; retry <= maxRetries; retry++ {
		if retry > 0 {
			caddy.Log().Error(err.Error())
			exponentialBackOff := math.Pow(2, float64(retry))
			caddy.Log().Sugar().Infof("reattempting http load in %g seconds (%v retries remain)", exponentialBackOff, maxRetries-retry)
			time.Sleep(time.Duration(exponentialBackOff) * time.Second)
		}
		resp, err = client.Do(request)
		if err != nil {
			err = fmt.Errorf("problem calling http loader url: %v", err)
		} else if resp.StatusCode < 200 || resp.StatusCode > 499 {
			err = fmt.Errorf("bad response status code from http loader url: %v", resp.StatusCode)
		} else {
			return resp, nil
		}
	}

	return resp, err
}

func (hl HTTPLoader) makeClient(ctx caddy.Context) (*http.Client, error) {
	client := &http.Client{
		Timeout: time.Duration(hl.Timeout),
	}

	if hl.TLS != nil {
		var tlsConfig *tls.Config

		// client authentication
		if hl.TLS.UseServerIdentity {
			certs, err := ctx.IdentityCredentials(ctx.Logger())

Review comment (refactor):
Oh, fancy. I wasn't expecting exponential backoff. Props for the effort here.

Some thoughts:

Overall the backoff implementation looks sound. How useful is it? Usually, backoff is implemented for shared or public resources where we need to be careful and mindful clients (and is only useful if most clients implement it, too). For something like config loading, do we expect that backing off will be needed? How many clients are slamming the config source? I'm guessing not a lot (like, not tens-to-hundreds of thousands or more)... so I don't know if it has much utility?

Backoff does also allow the developer more time between retries to fix the problem. A few seconds for the first few retries, and by the time you get to the 20th retry, it's already been a few weeks. That might be an unnecessarily long time between retries.

So one question I have is, how important is having the backoff? And if we do keep it, should we cap the max time between retries to something more reasonable? The longer that span gets, the more time there will be between the backend being available and the actual config being obtained.

If it was just a linear loop (no backoff), the amount of time you spend retrying changes from 2^N seconds 2*N seconds where N is the number of retries (assuming 2 seconds between each one). Or, you could instead have the user configure how _long_ to retry for, and you choose how often to retry. Or, you could have the user configure how long _between each retry_, either forever or with a hard cap on when to give up.

I'm not sure which of those options is preferable here, but what are your thoughts?

Also, we try to avoid ever using `time.Sleep()` because if a config change needs to happen sooner, it will either block or leak resources (continue to run in the background, with no way to stop it). Instead:

```go
select {
    case <-time.After(interval):
    case <-ctx.Done():
        return ctx.Err()
}
```

^ that is the general idea; always select on the Context's Done channel so if it gets canceled, you can stop waiting.

(And even using `time.After()` might not be ideal; the godoc recommends creating a `time.NewTimer()` so it can be stopped and cleaned up if needed. But same idea.)

Code after:
	if err != nil {
		return nil, err
	}
	for _, warn := range warnings {
		ctx.Logger().Warn(warn.String())
	}

	return result, nil
}

func attemptHttpCall(client *http.Client, request *http.Request) (*http.Response, error) {
	resp, err := client.Do(request)
	if err != nil {
		return nil, fmt.Errorf("problem calling http loader url: %v", err)
	} else if resp.StatusCode < 200 || resp.StatusCode > 499 {
		return nil, fmt.Errorf("bad response status code from http loader url: %v", resp.StatusCode)
	}
	return resp, nil
}

func doHttpCallWithRetries(ctx caddy.Context, client *http.Client, request *http.Request) (*http.Response, error) {
	var resp *http.Response
	var err error
	const maxAttempts = 10

	// attempt up to 10 times
	for i := 0; i < maxAttempts; i++ {
		resp, err = attemptHttpCall(client, request)
		if err != nil && i < maxAttempts-1 {
			// wait 500ms before reattempting, or until context is done
			select {
			case <-time.After(time.Millisecond * 500):
			case <-ctx.Done():
				return resp, ctx.Err()
			}
		}
	}

	return resp, err
}

func (hl HTTPLoader) makeClient(ctx caddy.Context) (*http.Client, error) {
	client := &http.Client{
		Timeout: time.Duration(hl.Timeout),
	}

	if hl.TLS != nil {
		var tlsConfig *tls.Config

		// client authentication
		if hl.TLS.UseServerIdentity {