hcm: ensure data added during filter callbacks get propagated#7700
hcm: ensure data added during filter callbacks get propagated#7700snowp wants to merge 3 commits intoenvoyproxy:masterfrom
Conversation
Currently, add*Data will write data to the filter buffer, but we only ever read from said buffer if we ever transition from stopping -> continuing. This means that if add*Data is called when every callback returns Continue, the data is never propagated to the next filter. This change adds a flag that is set whenever data is written to the buffer, using this to flush the buffer when onData returns Continue even if filter iteration was never stopped. Fixes envoyproxy#7579 Signed-off-by: Snow Pettersen <snowp@squareup.com>
|
@soya3129 could you help to do the first pass? Thank you. |
| recordLatestDataFilter(entry, state_.latest_data_decoding_filter_, decoder_filters_); | ||
|
|
||
| state_.filter_call_state_ |= FilterCallState::DecodeData; | ||
| (*entry)->inline_buffered_data_ = false; |
There was a problem hiding this comment.
Why we don't need (*entry)->inline_buffered_data_ = false; in ActiveStream::encodeData()?
|
|
||
| ENVOY_STREAM_LOG(trace, "continuing filter chain: filter={}", parent_, | ||
| static_cast<const void*>(this)); | ||
| ASSERT(!canIterate()); |
There was a problem hiding this comment.
I really like this solution! It's simple and it works! The only concern I have is the removal of ASSERT here and below. Is it possible to keep the assert? For example: ASSERT(!canIterate() || inline_buffered_data_)?
There was a problem hiding this comment.
yep that makes sense. I was ripping out all the asserts that failed, makes sense to make it more exact instead
Signed-off-by: Snow Pettersen <snowp@squareup.com>
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Currently, addData will write data to the filter buffer, but we only
ever read from said buffer if we ever transition from stopping ->
continuing. This means that if addData is called when every callback
returns Continue, the data is never propagated to the next filter.
This change adds a flag that is set whenever data is written to the
buffer, using this to flush the buffer when onData returns Continue even
if filter iteration was never stopped.
Fixes #7579
Signed-off-by: Snow Pettersen snowp@squareup.com
Risk Level: Medium
Testing: Added HCM UT
Docs Changes: n/a
Release Notes: n/a
Fixes #7579