-
Notifications
You must be signed in to change notification settings - Fork 217
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
Don't open and close HTTP connections too often #110
Don't open and close HTTP connections too often #110
Conversation
This adds a test to verify we aren't opening and closing HTTP connections too often when sending events to a single host. The test will fail in CI (it fails locally) until I push a subsequent change to actually fix this issue. The test tracks the count of opened HTTP connections and fails if it's an amount more than double the expected concurrency. In practice, when this is fixed properly, the number of opened connections should be close to if not exactly the expected concurrency. In the case of this test, that would be 200 new connections. As the code stands today, when I run this on my laptop, we actually opening over 3000 new connections in just the 1 second this test runs. Signed-off-by: Ben Browning <bbrownin@redhat.com>
4c1205b
to
5849507
Compare
This blew up as expected in CI, confirming we have an issue with the current behavior:
|
Signed-off-by: Ben Browning <bbrownin@redhat.com>
If a user explicitly needs to close the connection after sending every event, they can do so similar to how they can override the request method and headers - by setting the req.Close value in a http.Request passed and setting that as `transport.Req`. Signed-off-by: Ben Browning <bbrownin@redhat.com>
Thanks a bunch. Now @matzew can still set that setting using the request injection and it is off by default. |
return nil, err | ||
} | ||
netHTTPTransport := &http.Transport{ | ||
MaxIdleConnsPerHost: 1000, |
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.
@bbrowning, how does MaxIdleConnsPerHost
with a value of 1000
interact with MaxIdleConns
with its default value of 100
?
My understanding is that MaxIdleConns
is the number of connections to keep-alive across all hosts and MaxIdleConnsPerHost
is the number of connections to keep alive per host. I'm guessing that if MaxIdleConnsPerHost
has a higher value than MaxIdleConns
, only MaxIdleConns
will be used??
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.
@dubee I had to dig into the source to figure this out, but both values get applied. It first checks MaxIdleConnsPerHost
and then enforces MaxIdleConns
by evicting the oldest connections from the cache. There are slightly different semantics in the errors returned between the two limits as well as the actual behavior - hitting the global limit still puts the new connection in the cache and just evicts the oldest. Hitting the per-host limit doesn't put the new connection in the cache at all.
For the purposes of this test, that doesn't seem to actually matter, but it's good to know when tweaking these values on the Knative side of things.
This adds a test to verify we aren't opening and closing HTTP
connections too often when sending events to a single host. The test
will fail in CI (it fails locally) until I push a subsequent change to
actually fix this issue.
The test tracks the count of opened HTTP connections and fails if it's
an amount more than double the expected concurrency. In practice, when
this is fixed properly, the number of opened connections should be
close to if not exactly the expected concurrency. In the case of this
test, that would be 200 new connections. As the code stands today,
when I run this on my laptop, we actually opening over 3000 new
connections in just the 1 second this test runs.
Related to #98