-
Notifications
You must be signed in to change notification settings - Fork 1.2k
tonic: add max connection age grace #2449
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
base: v0.14.x
Are you sure you want to change the base?
Conversation
|
@LucioFranco any chance I could get a review from someone? Thank you! |
|
Hi, If I understand correctly https://github.com/grpc/proposal/blob/master/A9-server-side-conn-mgt.md and https://grpc.io/docs/guides/server-graceful-stop/ are not particularly related. https://github.com/grpc/proposal/blob/master/A9-server-side-conn-mgt.md is about garbage collection of old connections, while graceful shutdown is about server shutdowns. So, while this PR itself looks okay, I am not particularly sure this'll solve issues around server shutdown you may be facing, but it should help reduce unbounded memory growth due to unresponsible clients. |
|
Hey @sauravzg, thank you. I'm not facing any server shutdown issue, when I say shutdown I imply a connection shutdown and not a server shutdown. As you already said the main purpose is to reduce the memory growth due to unresponsible clients. |
sauravzg
left a comment
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. Let's wait for another review from another maintainer before merge.
arjan-bal
left a comment
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. To clarify, this change relates to the MAX_CONNECTION_AGE_GRACE configuration (documented here: https://grpc.io/docs/guides/keepalive/#keepalive-configuration-specification).
This is distinct from the Graceful Shutdown documentation, which describes shutting down the entire server instance.
This file seems to lack good unit test coverage. @LucioFranco are there existing tests that can be updated adding coverage the new field?
tottoto
left a comment
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.
Looks good to me.
Motivation
According to the gRPC
documentationdocumentation when a connection exceeds the graceful shutdown timeout, the server should begin a secondary forceful shutdown timer. If the client still does not close the connection within this period, the server must terminate the connection.Problem
Tonic currently does not enforce this second-phase shutdown. As a result, in long-running applications where clients become congested, stuck or simply ignore the
GOAWAYframe, connections can linger indefinitely. This leads to unbounded growth in memory usage and a continuously increasing number of live Tokio tasks.Solution
This PR introduces a
max_connection_age_gracetimeout that starts aftermax_connection_ageexpires. If the client does not close the connection within this grace period, the server forcefully closes it. This mirrors the behavior of the gRPC Go implementation.A similar idea was previously mentioned in a closed PR but it seems the discussion stalled before the feature was implemented.