From 980f75dfe4c6fd306f70c2176b863974f1c9336a Mon Sep 17 00:00:00 2001 From: Stainless Bot Date: Thu, 14 Sep 2023 22:17:51 +0000 Subject: [PATCH] fix(core): improve retry behavior and related docs --- README.md | 4 +- internal/requestconfig/requestconfig.go | 101 +++++++++++++++--------- 2 files changed, 66 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index 3534df2..806a5fa 100644 --- a/README.md +++ b/README.md @@ -241,8 +241,8 @@ client.Cards.List( ## Retries Certain errors will be automatically retried 2 times by default, with a short exponential backoff. -Connection errors (for example, due to a network connectivity problem), 408 Request Timeout, 409 Conflict, -429 Rate Limit, and >=500 Internal errors will all be retried by default. +We retry by default all connection errors, 408 Request Timeout, 409 Conflict, 429 Rate Limit, +and >=500 Internal errors. You can use the `WithMaxRetries` option to configure or disable this: diff --git a/internal/requestconfig/requestconfig.go b/internal/requestconfig/requestconfig.go index 1cd1472..dae8be1 100644 --- a/internal/requestconfig/requestconfig.go +++ b/internal/requestconfig/requestconfig.go @@ -6,6 +6,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "math" @@ -157,18 +158,59 @@ func applyMiddleware(middleware middleware, next middlewareNext) middlewareNext } } -func (cfg *RequestConfig) Execute() error { - u, err := cfg.BaseURL.Parse(cfg.Request.URL.String()) +func shouldRetry(req *http.Request, res *http.Response) bool { + // If there is no way to recover the Body, then we shouldn't retry. + if req.Body != nil && req.GetBody == nil { + return false + } + + // If there is no response, that indicates that there is a connection error + // so we retry the request. + if res == nil { + return true + } + + // If the header explictly wants a retry behavior, respect that over the + // http status code. + if res.Header.Get("x-should-retry") == "true" { + return true + } + if res.Header.Get("x-should-retry") == "false" { + return false + } + + return res.StatusCode == http.StatusRequestTimeout || + res.StatusCode == http.StatusConflict || + res.StatusCode == http.StatusTooManyRequests || + res.StatusCode >= http.StatusInternalServerError +} + +func retryDelay(res *http.Response, retryCount int) time.Duration { + maxDelay := 5 * time.Second + delay := time.Duration(500 * float64(time.Millisecond) * math.Pow(2, float64(retryCount))) + if res != nil { + if parsed, err := strconv.ParseInt(res.Header.Get("Retry-After"), 10, 64); err == nil { + delay = time.Duration(parsed) * time.Second + } + } + if delay > maxDelay { + delay = maxDelay + } + jitter := rand.Int63n(int64(delay / 4)) + delay -= time.Duration(jitter) + return delay +} + +func (cfg *RequestConfig) Execute() (err error) { + cfg.Request.URL, err = cfg.BaseURL.Parse(cfg.Request.URL.String()) if err != nil { return err } - cfg.Request.URL = u if len(cfg.Buffer) != 0 && cfg.Request.Body == nil { - buf := bytes.NewReader(cfg.Buffer) cfg.Request.ContentLength = int64(len(cfg.Buffer)) - cfg.Request.Body = io.NopCloser(buf) cfg.Request.GetBody = func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(cfg.Buffer)), nil } + cfg.Request.Body, _ = cfg.Request.GetBody() } handler := cfg.HTTPClient.Do @@ -177,60 +219,45 @@ func (cfg *RequestConfig) Execute() error { } var res *http.Response - for i := 0; i <= cfg.MaxRetries; i += 1 { + for retryCount := 0; retryCount <= cfg.MaxRetries; retryCount += 1 { ctx := cfg.Request.Context() if cfg.RequestTimeout != time.Duration(0) { - nctx, cancel := context.WithTimeout(ctx, cfg.RequestTimeout) - ctx = nctx + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, cfg.RequestTimeout) defer cancel() } - req := cfg.Request.Clone(ctx) - res, err = handler(req) - if res == nil { - break - } - - shouldRetry := err != nil || - res.StatusCode == http.StatusRequestTimeout || - res.StatusCode == http.StatusConflict || - res.StatusCode == http.StatusTooManyRequests || - res.StatusCode >= http.StatusInternalServerError - - if res.Header.Get("x-should-retry") == "true" { - shouldRetry = true - } - if res.Header.Get("x-should-retry") == "false" { - shouldRetry = false + res, err = handler(cfg.Request.Clone(ctx)) + if errors.Is(err, context.Canceled) { + return err } - - if !shouldRetry || i >= cfg.MaxRetries { + if !shouldRetry(cfg.Request, res) || retryCount >= cfg.MaxRetries { break } - duration := time.Duration(500) * time.Millisecond * time.Duration(math.Exp(float64(i))) - if res != nil { - if parsed, err := strconv.ParseInt(res.Header.Get("Retry-After"), 10, 64); err == nil { - duration = time.Duration(parsed) * time.Second + // Prepare next request and wait for the retry delay + if cfg.Request.GetBody != nil { + cfg.Request.Body, err = cfg.Request.GetBody() + if err != nil { + return err } } - if duration > time.Duration(60)*time.Second { - duration = time.Duration(60) * time.Second - } - duration += time.Millisecond * time.Duration(-500+rand.Intn(1000)) - time.Sleep(duration) + + time.Sleep(retryDelay(res, retryCount)) } if err != nil { return err } - if res.StatusCode > 399 { + if res.StatusCode >= 400 { aerr := apierror.Error{Request: cfg.Request, Response: res, StatusCode: res.StatusCode} contents, err := io.ReadAll(res.Body) if err != nil { return err } + // If there is an APIError, re-populate the response body so that debugging + // utilities can conveniently dump the response without issue. res.Body = io.NopCloser(bytes.NewBuffer(contents)) err = aerr.UnmarshalJSON(contents) if err != nil {