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 retryablehttp Client Option #619

Merged
merged 11 commits into from
Aug 3, 2023
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ go 1.18

require (
github.com/google/go-querystring v1.1.0
github.com/hashicorp/go-retryablehttp v0.7.4
github.com/stretchr/testify v1.4.0
golang.org/x/oauth2 v0.0.0-20220411215720-9780585627b5
golang.org/x/time v0.0.0-20220922220347-f3bd1da661af
Expand All @@ -12,6 +13,7 @@ require (
require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/golang/protobuf v1.5.2 // indirect
github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
golang.org/x/net v0.7.0 // indirect
google.golang.org/appengine v1.6.7 // indirect
Expand Down
7 changes: 7 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ github.com/google/pprof v0.0.0-20200708004538-1a94d8640e99/go.mod h1:ZgVRPoUq/hf
github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI=
github.com/googleapis/gax-go/v2 v2.0.4/go.mod h1:0Wqv26UfaUD9n4G6kQubkQ+KchISgw+vpHVxEJEs9eg=
github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5mhpdKc/us6bOk=
github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ=
github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48=
github.com/hashicorp/go-hclog v0.9.2 h1:CG6TE5H9/JXsFWJCfoIVpKFIkFe6ysEuHirp4DxCsHI=
github.com/hashicorp/go-hclog v0.9.2/go.mod h1:5CU+agLiy3J7N7QjHK5d05KxGsuXiQLrjA0H7acj2lQ=
github.com/hashicorp/go-retryablehttp v0.7.4 h1:ZQgVdpTdAL7WpMIwLzCfbalOcSUdkDZnpUv3/+BxzFA=
github.com/hashicorp/go-retryablehttp v0.7.4/go.mod h1:Jy/gPYAdjqffZ/yFGCFV2doI5wjtH1ewM9u8iYVjtX8=
github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
github.com/ianlancetaylor/demangle v0.0.0-20181102032728-5e5cf60278f6/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc=
Expand All @@ -118,6 +124,7 @@ github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZN
github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA=
github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk=
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
Expand Down
62 changes: 62 additions & 0 deletions godo.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"time"

"github.com/google/go-querystring/query"
"github.com/hashicorp/go-retryablehttp"
"golang.org/x/oauth2"
"golang.org/x/time/rate"
)
Expand Down Expand Up @@ -92,6 +93,29 @@ type Client struct {

// Optional rate limiter to ensure QoS.
rateLimiter *rate.Limiter

// Optional retry values. Setting the RetryConfig.RetryMax value enables automatically retrying requests
// that fail with 429 or 500-level response codes using the go-retryablehttp client
RetryConfig RetryConfig
}

// RetryConfig sets the values used for enabling retries and backoffs for
// requests that fail with 429 or 500-level response codes using the go-retryablehttp client.
// RetryConfig.RetryMax must be configured to enable this behavior. RetryConfig.RetryWaitMin and
// RetryConfig.RetryWaitMax are optional, with the default values being 1.0 and 30.0, respectively.
Copy link
Member Author

Choose a reason for hiding this comment

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

RetryWaitMax and RetryWaitMin do have default values: https://github.com/hashicorp/go-retryablehttp/blob/main/client.go#L51

Copy link
Member

Choose a reason for hiding this comment

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

Those defaults look good based on the testing I did over the course of looking at this branch, so I'm happy with using those 👍

//
// You can use
//
// godo.PtrTo(1.0)
//
// to explicitly set the RetryWaitMin and RetryWaitMax values.
//
// Note: Opting to use the go-retryablehttp client will overwrite any custom HTTP client passed into New().
danaelhe marked this conversation as resolved.
Show resolved Hide resolved
// Only the custom HTTP client's custom transport and timeout will be maintained.
type RetryConfig struct {
RetryMax int
RetryWaitMin *float64 // Minimum time to wait
RetryWaitMax *float64 // Maximum time to wait
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed RetryWaitMin and RetryWaitMax to pointers to differentiate between an unset variable and a variable set to 0.0.

}

// RequestCompletionCallback defines the type of the request callback function
Expand Down Expand Up @@ -271,6 +295,33 @@ func New(httpClient *http.Client, opts ...ClientOpt) (*Client, error) {
}
}

