From 805c3d3336d2b2a60168ab6d5d395435d03f1942 Mon Sep 17 00:00:00 2001 From: Jeevanandam M Date: Fri, 22 Nov 2024 21:48:26 -0800 Subject: [PATCH] feat(enhancement): enhance retry flow - add SetAllowNonIdempotentRetry method - enable multiple reads and seek start io.Reader flow during a retry if its *bytes.Buffer - use struct{} instead of bool - godoc update --- client.go | 33 ++++++++++++++++++++++++++ middleware.go | 21 ++++++++++++++++- request.go | 45 ++++++++++++++++++++++++++++-------- retry_test.go | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 152 insertions(+), 11 deletions(-) diff --git a/client.go b/client.go index 345d59dc..c4db7656 100644 --- a/client.go +++ b/client.go @@ -190,6 +190,7 @@ type Client struct { retryHooks []RetryHookFunc retryStrategy RetryStrategyFunc isRetryDefaultConditions bool + allowNonIdempotentRetry bool headerAuthorizationKey string responseBodyLimit int64 resBodyUnlimitedReads bool @@ -631,6 +632,7 @@ func (c *Client) R() *Request { ResponseBodyUnlimitedReads: c.resBodyUnlimitedReads, AllowMethodGetPayload: c.allowMethodGetPayload, AllowMethodDeletePayload: c.allowMethodDeletePayload, + AllowNonIdempotentRetry: c.allowNonIdempotentRetry, client: c, baseURL: c.baseURL, @@ -1199,6 +1201,12 @@ func (c *Client) RetryCount() int { // first attempt + retry count = total attempts // // See [Request.SetRetryStrategy] +// +// NOTE: +// - By default, Resty only does retry on idempotent HTTP methods, [RFC 9110 Section 9.2.2], [RFC 9110 Section 18.2] +// +// [RFC 9110 Section 9.2.2]: https://datatracker.ietf.org/doc/html/rfc9110.html#name-idempotent-methods +// [RFC 9110 Section 18.2]: https://datatracker.ietf.org/doc/html/rfc9110.html#name-method-registration func (c *Client) SetRetryCount(count int) *Client { c.lock.Lock() defer c.lock.Unlock() @@ -1296,6 +1304,31 @@ func (c *Client) SetRetryDefaultConditions(b bool) *Client { return c } +// AllowNonIdempotentRetry method returns true if the client is enabled to allow +// non-idempotent HTTP methods retry; otherwise, it is `false` +// +// Default value is `false` +func (c *Client) AllowNonIdempotentRetry() bool { + c.lock.RLock() + defer c.lock.RUnlock() + return c.allowNonIdempotentRetry +} + +// SetAllowNonIdempotentRetry method is used to enable/disable non-idempotent HTTP +// methods retry. By default, Resty only allows idempotent HTTP methods, see +// [RFC 9110 Section 9.2.2], [RFC 9110 Section 18.2] +// +// It can be overridden at request level, see [Request.SetAllowNonIdempotentRetry] +// +// [RFC 9110 Section 9.2.2]: https://datatracker.ietf.org/doc/html/rfc9110.html#name-idempotent-methods +// [RFC 9110 Section 18.2]: https://datatracker.ietf.org/doc/html/rfc9110.html#name-method-registration +func (c *Client) SetAllowNonIdempotentRetry(b bool) *Client { + c.lock.Lock() + defer c.lock.Unlock() + c.allowNonIdempotentRetry = b + return c +} + // RetryConditions method returns all the retry condition functions. func (c *Client) RetryConditions() []RetryConditionFunc { c.lock.RLock() diff --git a/middleware.go b/middleware.go index 0bacf275..49bf50ce 100644 --- a/middleware.go +++ b/middleware.go @@ -6,6 +6,7 @@ package resty import ( + "bytes" "fmt" "io" "mime/multipart" @@ -454,9 +455,27 @@ func handleRequestBody(c *Client, r *Request) error { r.bodyBuf = acquireBuffer() switch body := r.Body.(type) { - case io.Reader: // Resty v3 onwards io.Reader used as-is with the request body + case io.Reader: + // Resty v3 onwards io.Reader used as-is with the request body. releaseBuffer(r.bodyBuf) r.bodyBuf = nil + + // enable multiple reads if retry enabled + // and body type is *bytes.Buffer + if r.RetryCount > 0 { + if b, ok := r.Body.(*bytes.Buffer); ok { + v := b.Bytes() + r.Body = bytes.NewReader(v) + } + } + + // do seek start for retry attempt if io.ReadSeeker + // interface supported + if r.Attempt > 1 { + if rs, ok := r.Body.(io.ReadSeeker); ok { + _, _ = rs.Seek(0, io.SeekStart) + } + } return nil case []byte: r.bodyBuf.Write(body) diff --git a/request.go b/request.go index 93b942ed..70f5ce81 100644 --- a/request.go +++ b/request.go @@ -66,6 +66,7 @@ type Request struct { RetryMaxWaitTime time.Duration RetryStrategy RetryStrategyFunc IsRetryDefaultConditions bool + AllowNonIdempotentRetry bool // RetryTraceID provides GUID for retry count > 0 RetryTraceID string @@ -980,6 +981,12 @@ func (r *Request) AddRetryCondition(condition RetryConditionFunc) *Request { // first attempt + retry count = total attempts // // See [Request.SetRetryStrategy] +// +// NOTE: +// - By default, Resty only does retry on idempotent HTTP methods, [RFC 9110 Section 9.2.2], [RFC 9110 Section 18.2] +// +// [RFC 9110 Section 9.2.2]: https://datatracker.ietf.org/doc/html/rfc9110.html#name-idempotent-methods +// [RFC 9110 Section 18.2]: https://datatracker.ietf.org/doc/html/rfc9110.html#name-method-registration func (r *Request) SetRetryCount(count int) *Request { r.RetryCount = count return r @@ -1034,6 +1041,19 @@ func (r *Request) SetRetryDefaultConditions(b bool) *Request { return r } +// SetAllowNonIdempotentRetry method is used to enable/disable non-idempotent HTTP +// methods retry. By default, Resty only allows idempotent HTTP methods, see +// [RFC 9110 Section 9.2.2], [RFC 9110 Section 18.2] +// +// It overrides value set at the client instance level, see [Client.SetAllowNonIdempotentRetry] +// +// [RFC 9110 Section 9.2.2]: https://datatracker.ietf.org/doc/html/rfc9110.html#name-idempotent-methods +// [RFC 9110 Section 18.2]: https://datatracker.ietf.org/doc/html/rfc9110.html#name-method-registration +func (r *Request) SetAllowNonIdempotentRetry(b bool) *Request { + r.AllowNonIdempotentRetry = b + return r +} + //‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾ // HTTP request tracing //_______________________________________________________________________ @@ -1295,7 +1315,7 @@ func (r *Request) Execute(method, url string) (res *Response, err error) { } if backoff != nil { - needsRetry := false + needsRetry, isCtxDone := false, false // apply default retry conditions if r.IsRetryDefaultConditions { @@ -1343,10 +1363,15 @@ func (r *Request) Execute(method, url string) (res *Response, err error) { timer := time.NewTimer(waitDuration) select { case <-r.Context().Done(): + isCtxDone = true timer.Stop() - return nil, wrapErrors(r.Context().Err(), err) + err = wrapErrors(r.Context().Err(), err) + break case <-timer.C: } + if isCtxDone { + break + } } } @@ -1625,18 +1650,18 @@ func (r *Request) resetFileReaders() error { // https://datatracker.ietf.org/doc/html/rfc9110.html#name-idempotent-methods // https://datatracker.ietf.org/doc/html/rfc9110.html#name-method-registration -var idempotentMethods = map[string]bool{ - MethodDelete: true, - MethodGet: true, - MethodHead: true, - MethodOptions: true, - MethodPut: true, - MethodTrace: true, +var idempotentMethods = map[string]struct{}{ + MethodDelete: {}, + MethodGet: {}, + MethodHead: {}, + MethodOptions: {}, + MethodPut: {}, + MethodTrace: {}, } func (r *Request) isIdempotent() bool { _, found := idempotentMethods[r.Method] - return found + return found || r.AllowNonIdempotentRetry } func jsonIndent(v []byte) []byte { diff --git a/retry_test.go b/retry_test.go index db8ad6cd..96448a54 100644 --- a/retry_test.go +++ b/retry_test.go @@ -14,6 +14,7 @@ import ( "fmt" "io" "net/http" + "net/http/httptest" "reflect" "strconv" "strings" @@ -849,6 +850,69 @@ func TestRetryDefaultConditions(t *testing.T) { }) } +func TestRetryRequestPutIoReadSeekerForBuffer(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + b, err := io.ReadAll(r.Body) + assertError(t, err) + assertEqual(t, 12, len(b)) + assertEqual(t, "body content", string(b)) + w.WriteHeader(http.StatusInternalServerError) + })) + + c := dcnl(). + AddRetryCondition( + func(r *Response, err error) bool { + return err != nil || r.StatusCode() > 499 + }, + ). + SetRetryCount(3). + SetAllowNonIdempotentRetry(true) + + assertEqual(t, true, c.AllowNonIdempotentRetry()) + + buf := bytes.NewBuffer([]byte("body content")) + resp, err := c.R(). + SetBody(buf). + SetAllowMethodGetPayload(false). + Put(srv.URL) + + assertNil(t, err) + assertEqual(t, 4, resp.Request.Attempt) + assertEqual(t, http.StatusInternalServerError, resp.StatusCode()) + assertEqual(t, "", resp.String()) +} + +func TestRetryRequestPostIoReadSeeker(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + b, err := io.ReadAll(r.Body) + assertError(t, err) + assertEqual(t, 12, len(b)) + assertEqual(t, "body content", string(b)) + w.WriteHeader(http.StatusInternalServerError) + })) + + c := dcnl(). + AddRetryCondition( + func(r *Response, err error) bool { + return err != nil || r.StatusCode() > 499 + }, + ). + SetRetryCount(3). + SetAllowNonIdempotentRetry(false) + + assertEqual(t, false, c.AllowNonIdempotentRetry()) + + resp, err := c.R(). + SetBody([]byte("body content")). + SetAllowNonIdempotentRetry(true). + Post(srv.URL) + + assertNil(t, err) + assertEqual(t, 4, resp.Request.Attempt) + assertEqual(t, http.StatusInternalServerError, resp.StatusCode()) + assertEqual(t, "", resp.String()) +} + func TestRetryCoverage(t *testing.T) { t.Run("apply retry default min and max value", func(t *testing.T) { backoff := newBackoffWithJitter(0, 0)