From 0873a5d510638cecaf0340151ff6eadfdc60024e Mon Sep 17 00:00:00 2001 From: Joel Hendrix Date: Fri, 8 Dec 2023 13:27:48 -0800 Subject: [PATCH 1/3] Add more retry after headers to check during retries Include the headers that provide sub-second granularity. --- sdk/azcore/CHANGELOG.md | 2 + sdk/azcore/internal/shared/constants.go | 2 + sdk/azcore/internal/shared/shared.go | 59 +++++++++++++++++++---- sdk/azcore/internal/shared/shared_test.go | 16 ++++++ 4 files changed, 69 insertions(+), 10 deletions(-) diff --git a/sdk/azcore/CHANGELOG.md b/sdk/azcore/CHANGELOG.md index 1622ba9930cd..538ea017f2dc 100644 --- a/sdk/azcore/CHANGELOG.md +++ b/sdk/azcore/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +* The `retry-after-ms` and `x-ms-retry-after-ms` headers weren't being checked during retries. + ### Other Changes ## 1.9.0 (2023-11-06) diff --git a/sdk/azcore/internal/shared/constants.go b/sdk/azcore/internal/shared/constants.go index 1a094c2c46fa..bb93daee6818 100644 --- a/sdk/azcore/internal/shared/constants.go +++ b/sdk/azcore/internal/shared/constants.go @@ -22,11 +22,13 @@ const ( HeaderLocation = "Location" HeaderOperationLocation = "Operation-Location" HeaderRetryAfter = "Retry-After" + HeaderRetryAfterMS = "Retry-After-Ms" HeaderUserAgent = "User-Agent" HeaderWWWAuthenticate = "WWW-Authenticate" HeaderXMSClientRequestID = "x-ms-client-request-id" HeaderXMSRequestID = "x-ms-request-id" HeaderXMSErrorCode = "x-ms-error-code" + HeaderXMSRetryAfterMS = "x-ms-retry-after-ms" ) const BearerTokenPrefix = "Bearer " diff --git a/sdk/azcore/internal/shared/shared.go b/sdk/azcore/internal/shared/shared.go index 16bc105f4811..457a4d0dbf9b 100644 --- a/sdk/azcore/internal/shared/shared.go +++ b/sdk/azcore/internal/shared/shared.go @@ -44,22 +44,61 @@ func Delay(ctx context.Context, delay time.Duration) error { } } -// RetryAfter returns non-zero if the response contains a Retry-After header value. +// RetryAfter returns non-zero if the response contains one of the headers with a "retry after" value. +// Headers are checked in the following order: retry-after-ms, x-ms-retry-after-ms, retry-after func RetryAfter(resp *http.Response) time.Duration { if resp == nil { return 0 } - ra := resp.Header.Get(HeaderRetryAfter) - if ra == "" { - return 0 + + type retryData struct { + header string + units time.Duration + custom func(string) (time.Duration, error) + } + + nop := func(string) (time.Duration, error) { return 0, nil } + + // the headers are listed in order of preference + retries := []retryData{ + { + header: HeaderRetryAfterMS, + units: time.Millisecond, + custom: nop, + }, + { + header: HeaderXMSRetryAfterMS, + units: time.Millisecond, + custom: nop, + }, + { + header: HeaderRetryAfter, + units: time.Second, + + // retry-after values are expressed in either number of + // seconds or an HTTP-date indicating when to try again + custom: func(ra string) (time.Duration, error) { + t, err := time.Parse(time.RFC1123, ra) + if err != nil { + return 0, err + } + return time.Until(t), nil + }, + }, } - // retry-after values are expressed in either number of - // seconds or an HTTP-date indicating when to try again - if retryAfter, _ := strconv.Atoi(ra); retryAfter > 0 { - return time.Duration(retryAfter) * time.Second - } else if t, err := time.Parse(time.RFC1123, ra); err == nil { - return time.Until(t) + + for _, retry := range retries { + v := resp.Header.Get(retry.header) + if v == "" { + continue + } + if retryAfter, _ := strconv.Atoi(v); retryAfter > 0 { + return time.Duration(retryAfter) * retry.units + } else if d, err := retry.custom(v); err == nil { + return d + } } + return 0 } diff --git a/sdk/azcore/internal/shared/shared_test.go b/sdk/azcore/internal/shared/shared_test.go index b17b37ed5c09..ee4907e995a4 100644 --- a/sdk/azcore/internal/shared/shared_test.go +++ b/sdk/azcore/internal/shared/shared_test.go @@ -59,6 +59,22 @@ func TestRetryAfter(t *testing.T) { if d = RetryAfter(resp); d != 0 { t.Fatalf("expected zero for invalid value, got %d", d) } + // verify that the ms-granularity headers are preferred + resp.Header = http.Header{} + resp.Header.Set(HeaderRetryAfterMS, "500") + require.Equal(t, time.Duration(500)*time.Millisecond, RetryAfter(resp)) + resp.Header = http.Header{} + resp.Header.Set(HeaderXMSRetryAfterMS, "400") + require.Equal(t, time.Duration(400)*time.Millisecond, RetryAfter(resp)) + resp.Header = http.Header{} + resp.Header.Set(HeaderRetryAfterMS, "500") + resp.Header.Set(HeaderXMSRetryAfterMS, "400") + resp.Header.Set(HeaderRetryAfter, "300") + require.Equal(t, time.Duration(500)*time.Millisecond, RetryAfter(resp)) + resp.Header = http.Header{} + resp.Header.Set(HeaderXMSRetryAfterMS, "400") + resp.Header.Set(HeaderRetryAfter, "300") + require.Equal(t, time.Duration(400)*time.Millisecond, RetryAfter(resp)) } func TestTypeOfT(t *testing.T) { From 0095bebb4a174250f9d1a2f497a3493353f6ec2f Mon Sep 17 00:00:00 2001 From: Joel Hendrix Date: Fri, 8 Dec 2023 13:54:04 -0800 Subject: [PATCH 2/3] add clarifying comment and negative tests --- sdk/azcore/internal/shared/shared.go | 3 +++ sdk/azcore/internal/shared/shared_test.go | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/sdk/azcore/internal/shared/shared.go b/sdk/azcore/internal/shared/shared.go index 457a4d0dbf9b..10e0d59986b3 100644 --- a/sdk/azcore/internal/shared/shared.go +++ b/sdk/azcore/internal/shared/shared.go @@ -54,6 +54,9 @@ func RetryAfter(resp *http.Response) time.Duration { type retryData struct { header string units time.Duration + + // custom is used when the regular algorithm failed and is optional. + // the returned duration is used verbatim (units is not applied). custom func(string) (time.Duration, error) } diff --git a/sdk/azcore/internal/shared/shared_test.go b/sdk/azcore/internal/shared/shared_test.go index ee4907e995a4..46a2ecfbb24c 100644 --- a/sdk/azcore/internal/shared/shared_test.go +++ b/sdk/azcore/internal/shared/shared_test.go @@ -75,6 +75,12 @@ func TestRetryAfter(t *testing.T) { resp.Header.Set(HeaderXMSRetryAfterMS, "400") resp.Header.Set(HeaderRetryAfter, "300") require.Equal(t, time.Duration(400)*time.Millisecond, RetryAfter(resp)) + resp.Header = http.Header{} + resp.Header.Set(HeaderRetryAfterMS, "invalid") + require.Zero(t, RetryAfter(resp)) + resp.Header = http.Header{} + resp.Header.Set(HeaderXMSRetryAfterMS, "invalid") + require.Zero(t, RetryAfter(resp)) } func TestTypeOfT(t *testing.T) { From afbc1ba4c1300aa1f51809ee61063d4b796748a2 Mon Sep 17 00:00:00 2001 From: Joel Hendrix Date: Fri, 8 Dec 2023 13:57:19 -0800 Subject: [PATCH 3/3] simplify custom --- sdk/azcore/internal/shared/shared.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sdk/azcore/internal/shared/shared.go b/sdk/azcore/internal/shared/shared.go index 10e0d59986b3..d3da2c5fdfa3 100644 --- a/sdk/azcore/internal/shared/shared.go +++ b/sdk/azcore/internal/shared/shared.go @@ -57,10 +57,10 @@ func RetryAfter(resp *http.Response) time.Duration { // custom is used when the regular algorithm failed and is optional. // the returned duration is used verbatim (units is not applied). - custom func(string) (time.Duration, error) + custom func(string) time.Duration } - nop := func(string) (time.Duration, error) { return 0, nil } + nop := func(string) time.Duration { return 0 } // the headers are listed in order of preference retries := []retryData{ @@ -80,12 +80,12 @@ func RetryAfter(resp *http.Response) time.Duration { // retry-after values are expressed in either number of // seconds or an HTTP-date indicating when to try again - custom: func(ra string) (time.Duration, error) { + custom: func(ra string) time.Duration { t, err := time.Parse(time.RFC1123, ra) if err != nil { - return 0, err + return 0 } - return time.Until(t), nil + return time.Until(t) }, }, } @@ -97,7 +97,7 @@ func RetryAfter(resp *http.Response) time.Duration { } if retryAfter, _ := strconv.Atoi(v); retryAfter > 0 { return time.Duration(retryAfter) * retry.units - } else if d, err := retry.custom(v); err == nil { + } else if d := retry.custom(v); d > 0 { return d } }