⚡ http client defer CloseIdleConnections

[cuisongliu]

when high concurrency occurs, the connection is full and the connection is rejected.

[bwplotka]

Hm, my understanding is that this change would only makes things slower, will not help with too many concurrent connections (if you mean that).

Perhaps a reproduction test would help? Perhaps there is some http transport settings you can adjust, or you hit some other limits?

[bwplotka]

Are you sure this is a correct solution?

My understanding is that this closes connections which might be REUSED on the subsequent Do request. So you have to establish them again (overhead)?

Source: https://forum.golangbridge.org/t/when-should-i-use-client-closeidleconnections/19254, https://stackoverflow.com/questions/18697290/apache-httpclient-how-to-auto-close-connections-by-servers-keep-alive-time

[cuisongliu]

Currently, doing an HTTP probe requiring allocating a new http.Client and http.Transport, and the underlying connections managed by http.Transport might be ESTABLISHED indefinitely if no parties (client or server) directly close them. As a result, if the probes continuously happen, we can see TCP connections opened by one process
drastically increase.

I understand that there may be such a problem in high concurrency. Even if it is reused, the connection is limited and cannot meet the requirements. In this case, the connection may be occupied and new connections cannot be applied for. As a result, the connection request cannot be continued.

PS. I appeared in a high-concurrency scenario, with about 10000 of concurrencies.

[bwplotka]

Nice! What "HTTP probe" do you mean? Is it some OSS code or your own app?

Generally it sounds you might use this client incorrectly if you create new client for single Do, assuming you can share the client.

For cases you cannot share e.g. it's serverless or it's on-off request to adhoc destination, I think you can access Config.Client (which is used to create API client) and invoke this method after Do on your own, which would be a correct thing to do 👍🏽

I am also open to create CloseIdleConnections() method on our client with a clear commentary when it's useful to make it easier (:

Would that help?

[cuisongliu]

in this conde add Close func ?

// Client is the interface for an API client.
type Client interface {
	URL(ep string, args map[string]string) *url.URL
	Do(context.Context, *http.Request) (*http.Response, []byte, error)
}

I think it would be more reasonable if the interface or open functions could be adjusted and allowed to be called by users themselves. Very useful to me too.

[cuisongliu]

@bwplotka i am add code to interface using CloseIdleConnections() func . plz review this code ?

[bwplotka]

Why do we need interface change? Can we remove this defer? Otherwise new method is good 👍🏽

[bwplotka]

Ok interface change is needed. Let's kill the defer statement here and LGTM!

[bwplotka]

Let's move it down (as 3rd in order)

[bwplotka]

Ok interface change is needed. Let's kill the defer statement here and LGTM!

[bwplotka]

Do we need this change on both interfaces really? Ideally those are very small.

[bwplotka]

So sorry for blocking this PR again, but I think we should remove interface changes. It is possible to avoid them. WDYT?

Thanks for patience!

[bwplotka]

Thanks! Almost there 💪🏽

[bwplotka]

Suggested change

	type closeIdler interface {
	type CloseIdler interface {

[bwplotka]

This allows us to avoid shallow ClientCloseIdler function.

[bwplotka]

Suggested change

	func ClientCloseIdler(cl Client) {
	cl.(closeIdler).CloseIdleConnections()
	}
	func ClientCloseIdler(cl Client) {
	cl.(closeIdler).CloseIdleConnections()
	}

No need for that, anyone can do cl.(CloseIdler).CloseIdleConnection()

[cuisongliu]

done

[bwplotka]

Ideally we can comment what this is for, but we can do in a separare PR, thanks!

[bwplotka]

Thanks!