-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Upgrade go-connlimit to v0.3.0 / return http 429 on too many connections #8221
Upgrade go-connlimit to v0.3.0 / return http 429 on too many connections #8221
Conversation
@@ -1154,7 +1154,7 @@ func (a *Agent) listenHTTP() ([]*HTTPServer, error) { | |||
srv.Server.Handler = srv.handler(a.config.EnableDebug) | |||
|
|||
// Load the connlimit helper into the server | |||
connLimitFn := a.httpConnLimiter.HTTPConnStateFunc() | |||
connLimitFn := a.httpConnLimiter.HTTPConnStateFuncWithDefault429Handler(10 * time.Millisecond) |
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 want to highlight this and explain what I think the implications are and make sure we are aware:
HTTPConnStateFunc
closes the connection when it is beyond the limit.Close
does not block.HTTPConnStateFuncWithDefault429Handler(10 * time.Millisecond)
blocks until the following is done (worst case):conn.SetDeadline(10*time.Millisecond)
so thatconn.Write(429error)
is guaranteed to timeout after 10ms, so that the http 429 can be written andconn.Close
can happen
The implication of this change is that accepting any new connection is worst case delayed by 10ms. But only after a client reached the limit already.
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.
Yes, indeed, you are fully correct. From my test, even by forcing very high latency with the clients, there ate no measurable change since write/close is fast because probably buffered by the OS.
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.
LGTM!
This implementation is basic and returns HTTP 429 when too many connections are done as explained in #7527, and will:
In my experience, this is one of the biggest source of questions since the introduction of the feature in Consul 1.6.x
This implementation is simple as it uses the default implementation from hashicorp/go-connlimit#6, I made a proof of concept to handle it differently to avoid closing the connection in the main thread as quickly demonstrated here: pierresouchay@8399df9
I am not sure it deserves such complexity, however, but if you want to another implementation, it is still possible.