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.

Sign up for GitHub

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

Jump to bottom

Use cleanhttp.DefaultPooledConnection and enforce cleanup #57

Merged
merged 3 commits into from
Aug 26, 2019
Merged

Use cleanhttp.DefaultPooledConnection and enforce cleanup #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef2c50a
Use cleanhttp.DefaultPooledConnection and enforce cleanup
jefferai May 7, 2019
9d0e946
Merge branch 'master' into issue-44
jefferai Aug 26, 2019
e3126ad
Fix merge
jefferai Aug 26, 2019
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
11 changes: 10 additions & 1 deletion client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ type Client struct {
// NewClient creates a new Client with default settings.
func NewClient() *Client {
return &Client{
HTTPClient: cleanhttp.DefaultClient(),
HTTPClient: cleanhttp.DefaultPooledClient(),
RetryWaitMin: defaultRetryWaitMin,
RetryWaitMax: defaultRetryWaitMax,
RetryMax: defaultRetryMax,
Expand Down Expand Up @@ -418,6 +418,10 @@ func PassthroughErrorHandler(resp *http.Response, err error, _ int) (*http.Respo

// Do wraps calling an HTTP method with retries.
func (c *Client) Do(req *Request) (*http.Response, error) {
if c.HTTPClient == nil {
c.HTTPClient = cleanhttp.DefaultPooledClient()
}

if c.logger() != nil {
switch v := c.logger().(type) {
case Logger:
Expand All @@ -437,6 +441,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) {
if req.body != nil {
body, err := req.body()
if err != nil {
c.HTTPClient.CloseIdleConnections()
return resp, err
}
if c, ok := body.(io.ReadCloser); ok {
Expand Down Expand Up @@ -496,6 +501,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) {
if checkErr != nil {
err = checkErr
}
c.HTTPClient.CloseIdleConnections()
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be here. This branch is when good requests are returned and closing idle here will make it mostly useless. You could probably safely move this up 2 lines inside check checkErr if so it's only run if we're running with an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we not want to close idle connections if it's successful?

Before this patch we have keepalives totally disabled. Closing idle connections in all cases preserves that behavior except when actively retrying.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the point of this patch was to leave connections alive so that future ones are faster?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but scoped only to the library's own retries.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok.

Copy link

Choose a reason for hiding this comment

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

This breaks compatibility with Go < v1.12, HTTPClient.CloseIdleConnections was only added in v1.12 😿

return resp, err
}

Expand Down Expand Up @@ -526,12 +532,14 @@ func (c *Client) Do(req *Request) (*http.Response, error) {
}
select {
case <-req.Context().Done():
c.HTTPClient.CloseIdleConnections()
return nil, req.Context().Err()
case <-time.After(wait):
}
}

if c.ErrorHandler != nil {
c.HTTPClient.CloseIdleConnections()
return c.ErrorHandler(resp, err, c.RetryMax+1)
}

Expand All @@ -540,6 +548,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) {
if resp != nil {
resp.Body.Close()
}
c.HTTPClient.CloseIdleConnections()
return nil, fmt.Errorf("%s %s giving up after %d attempts",
req.Method, req.URL, c.RetryMax+1)
}
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/hashicorp/go-retryablehttp

require (
github.com/hashicorp/go-cleanhttp v0.5.0
github.com/hashicorp/go-cleanhttp v0.5.1
github.com/hashicorp/go-hclog v0.9.2
)
7 changes: 5 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/hashicorp/go-cleanhttp v0.5.0 h1:wvCrVc9TjDls6+YGAF2hAifE1E5U1+b4tH6KdvN3Gig=
github.com/hashicorp/go-cleanhttp v0.5.0/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80=
github.com/hashicorp/go-cleanhttp v0.5.1 h1:dH3aiDG9Jvb5r5+bYHsikaOUIpcM0xvgMXVoDkXMzJM=
github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80=
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/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/stretchr/testify v1.2.2 h1:bSDNvY7ZPG5RlJ8otE/7V6gMiyenm9RtJ7IUVIAoJ1w=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=