Why close all idle connections when retry check is not OK?

[fisher046]

Hi, thanks for developing such wonderful library. I was trying to use it in my project and I have a question on below line.

https://github.com/hashicorp/go-retryablehttp/blob/4af2e4b1970397d927d9e8d74c97e8659c0350c3/client.go#L575-L581

May I know why need to do c.HTTPClient.CloseIdleConnections() when do not need retry? I found this will send a FIN to server side and start to close connection. But In my understanding, the connection can be still reused. Actually in my project, I found the connection number was keep growing after my service starts and accepts API requests. Then I removed this line in my project, the function is working and connections stop to keep growing.

[jefferai]

By default we use a "clean" http client that is thrown away after the request ends. This ensures that its connections are thus closed properly instead of leaking.

It's possible we should only do this if we create and use our own client, but not if we are using a client that was supplied externally.

[fisher046]

OK, thanks for your explanation. Actually in my case, I use one client globally through multiple goroutines. The test result shows there will be so many connections to downstream API servers kept. But I think it is ok for me because the connection number was not kept increasing. Remove the line c.HTTPClient.CloseIdleConnections() can help but may have potential issues as you said...

[PaulLiang1]

Hi @jefferai， just wondering if you use a clean http client each time, wouldn't that be very expensive as there is no connection re-use? every new connection requires dns lookup / tls handshake etc..

[jefferai]

Correct. But you can supply your own client.

The thing we're trying to ensure is that the default behavior of the library is not dependent on what other libraries are doing with a global client/transport -- basically the exact thing cleanhttp was created to help avoid, and that because of real world issues our users were seeing.

Absolutely feel free to bring your own client, I just advise having positive control over it :-)

[ppai-plivo]

Absolutely feel free to bring your own client

Turns out that this is not enough as you rightly pointed out in

It's possible we should only do this if we create and use our own client, but not if we are using a client that was supplied externally.

and since internal calls to CloseIdleConnections() are unconditional even when a custom HTTPClient is passed. In effect, calls to CloseIdleConnections makes many fields (MaxIdleConns, MaxIdleConnsPerHost, MaxConnsPerHost, IdleConnTimeout) set in a custom http.Client client to not behave as intended, when seen externally.

There's also the issue of client sharing...

go-retryablehttp closes all idle connections including the ones that it did not explicitly create. This means that any custom http.Client passed to go-retryablehttp is for its exclusive use. Here's an example use-case where the same http client is shared outside the scope of go-retryablehttp:

type someClient struct{
    client  *retryablehttp.Client
}

func newClient() *someClient  {
    rc := retryablehttp.NewClient()
    rc.HTTPClient = &http.Client{...}  // BYOC
    return &someClient{rc}
}

func (c *someClient) GetPrice(...) ... {
    // requests with retries enabled because it's safe to retry
    c.client.Do(..)
}

func (c *someClient) DeductPrice(...) ... {
    // requests with retries disabled
    c.client.HTTPClient.Do(...)
}

As pointed out here, one work-around is to create a custom transport which wraps around http.Transport with its CloseIdleConnections() method implemented as a no-op.

However, reverting PR https://github.com/hashicorp/go-retryablehttp/pull/57 allows:

• previous default behaviour of the library to be retained i.e keepalives disabled (including between retries)
• bring your own client to work as expected i.e no hidden surprises

Documenting the default behaviour of keep-alive and cleanhttp client used, IMHO, should suffice to consider issue #44 as resolved since that is what was initially requested. I can send a PR for this. @jefferai Would you be open to consider accepting a PR that reverts the changes in #57 ? Thank you.

[jefferai]

No, but I'd be open to a PR that doesn't close idle connections if the http client is not the default one created by the library.

[ppai-plivo]

Well, I should've looked at the code from master.

PR #70 fortunately removed calls to CloseIdleConnections() in the happy path 🎉 The current latest release v0.6.6 does not have this change. All I need now is a tagged release.

@jefferai Can you please consider tagging a release? Thanks!