From e16f958491054beac9d5fb81027c3b3744d92978 Mon Sep 17 00:00:00 2001 From: Justen Walker Date: Mon, 9 Aug 2021 13:20:47 -0400 Subject: [PATCH 1/5] support HTTPDate in Retry-After header According to [RFC7231](https://httpwg.org/specs/rfc7231.html#header.retry-after) the Retry-After header may be specified in both number of seconds OR an HTTPDate > The value of this field can be either an HTTP-date or a number of seconds to delay after the response is received. > > Retry-After = HTTP-date / delay-seconds > > A delay-seconds value is a non-negative decimal integer, representing time in seconds. > > delay-seconds = 1*DIGIT > > Two examples of its use are > > Retry-After: Fri, 31 Dec 1999 23:59:59 GMT > Retry-After: 120 This change adds support for parsing the date if the format of the header contains non-numeric characters --- client.go | 47 +++++++++++++++++++++++++++++++++++++++++++---- client_test.go | 28 ++++++++++++++++++++++++---- 2 files changed, 67 insertions(+), 8 deletions(-) diff --git a/client.go b/client.go index adbdd92..457f930 100644 --- a/client.go +++ b/client.go @@ -69,6 +69,14 @@ var ( // scheme specified in the URL is invalid. This error isn't typed // specifically so we resort to matching on the error string. schemeErrorRe = regexp.MustCompile(`unsupported protocol scheme`) + + // A regular expression to match the number of seconds to delay + // when parsing the Retry-After header + retryAfterSecondsRe = regexp.MustCompile(`^[0-9]+$`) + + // timeNow sets the function that returns the current time. + // This defaults to time.Now. Changes to this should only be done in tests. + timeNow = time.Now ) // ReaderFunc is the type of function that can be given natively to NewRequest @@ -472,10 +480,8 @@ func baseRetryPolicy(resp *http.Response, err error) (bool, error) { func DefaultBackoff(min, max time.Duration, attemptNum int, resp *http.Response) time.Duration { if resp != nil { if resp.StatusCode == http.StatusTooManyRequests || resp.StatusCode == http.StatusServiceUnavailable { - if s, ok := resp.Header["Retry-After"]; ok { - if sleep, err := strconv.ParseInt(s[0], 10, 64); err == nil { - return time.Second * time.Duration(sleep) - } + if sleep, ok := parseRetryAfterHeader(resp.Header["Retry-After"]); ok { + return sleep } } } @@ -488,6 +494,39 @@ func DefaultBackoff(min, max time.Duration, attemptNum int, resp *http.Response) return sleep } +// parseRetryAfterHeader parses the Retry-After header and returns the +// delay duration according to the spec: https://httpwg.org/specs/rfc7231.html#header.retry-after +// +// Retry-After headers come in two flavors: Seconds or HTTP-Date +// +// Examples: +// * Retry-After: Fri, 31 Dec 1999 23:59:59 GMT +// * Retry-After: 120 +func parseRetryAfterHeader(headers []string) (time.Duration, bool) { + if len(headers) == 0 || headers[0] == "" { + return 0, false + } + header := headers[0] + // Retry-After: 120 + if retryAfterSecondsRe.MatchString(header) { + if sleep, err := strconv.ParseInt(header, 10, 64); err == nil { + return time.Second * time.Duration(sleep), true + } + return 0, false + } + + // Retry-After: Mon, 02 Jan 2006 15:04:05 MST + retryTime, err := time.Parse(time.RFC1123, header) + if err != nil { + return 0, false + } + if until := retryTime.Sub(timeNow()); until > 0 { + return until, true + } + // date is in the past + return 0, true +} + // LinearJitterBackoff provides a callback for Client.Backoff which will // perform linear backoff based on the attempt number and with jitter to // prevent a thundering herd. diff --git a/client_test.go b/client_test.go index 082b407..401b5ea 100644 --- a/client_test.go +++ b/client_test.go @@ -524,11 +524,31 @@ func TestClient_CheckRetry(t *testing.T) { } func TestClient_DefaultBackoff(t *testing.T) { - for _, code := range []int{http.StatusTooManyRequests, http.StatusServiceUnavailable} { - t.Run(fmt.Sprintf("http_%d", code), func(t *testing.T) { + timeNow = func() time.Time { + now, err := time.Parse(time.RFC1123, "Fri, 31 Dec 1999 23:59:57 GMT") + if err != nil { + panic(err) + } + return now + } + defer func() { + timeNow = time.Now + }() + tests := []struct { + name string + code int + retryHeader string + }{ + {"http_429_seconds", http.StatusTooManyRequests, "2"}, + {"http_429_date", http.StatusTooManyRequests, "Fri, 31 Dec 1999 23:59:59 GMT"}, + {"http_503_seconds", http.StatusServiceUnavailable, "2"}, + {"http_503_date", http.StatusTooManyRequests, "Fri, 31 Dec 1999 23:59:59 GMT"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Retry-After", "2") - http.Error(w, fmt.Sprintf("test_%d_body", code), code) + w.Header().Set("Retry-After", test.retryHeader) + http.Error(w, fmt.Sprintf("test_%d_body", test.code), test.code) })) defer ts.Close() From 0dc51cc2ee4ea10708cb9dad85141c7c3a83c37d Mon Sep 17 00:00:00 2001 From: Justen Walker Date: Fri, 13 Aug 2021 11:40:56 -0400 Subject: [PATCH 2/5] tests: fix DefaultBackoff case --- client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client_test.go b/client_test.go index 401b5ea..063f258 100644 --- a/client_test.go +++ b/client_test.go @@ -542,7 +542,7 @@ func TestClient_DefaultBackoff(t *testing.T) { {"http_429_seconds", http.StatusTooManyRequests, "2"}, {"http_429_date", http.StatusTooManyRequests, "Fri, 31 Dec 1999 23:59:59 GMT"}, {"http_503_seconds", http.StatusServiceUnavailable, "2"}, - {"http_503_date", http.StatusTooManyRequests, "Fri, 31 Dec 1999 23:59:59 GMT"}, + {"http_503_date", http.StatusServiceUnavailable, "Fri, 31 Dec 1999 23:59:59 GMT"}, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { From 0ed3cd7540edad37b50599779c656c6340ffc7a3 Mon Sep 17 00:00:00 2001 From: Justen Walker Date: Fri, 13 Aug 2021 11:41:37 -0400 Subject: [PATCH 3/5] tests: helper to set static time.Now --- client_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/client_test.go b/client_test.go index 063f258..e14e606 100644 --- a/client_test.go +++ b/client_test.go @@ -523,7 +523,7 @@ func TestClient_CheckRetry(t *testing.T) { } } -func TestClient_DefaultBackoff(t *testing.T) { +func testStaticTime(t *testing.T) { timeNow = func() time.Time { now, err := time.Parse(time.RFC1123, "Fri, 31 Dec 1999 23:59:57 GMT") if err != nil { @@ -531,9 +531,13 @@ func TestClient_DefaultBackoff(t *testing.T) { } return now } - defer func() { + t.Cleanup(func() { timeNow = time.Now - }() + }) +} + +func TestClient_DefaultBackoff(t *testing.T) { + testStaticTime(t) tests := []struct { name string code int From 012f144018e17693cf92c653a60f7060f262e20d Mon Sep 17 00:00:00 2001 From: Justen Walker Date: Fri, 13 Aug 2021 11:42:17 -0400 Subject: [PATCH 4/5] tests: add test cases for parseRetryAfterHeader --- client_test.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/client_test.go b/client_test.go index e14e606..9fdb758 100644 --- a/client_test.go +++ b/client_test.go @@ -536,6 +536,37 @@ func testStaticTime(t *testing.T) { }) } +func TestParseRetryAfterHeader(t *testing.T) { + testStaticTime(t) + tests := []struct { + name string + headers []string + sleep time.Duration + ok bool + }{ + {"seconds", []string{"2"}, time.Second * 2, true}, + {"date", []string{"Fri, 31 Dec 1999 23:59:59 GMT"}, time.Second * 2, true}, + {"past-date", []string{"Fri, 31 Dec 1999 23:59:00 GMT"}, 0, true}, + {"nil", nil, 0, false}, + {"two-headers", []string{"2", "3"}, time.Second * 2, true}, + {"empty", []string{""}, 0, false}, + {"negative", []string{"-2"}, 0, false}, + {"bad-date", []string{"Fri, 32 Dec 1999 23:59:59 GMT"}, 0, false}, + {"bad-date-format", []string{"badbadbad"}, 0, false}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + sleep, ok := parseRetryAfterHeader(test.headers) + if ok != test.ok { + t.Fatalf("expected ok=%t, got ok=%t", test.ok, ok) + } + if sleep != test.sleep { + t.Fatalf("expected sleep=%v, got sleep=%v", test.sleep, sleep) + } + }) + } +} + func TestClient_DefaultBackoff(t *testing.T) { testStaticTime(t) tests := []struct { From f2396f056078c3077dd837617ce8ba7ca7c4413d Mon Sep 17 00:00:00 2001 From: Justen Walker Date: Fri, 13 Aug 2021 11:42:47 -0400 Subject: [PATCH 5/5] remove regexp for parseRetryAfterHeader Removes dependency on regular expressions for detecting numeric header values vs date values --- client.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/client.go b/client.go index 457f930..3b104be 100644 --- a/client.go +++ b/client.go @@ -70,10 +70,6 @@ var ( // specifically so we resort to matching on the error string. schemeErrorRe = regexp.MustCompile(`unsupported protocol scheme`) - // A regular expression to match the number of seconds to delay - // when parsing the Retry-After header - retryAfterSecondsRe = regexp.MustCompile(`^[0-9]+$`) - // timeNow sets the function that returns the current time. // This defaults to time.Now. Changes to this should only be done in tests. timeNow = time.Now @@ -496,6 +492,8 @@ func DefaultBackoff(min, max time.Duration, attemptNum int, resp *http.Response) // parseRetryAfterHeader parses the Retry-After header and returns the // delay duration according to the spec: https://httpwg.org/specs/rfc7231.html#header.retry-after +// The bool returned will be true if the header was successfully parsed. +// Otherwise, the header was either not present, or was not parseable according to the spec. // // Retry-After headers come in two flavors: Seconds or HTTP-Date // @@ -508,14 +506,14 @@ func parseRetryAfterHeader(headers []string) (time.Duration, bool) { } header := headers[0] // Retry-After: 120 - if retryAfterSecondsRe.MatchString(header) { - if sleep, err := strconv.ParseInt(header, 10, 64); err == nil { - return time.Second * time.Duration(sleep), true + if sleep, err := strconv.ParseInt(header, 10, 64); err == nil { + if sleep < 0 { // a negative sleep doesn't make sense + return 0, false } - return 0, false + return time.Second * time.Duration(sleep), true } - // Retry-After: Mon, 02 Jan 2006 15:04:05 MST + // Retry-After: Fri, 31 Dec 1999 23:59:59 GMT retryTime, err := time.Parse(time.RFC1123, header) if err != nil { return 0, false