From de806d8f7beae6a7678b26793091512a1d717391 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 19 Jan 2021 16:56:58 -0500 Subject: [PATCH 1/2] thrift proxy: add comments explaining local replies While debugging #14723, it took me a long time to figure out how filter chain processing is supposed to _safely_ stop after a local reply was triggered. This gets slightly tricky because there's two possibilities after sending a local response: 1) you don't close the connection, so you need to keep decoding (but shouldn't run the filters) 2) you do close the connection, which means a onEvent(LocalClose) happens synchronously and rpcs are marked for deletion or data processing fully stopped because of StopIteration. Hopefully the comments help me or someone else the next time we need to debug something around handling local replies. Signed-off-by: Raul Gutierrez Segales --- .../network/thrift_proxy/conn_manager.cc | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/network/thrift_proxy/conn_manager.cc b/source/extensions/filters/network/thrift_proxy/conn_manager.cc index 7f7129715edf6..4d4b84f5b3930 100644 --- a/source/extensions/filters/network/thrift_proxy/conn_manager.cc +++ b/source/extensions/filters/network/thrift_proxy/conn_manager.cc @@ -323,12 +323,26 @@ FilterStatus ConnectionManager::ActiveRpc::applyDecoderFilters(ActiveRpcDecoderF for (; entry != decoder_filters_.end(); entry++) { const FilterStatus status = filter_action_((*entry)->handle_.get()); if (local_response_sent_) { - // The filter called sendLocalReply: stop processing filters and return - // FilterStatus::Continue irrespective of the current result. + // The filter called sendLocalReply but _did not_ close the connection. + // We return FilterStatus::Continue irrespective of the current result, + // which is fine because subsequent calls to this method will skip + // filters anyway. + // + // Note: we need to return FilterStatus::Continue here, in order for decoding + // to proceed. This is important because as noted above, the connection remains + // open so we need to consume the remaining bytes. break; } if (status != FilterStatus::Continue) { + // If we got FilterStatus::StopIteration and a local reply happend but + // local_response_sent_ was not set, the connection was closed. + // + // In this case, either resetAllRpcs() gets called via onEvent(LocalClose) or + // dispatch() stops the processing. + // + // In other words, after a local reply closes the connection and StopIteration + // is returned we are done. return status; } } From d51ffd9161655a75fec86fdf12fd73f599443eb2 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 19 Jan 2021 18:13:05 -0500 Subject: [PATCH 2/2] Fix typo Signed-off-by: Raul Gutierrez Segales --- source/extensions/filters/network/thrift_proxy/conn_manager.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/network/thrift_proxy/conn_manager.cc b/source/extensions/filters/network/thrift_proxy/conn_manager.cc index 4d4b84f5b3930..406fd1a8479bc 100644 --- a/source/extensions/filters/network/thrift_proxy/conn_manager.cc +++ b/source/extensions/filters/network/thrift_proxy/conn_manager.cc @@ -335,7 +335,7 @@ FilterStatus ConnectionManager::ActiveRpc::applyDecoderFilters(ActiveRpcDecoderF } if (status != FilterStatus::Continue) { - // If we got FilterStatus::StopIteration and a local reply happend but + // If we got FilterStatus::StopIteration and a local reply happened but // local_response_sent_ was not set, the connection was closed. // // In this case, either resetAllRpcs() gets called via onEvent(LocalClose) or