-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
⚡ http client defer CloseIdleConnections #1513
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
api/client.go
Outdated
@@ -122,6 +122,7 @@ func (c *httpClient) Do(ctx context.Context, req *http.Request) (*http.Response, | |||
if ctx != nil { | |||
req = req.WithContext(ctx) | |||
} | |||
defer c.client.CloseIdleConnections() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bwplotka i am add code to interface using CloseIdleConnections() func . plz review this code ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need interface change? Can we remove this defer? Otherwise new method is good 👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok interface change is needed. Let's kill the defer statement here and LGTM!
274e000
to
05e818a
Compare
api/client.go
Outdated
@@ -75,6 +75,7 @@ func (cfg *Config) validate() error { | |||
|
|||
// Client is the interface for an API client. | |||
type Client interface { | |||
CloseIdleConnections() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move it down (as 3rd in order)
api/client.go
Outdated
@@ -122,6 +122,7 @@ func (c *httpClient) Do(ctx context.Context, req *http.Request) (*http.Response, | |||
if ctx != nil { | |||
req = req.WithContext(ctx) | |||
} | |||
defer c.client.CloseIdleConnections() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok interface change is needed. Let's kill the defer statement here and LGTM!
api/prometheus/v1/api.go
Outdated
@@ -1342,6 +1342,7 @@ type Warnings []string | |||
// apiClient wraps a regular client and processes successful API responses. | |||
// Successful also includes responses that errored at the API level. | |||
type apiClient interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this change on both interfaces really? Ideally those are very small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@@ -77,6 +77,7 @@ func (cfg *Config) validate() error { | |||
type Client interface { | |||
URL(ep string, args map[string]string) *url.URL | |||
Do(context.Context, *http.Request) (*http.Response, []byte, error) | |||
CloseIdleConnections() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double check and I would propose we remove CloseIdleConnections()
from both interfaces. We don't want to break compatibility for not too important reason (we kind of can because it's an experimental package, but only if we have to).
This method on interface is not needed because you can totally do
type closeIdler interface {
CloseIdleConnections()
}
// .. in some function
cl, err := NewClient()
// check err
cl.(closeIdler).CloseIdleConnection()
Thus to be safe, I would start with this approach first. It unblocks you without impacting potentially many packages in a very painful way (when upgrading client_golang).
ac1acdf
to
4029570
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Almost there 💪🏽
api/client.go
Outdated
@@ -79,6 +79,10 @@ type Client interface { | |||
Do(context.Context, *http.Request) (*http.Response, []byte, error) | |||
} | |||
|
|||
type closeIdler interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type closeIdler interface { | |
type CloseIdler interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows us to avoid shallow ClientCloseIdler
function.
api/client.go
Outdated
func ClientCloseIdler(cl Client) { | ||
cl.(closeIdler).CloseIdleConnections() | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func ClientCloseIdler(cl Client) { | |
cl.(closeIdler).CloseIdleConnections() | |
} | |
func ClientCloseIdler(cl Client) { | |
cl.(closeIdler).CloseIdleConnections() | |
} |
No need for that, anyone can do cl.(CloseIdler).CloseIdleConnection()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…tion is rejected. Signed-off-by: cuisongliu <cuisongliu@qq.com>
when high concurrency occurs, the connection is full and the connection is rejected.