Skip to content
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

MAX_CONNECTION_AGE_GRACE not really implemented #9721

Closed
ejona86 opened this issue Nov 30, 2022 · 1 comment · Fixed by #9968
Closed

MAX_CONNECTION_AGE_GRACE not really implemented #9721

ejona86 opened this issue Nov 30, 2022 · 1 comment · Fixed by #9968
Assignees
Milestone

Comments

@ejona86
Copy link
Member

ejona86 commented Nov 30, 2022

#9649 implemented MAX_CONNECTION_AGE and claims to implement the grace period as well, but it doesn't really. The shutdown code normally uses 1s as a limit during GOAWAY. That's actually fine as-is for MAX_CONNECTION_AGE. Most of the time that limit won't matter; most of the time the client will respond with a PING before it expires and we'll then promptly send the second GOAWAY.

What is missing is we should set a graceful timer to close the connection. That could either be an abrupt closure, but I think in Netty we trigger RST_STREAM all the streams and then the normal 'the connection is shutdown and there are no more streams' logic can naturally close the connection.

@ejona86 ejona86 added this to the Next milestone Nov 30, 2022
@YifeiZhuang
Copy link
Contributor

There is no RST_STREAM sent in the GracefulShutdown in grpc-netty.

There is RST_STREAM sent in shutdownNow in netty, and okhttp.

OkHttp will send two consecutive goAway(). Then wait MAX_CONNECTION_AGE_GRACE and close abruptly (a graceful shutdown similar to grpc-netty).

rfc7540#section-6.4

@ejona86 ejona86 modified the milestones: Next, 1.55 Apr 13, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants