http3: add shutdown notification support #13332
Conversation
Use the newly-added quiche support for sending an advisory GOAWAY frame on HTTP3 connections when an Envoy starts draining a connection. Signed-off-by: Alex Konradi <[email protected]>
| @@ -47,10 +47,7 @@ class QuicHttpServerConnectionImpl : public QuicHttpConnectionImplBase, | |||
|
|
|||
| // Http::Connection | |||
| void goAway() override; | |||
| void shutdownNotice() override { | |||
| // TODO(danzh): Add double-GOAWAY support in QUIC. | |||
There was a problem hiding this comment.
Can you leave the TODO there? It's not implemented in QUICHE yet. Or is it already implemented somehow in Envoy HCM? And shutdownNotice() is just part of the work flow?
There was a problem hiding this comment.
HCM already calls shutdownNotice(), which is used to send a first GOAWAY for H2 but it wasn't implemented by the quiche codec. This PR plugs the support I added in quiche into Envoy to get the equivalent behavior for H3.
There was a problem hiding this comment.
ACK. Thanks for doing this!
| if (quic::VersionUsesHttp3(quic_server_session_.transport_version())) { | ||
| quic_server_session_.SendHttp3Shutdown(); | ||
| } else { | ||
| ENVOY_CONN_LOG(error, "Shutdown notice is not propagated to QUIC.", quic_server_session_); |
There was a problem hiding this comment.
This seems like it could spam pretty badly? Should this be a debug log or a rate limited log variabt?
There was a problem hiding this comment.
Debug should be fine since this doesn't affect correctness. Also, the log line is only seen for older versions of HTTP-over-QUIC, which should be disappearing as HTTP3 nears standardization.
Signed-off-by: Alex Konradi <[email protected]>
Commit Message: Add shutdown notification support for HTTP3
Additional Description:
Use the newly-added quiche support for sending an advisory GOAWAY frame
on HTTP3 connections when an Envoy starts draining a connection.
Risk Level: low
Testing: ran quiche codec and integration tests
Docs Changes: none
Release Notes: none