Use cleanhttp.DefaultPooledConnection and enforce cleanup #57
Conversation
This allows reuse of the existing connection on retries, since they're going to the same host, but we explicitly close idle connections to ensure they don't stick around forever after the series of requests is done. Fixes #44
| @@ -422,6 +427,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) { | |||
| if checkErr != nil { | |||
| err = checkErr | |||
| } | |||
| c.HTTPClient.CloseIdleConnections() | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I thought the point of this patch was to leave connections alive so that future ones are faster?
There was a problem hiding this comment.
Yes, but scoped only to the library's own retries.
There was a problem hiding this comment.
This breaks compatibility with Go < v1.12, HTTPClient.CloseIdleConnections was only added in v1.12 😿
This allows reuse of the existing connection on retries, since they're
going to the same host, but we explicitly close idle connections to
ensure they don't stick around forever after the series of requests is
done.
Fixes #44