Renew gRPC and REST connections after max_connection_age#6547
Conversation
guilload
force-pushed
the
guilload/expire-in-flight-connections
branch
2 times, most recently
from
6b01139 to
34ce5f1
Compare
guilload
force-pushed
the
guilload/expire-in-flight-connections
branch
from
34ce5f1 to
204b588
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 204b588544
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let max_connection_age_grace_opt = match max_connection_age_opt { | ||
| Some(max_connection_age) => max_connection_age.grace, | ||
| None => None, | ||
| }; | ||
| let max_connection_age_grace_sleep = match max_connection_age_grace_opt { | ||
| Some(max_connection_age_grace) => { | ||
| Either::Left(tokio::time::sleep(max_connection_age_grace)) |
There was a problem hiding this comment.
When the global shutdown token wins the first select!, this same grace timeout is still used, so any REST config that sets rest.max_connection_age_grace will force-close otherwise graceful process shutdowns after that interval. For example, a long-running REST request during a restart will now be dropped after the max-age grace (30s in the sample config), even though the old GracefulShutdown path waited for in-flight connections and tonic's analogous option only applies after a connection exceeds max_connection_age; track whether phase 2 was entered because of max age before applying this timeout.
Useful? React with 👍 / 👎.
| /// Bounding the connection lifetime is what lets a hot-reloaded TLS certificate eventually reach | ||
| /// long-lived clients: the new certificate is only presented on a fresh handshake, so the client | ||
| /// must reconnect to pick it up. | ||
| async fn drain_connection<C>( |
There was a problem hiding this comment.
i don't think this is a good name, as the document says, it drives a connection, not merely drain it
| // The server closes the long-lived connection on its own once the max connection age elapses. | ||
| // An idle HTTP/2 connection without a max age would otherwise stay open indefinitely, so a | ||
| // bounded close demonstrates the feature. | ||
| tokio::time::timeout(Duration::from_secs(10), connection_handle) |
There was a problem hiding this comment.
can we somehow check that connection_handle didn't terminate before the 2s mark (make sur the test isn't just passing because we somehow close unused connections quickly)
2 participants
Description
Renew gRPC and REST connections after
max_connection_agewhen set so hot-reloads of mTLS certs, keys, etc. takes effect.How was this PR tested?
Added unit and integration tests