http2: fixing a (known) potential stall in the filter chain for H2 #7093
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Alternately we could always just create the buffer on returning StopIterationNoBuffer, but that felt higher risk |
| @@ -164,6 +164,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |||
| // current filter instead of the next one. If true, filter iteration starts with the current | |||
| // filter. Otherwise, starts with the next filter in the chain. | |||
| bool iterate_from_current_filter_; | |||
| // Tracks if the stream end has been sent with an encode/decodeData call, to handle the case | |||
| // where commonContinue is called with the only information being a terminal fin. | |||
| bool end_stream_sent_with_data_; | |||
There was a problem hiding this comment.
Can we add this bool to the bitfield below? IIRC the reason the one above isn't in it is because it needs to be passed by address somewhere.
| doData(complete() && !trailers()); | ||
| // Make sure we handle filters returning StopIterationNoBuffer and then commonContinue by tracking | ||
| // if the terminal fin has already been proxied. | ||
| bool end_stream_with_data = complete() && !trailers(); |
| // buffer but encode/decodeData expects the buffer to exist, so create one. | ||
| bufferedData() = createBuffer(); | ||
| } | ||
| end_stream_sent_with_data_ = true; |
There was a problem hiding this comment.
Sorry why do we need the member variable? Can't we just compute this locally and then decide whether to create the buffer or not?
There was a problem hiding this comment.
My concern was there might be some case with StopAllIIteration where we might end up relaying the FIN twice. if you're confident that commonDecodePrefix will pick up the right filter (which theoretically it ought to when bodies are present, so ought to when they're absent?) I'm fine nixing the local state.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Fixing a bug where if pausing with StopIterationNoBuffer, continuing with fin-only payload wouldn't result in the filter chain continuing.
Risk Level: High (tweak to the HCM)
Testing: new integration test
Docs Changes: n/a
Release Notes: n/a