// if retryMax is set it will use the retryablehttp client.
if c.RetryConfig.RetryMax > 0 {
retryableClient := retryablehttp.NewClient()
retryableClient.RetryMax = c.RetryConfig.RetryMax

if c.RetryConfig.RetryWaitMin != nil {
retryableClient.RetryWaitMin = time.Duration(*c.RetryConfig.RetryWaitMin * float64(time.Second))
}
if c.RetryConfig.RetryWaitMax != nil {
retryableClient.RetryWaitMax = time.Duration(*c.RetryConfig.RetryWaitMax * float64(time.Second))
Copy link
Member Author

@danaelhe danaelhe Aug 2, 2023

Choose a reason for hiding this comment

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

An effort to avoid an unset RetryWaitMax and RetryWaitMin with a value of 0.0 overriding the retryablehttp's default value of 30 seconds and 1 second, respectively.

}

// if timeout is set, it is maintained before overwriting client with StandardClient()
retryableClient.HTTPClient.Timeout = c.client.Timeout
Copy link
Member Author

Choose a reason for hiding this comment

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

I find that some opensource packages will also set a timeout, so decided to maintain it as well.


var source *oauth2.Transport
if _, ok := c.client.Transport.(*oauth2.Transport); ok {
source = c.client.Transport.(*oauth2.Transport)
}
c.client = retryableClient.StandardClient()
danaelhe marked this conversation as resolved.
Show resolved Hide resolved
c.client.Transport = &oauth2.Transport{
Base: c.client.Transport,
Source: source.Source,
}

}

return c, nil
}

Expand Down Expand Up @@ -315,6 +366,17 @@ func SetStaticRateLimit(rps float64) ClientOpt {
}
}

// WithRetryAndBackoffs sets retry values. Setting the RetryConfig.RetryMax value enables automatically retrying requests
// that fail with 429 or 500-level response codes using the go-retryablehttp client
func WithRetryAndBackoffs(retryConfig RetryConfig) ClientOpt {
return func(c *Client) error {
c.RetryConfig.RetryMax = retryConfig.RetryMax
c.RetryConfig.RetryWaitMax = retryConfig.RetryWaitMax
c.RetryConfig.RetryWaitMin = retryConfig.RetryWaitMin
return nil
}
}

// NewRequest creates an API request. A relative URL can be provided in urlStr, which will be resolved to the
// BaseURL of the Client. Relative URLS should always be specified without a preceding slash. If specified, the
// value pointed to by body is JSON encoded and included in as the request body.
Expand Down
49 changes: 49 additions & 0 deletions godo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"testing"
"time"

"golang.org/x/oauth2"
"golang.org/x/time/rate"
)

Expand Down Expand Up @@ -603,6 +604,54 @@ func TestDo_rateLimit_errorResponse(t *testing.T) {
}
}

// TestWithRetryAndBackoffs tests the retryablehttp client's default retry policy.
func TestWithRetryAndBackoffs(t *testing.T) {
// Mock server which always responds 500.
setup()
defer teardown()

url, _ := url.Parse(server.URL)
mux.HandleFunc("/foo", func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(500)
})

tokenSrc := oauth2.StaticTokenSource(&oauth2.Token{
AccessToken: "new_token",
})

oauth_client := oauth2.NewClient(oauth2.NoContext, tokenSrc)

waitMax := PtrTo(6.0)
waitMin := PtrTo(3.0)

retryConfig := RetryConfig{
RetryMax: 3,
RetryWaitMin: waitMin,
RetryWaitMax: waitMax,
}

// Create the client. Use short retry windows so we fail faster.
client, err := New(oauth_client, WithRetryAndBackoffs(retryConfig))
client.BaseURL = url
if err != nil {
t.Fatalf("err: %v", err)
}

// Create the request
req, err := client.NewRequest(ctx, http.MethodGet, "/foo", nil)
if err != nil {
t.Fatalf("err: %v", err)
}

expectingErr := "giving up after 4 attempt(s)"
// Send the request.
_, err = client.Do(context.Background(), req, nil)
if err == nil || !strings.HasSuffix(err.Error(), expectingErr) {
t.Fatalf("expected giving up error, got: %#v", err)
}

}

func checkCurrentPage(t *testing.T, resp *Response, expectedPage int) {
links := resp.Links
p, err := links.CurrentPage()
Expand Down
Loading