Force stop iteration after local response is sent

[mathetake]

Signed-off-by: mathetake [email protected]

ref

• wasm: Force stop iteration after local response is sent envoyproxy/envoy#13930
• Crash using wasm-filter envoyproxy/envoy#13857

[mathetake]

I made the change almost identical to envoyproxy/envoy#13873 but something weird seems to happen here:

with the change in Envoy, the decorder/encoder message like this and it looks fine (I mean, the local response is be sent to the client properly).

[2020-11-03 16:21:04.041][871494][trace][http] [source/common/http/filter_manager.cc:938] [C0][S552029742600055844] encode headers called: filter=0x3f33ffcd47d0 status=0
[2020-11-03 16:21:04.041][871494][trace][http] [source/common/http/filter_manager.cc:1080] [C0][S552029742600055844] encode data called: filter=0x3f33ffcd47d0 status=0
[2020-11-03 16:21:04.041][871494][trace][http] [source/common/http/filter_manager.cc:451] [C0][S552029742600055844] decode headers called: filter=0x3f33fea12540 status=1

but with the change here, we have the following logs, and the response never is never returned to the client (curl in this case)

[2020-11-03 16:26:52.872][887837][trace][http] [source/common/http/filter_manager.cc:938] [C0][S6785004984496335233] encode headers called: filter=0xe013e0a8370 status=1
[2020-11-03 16:26:52.872][887837][trace][http] [source/common/http/filter_manager.cc:1080] [C0][S6785004984496335233] encode data called: filter=0xe013e0a8370 status=3
[2020-11-03 16:26:52.872][887837][trace][http] [source/common/http/filter_manager.cc:451] [C0][S6785004984496335233] decode headers called: filter=0xe013ea07f20 status=1

It seems like the change here somehow affects the encoder of local reply (I'm not sure how local response is handled in Envoy) and stops it from continuing 🤔

[mathetake]

same problem lingers on.......

[mathetake]

it seems to me that OnResponseHeaders and OnResponseBody are called in response to the sendLocalResponse? And the BaseContext used to handle these events must be the same the context which invoked sendLocalResponse, which means stop_iteration_ is true for the local response as well. I think that is the difference and something unexpected seems happening

[mathetake]

I'm know next to nothing about the internal behavior of local response, but the logs show that the filter addresses(?) are different

[PiotrSikora]

it seems to me that OnResponseHeaders and OnResponseBody are called in response to the sendLocalResponse? And the BaseContext used to handle these events must be the same the context which invoked sendLocalResponse, which means stop_iteration_ is true for the local response as well. I think that is the difference and something unexpected seems happening

Yes, OnResponse{Headers,Body,Trailers} are called for the local response as well.

[PiotrSikora]

This looks great, thanks! Although, I wonder if we should be so permissive and "fix" the broken code on the fly.

Similarly, could you open a PR in Envoy against your branch to verify that it passes all tests?

[mathetake]

Although, I wonder if we should be so permissive and "fix" the broken code on the fly.

Agreed

[mathetake]

@PiotrSikora envoyproxy/envoy#13930 all tests passed!

[PiotrSikora]

@mathetake It looks that we don't have good coverage, since this breaks send local reply in production code.

[PiotrSikora]

@mathetake OK, I see the issue now. We're overriding "current" action with StopAllIterationAndWatermark, however, not all callbacks return action (most notably, on_http_call_response does not), in which case we're not overriding "current" action, but action of the subsequent call. We need to clear stop_iteration_ flag after we return from VM, not when converting actions.