Skip to content

Commit

Permalink
thrift proxy: add comments explaining local replies
Browse files Browse the repository at this point in the history
While debugging envoyproxy#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 <rgs@pinterest.com>
  • Loading branch information
Raul Gutierrez Segales committed Jan 19, 2021
1 parent 610900e commit de806d8
Showing 1 changed file with 16 additions and 2 deletions.
18 changes: 16 additions & 2 deletions source/extensions/filters/network/thrift_proxy/conn_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down

0 comments on commit de806d8

Please sign in to comment.