thrift_proxy: fix crash when remote closes the connection

[rgs1]

There's a few paths within the Thrift Proxy where we should ensure
the connection is not closed, before trying to write. This change
ensures that sendLocalReply() will return early if the connection
is gone.

It also adds a check for transformEnd(), which gets called from
upstreamData().

Risk Level: low
Testing: unit tests added
Fixes: #6496

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

[rgs1]

Will follow-up with tests in a bit.

[zuercher]

@rgs1 this looks correct to me -- are you going to add a test for the ConnectionManager::ResponseDecoder::transportEnd case as well?

[rgs1]

@rgs1 this looks correct to me -- are you going to add a test for the ConnectionManager::ResponseDecoder::transportEnd case as well?

Yup, yup. For the first unit test tho, I am still figuring out why cx_destroy_remote_with_active_rq isn't increased... I'll dig in a bit more.

[zuercher]

I believe cx_destroy_remote_with_active_rq isn't increased because ConnectionManager receives a full request and thinks it's transmitted a response:

• ActiveRpc::sendLocalReply sets local_response_sent_ indicating that we shouldn't apply the filter chain to the remainder of the downstream request.
• ActiveRpc::transportEnd is invoked when the end of processing the downstream's request is reached
• ActiveRpc::finalizeRequest is called by transportEnd and detects local_response_sent_ is true and sets up deferred destroy for the active rpc.
• ConnectionManager::onData invokes resetAllRpcs because of the end of stream, but there are no active RPCs so it doesn't increment the counter.

If the downstream request was partial (e.g. if the buffer didn't have a complete RPC message in it), ActiveRpc::transportEnd would not be called and ConnectionManager::resetAllRpcs would find incomplete RPCs and increment the counter.

But, small requests will fit in a buffer so this isn't just an artifact of the test. If you really want to count these as cx_destroy_remote_with_active_rq, I think you'd want to switch ActiveRpc::local_response_sent_ to be an enum with 3 values (not sent, sent successfully, send failed) and arrange for ConnectionManager::sendLocalReply to report the send success/failed to ActiveRpc::sendLocalReply so it can set local_response_sent_ correctly. Then in ActiveRpc::applyDecoderFilters you could return FilterStatus::Continue if local_response_sent_ indicates success and FilterStatus::StopIteration on failure. I believe that would cause ConnectionManager::onData to detect an incomplete RPC with closed connection and trigger the counter to be incremented.

[rgs1]

@zuercher thanks for the detailed explanation. I am not that crazy about another counter for this special-ish case... In the meanwhile, I'll remove my comment about that counter not being incremented.

@fishcakez @derekargueta thoughts?

[fishcakez]

I'm not sure we need to differentiate at that level if it adds complexity, and the current behavior around having seen transportEnd makes good sense. I left a couple of nits for other metrics though.

[fishcakez]

This looks good to me, can we merge it @zuercher ?

[zuercher]

Look good. Thanks for picking this up.