Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add client-side support for datetime value of Retry-After header #131

Merged
merged 4 commits into from
Sep 26, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions internal/retryafter.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,19 @@ func ExtractRetryAfterHeader(resp *http.Response) OptionalDuration {
resp.StatusCode == http.StatusTooManyRequests {
retryAfter := strings.TrimSpace(resp.Header.Get(retryAfterHTTPHeader))
if retryAfter != "" {
// Try to parse retryAfterHTTPHeader as delay-seconds
retryIntervalSec, err := strconv.Atoi(retryAfter)
if err == nil {
retryInterval := time.Duration(retryIntervalSec) * time.Second
return OptionalDuration{Defined: true, Duration: retryInterval}
}
// Try parse retryAfterHTTPHeader as HTTP-date
// See https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.3
t, err := http.ParseTime(retryAfter)
if err == nil {
retryInterval := time.Until(t)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably check that this is not negative. And the same in the case above with seconds. Seems like omission to me.

return OptionalDuration{Defined: true, Duration: retryInterval}
}
}
}
return OptionalDuration{Defined: false}
Expand Down
100 changes: 100 additions & 0 deletions internal/retryafter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
package internal

import (
"github.com/stretchr/testify/assert"
"math/rand"
"net/http"
"strconv"
"testing"
"time"
)

func response503() *http.Response {
return &http.Response{
StatusCode: http.StatusServiceUnavailable,
Header: map[string][]string{},
}
}

func TestExtractRetryAfterHeaderDelaySeconds(t *testing.T) {
// Generate random n > 0 int
rand.Seed(time.Now().UnixNano())
retryIntervalSec := rand.Intn(9999)

// Generate a 503 status code response with Retry-After = n header
resp := response503()
resp.Header.Add(retryAfterHTTPHeader, strconv.Itoa(retryIntervalSec))

expectedDuration := OptionalDuration{
Defined: true,
Duration: time.Second * time.Duration(retryIntervalSec),
}
assert.Equal(t, expectedDuration, ExtractRetryAfterHeader(resp))
}

func TestExtractRetryAfterHeader429StatusCode(t *testing.T) {
// Generate random n > 0 int
rand.Seed(time.Now().UnixNano())
retryIntervalSec := rand.Intn(9999)

// Generate a 429 status code response with Retry-After = n header
resp := response503()
resp.StatusCode = http.StatusTooManyRequests
resp.Header.Add(retryAfterHTTPHeader, strconv.Itoa(retryIntervalSec))

expectedDuration := OptionalDuration{
Defined: true,
Duration: time.Second * time.Duration(retryIntervalSec),
}
assert.Equal(t, expectedDuration, ExtractRetryAfterHeader(resp))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 tests are almost identical. It would be nice to make them one test with 2 sub-tests. It will eliminate a lot of lines.


func TestExtractRetryAfterHeaderInvalidFormat(t *testing.T) {
// Generate a random n > 0 int
now := time.Now()
//rand.Seed(now.UnixNano())
retryIntervalSec := rand.Intn(9999)

// Set a response with Retry-After header = random n > 0 int
resp := response503()
ra := now.Add(time.Second * time.Duration(retryIntervalSec)).UTC()

// Verify non HTTP-date RFC1123 format isn't being parsed
resp.Header.Set(retryAfterHTTPHeader, ra.Format(time.RFC1123))
d := ExtractRetryAfterHeader(resp)
assert.NotNil(t, d)
assert.Equal(t, false, d.Defined)
assert.Equal(t, time.Duration(0), d.Duration)
}

func TestExtractRetryAfterHeaderHttpDate(t *testing.T) {
// Generate a random n > 0 int
now := time.Now()
//rand.Seed(now.UnixNano())
retryIntervalSec := rand.Intn(9999)

// Set a response with Retry-After header = random n > 0 int
resp := response503()
ra := now.Add(time.Second * time.Duration(retryIntervalSec)).UTC()

// Verify HTTP-date TimeFormat format is being parsed correctly
resp.Header.Set(retryAfterHTTPHeader, ra.Format(http.TimeFormat))
d := ExtractRetryAfterHeader(resp)
assert.NotNil(t, d)
assert.Equal(t, true, d.Defined)
assert.Less(t, d.Duration, time.Second*time.Duration(retryIntervalSec))

// Verify ANSI time format
resp.Header.Set(retryAfterHTTPHeader, ra.Format(time.ANSIC))
d = ExtractRetryAfterHeader(resp)
assert.NotNil(t, d)
assert.Equal(t, true, d.Defined)
assert.Less(t, d.Duration, time.Second*time.Duration(retryIntervalSec))

// Verify RFC850 time format
resp.Header.Set(retryAfterHTTPHeader, ra.Format(time.RFC850))
d = ExtractRetryAfterHeader(resp)
assert.NotNil(t, d)
assert.Equal(t, true, d.Defined)
assert.Less(t, d.Duration, time.Second*time.Duration(retryIntervalSec))
}