Should file system buffer filer call injectDecodedDataToFilterChain with appropriate end_stream flag?

[yanjunxiang-google]

Title: File system buffer filer should call injectDecodedDataToFilterChain with appropriate end_stream flag

Description:

Currently it is calling injectDecodedDataToFilterChain() and injectencodedDataToFilterChain() with hard coded "false", regardless whether the data is the last chunk, or whether there will be trailers behind:

envoy/source/extensions/filters/http/file_system_buffer/filter.cc

Line 248 in fcaf66a

request_callbacks_->injectDecodedDataToFilterChain(*out, false);

envoy/source/extensions/filters/http/file_system_buffer/filter.cc

Line 273 in fcaf66a

response_callbacks_->injectEncodedDataToFilterChain(*out, false);

[yanjunxiang-google]

/assign @mattklein123 @ravenblackx

[ravenblackx]

My understanding is that the subsequent continueDecoding resolves this correctly because the buffer is empty at that point (and leads to correctly handling trailers automatically - if there's some shared-buffer-related issue that can be provoked by the current implementation then that issue would still be problematic in the case where trailers are present, where the end_stream argument must still be false.)

I would not endorse changing this unless it can be demonstrated that the current implementation behaves incorrectly under some circumstances and that changing it would fix it for both of those situations.

(My guess is that it does behave incorrectly under some rare configuration circumstances, but that those circumstances are inherently broken because of the shared filter chain buffer; the correct fix for that is to fix that much older and broader mistake that impacts multiple filters, which would also make this implementation behave correctly, under the problematic "multiple filters in a filter chain have internal buffers" case, for the trailers case.)

[ravenblackx]

Or: If it was behaving incorrectly under normal circumstances then I would expect the integration tests to be failing. If there's a reason to change it, I would expect an integration test demonstrating the "wrong" behavior to be a start point.

[ravenblackx]

Ah, I see, this is not actually "should it be changed now" but rather "we want to change the API of injectDecodedDataToFilterChain to be more strict", per #43042 (comment)

If it's for that purpose then I would approve a change to this filter to facilitate that, though I can pretty much guarantee it will be a pain!