Fix premature termination of transaction client

[asheryerm]

Signed-off-by: asheryer asheryer@amazon.com

This commit fixes the issue #23651
found by @untitaker

When pipelining a transaction the transaction connection terminates early which causes a segmentation fault.
I fixed this by checking that there are no more pending requests before closing the transaction.
I've tested this with several different scenarios and it works well.

Transactions can be pipelined now through envoy:

$printf '*1\r\n$5\r\nMULTI\r\n*3\r\n$3\r\nset\r\n$4\r\nfoo1\r\n$4\r\nbar1\r\n*2\r\n$3\r\nGET\r\n$4\r\nfoo1\r\n*1\r\n$4\r\nEXEC\r\n' | nc -w1 localhost 1998
+OK
+QUEUED
+QUEUED
*2
+OK
$4
bar1

Commit Message: Fix premature termination of transaction client
Additional Description: When transactions are pipelined the transaction client terminates early and this causes a segmentation fault.
Risk Level: Low
Testing: Local redis cluster
Docs Changes: Not needed
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] #23651
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

[mattklein123]

Please add a crashing test? Thank you.

/wait

[asheryerm]

Please add a crashing test? Thank you.

Do you mean as a unit test?

[mattklein123]

Do you mean as a unit test?

Either a unit or integration test that crashes without the fix.

[asheryerm]

Do you mean as a unit test?

Either a unit or integration test that crashes without the fix.

I think that the current testing infrastructure is not capable of reproducing this scenario. This would require a dedicated uplink connection that closes before all the pipelined responses are sent downstream. I could be wrong though, because I don't have a full understanding of the testing infra in envoy. Is there another test/s that I could use as a reference?

[mattklein123]

I think that the current testing infrastructure is not capable of reproducing this scenario. This would require a dedicated uplink connection that closes before all the pipelined responses are sent downstream. I could be wrong though, because I don't have a full understanding of the testing infra in envoy. Is there another test/s that I could use as a reference?

I'm quite sure you can reproduce this with an integration test. See https://github.com/envoyproxy/envoy/blob/main/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc

[mattklein123]

(Note that if we can't quickly get this fixed/merged with a test we can revert the original PR while it gets sorted out. Thank you.)

[asheryerm]

@mattklein123 I've managed to successfully add a crash test (PipelinedTransactionTest).
This is the test output without the fix:

[       OK ] IpVersions/RedisProxyWithCommandStatsIntegrationTest.SendMultiBeforeCommandInTransaction/IPv6 (48 ms)
[ RUN      ] IpVersions/RedisProxyWithCommandStatsIntegrationTest.PipelinedTransactionTest/IPv4
[2022-10-29 14:29:59.438][770][critical][assert] [source/common/stats/symbol_table.cc:487] assert failure: search != decode_map_.end(). Details: no such symbol
[2022-10-29 14:29:59.438][770][critical][backtrace] [./source/server/backtrace.h:104] Caught Aborted, suspect faulting address 0xfffe00000010
[2022-10-29 14:29:59.438][770][critical][backtrace] [./source/server/backtrace.h:91] Backtrace (use tools/stack_decode.py to get line numbers):
[2022-10-29 14:29:59.439][770][critical][backtrace] [./source/server/backtrace.h:92] Envoy version: 0/1.25.0-dev/test/RELEASE/BoringSSL
[2022-10-29 14:29:59.439][770][critical][backtrace] [./source/server/backtrace.h:96] #0: __restore_rt [0x7f8dffeb1420]
[2022-10-29 14:29:59.442][770][critical][backtrace] [./source/server/backtrace.h:96] #1: std::__1::__function::__func<>::operator()() [0x190cdc3]
[2022-10-29 14:29:59.446][770][critical][backtrace] [./source/server/backtrace.h:96] #2: Envoy::Stats::SymbolTable::Encoding::decodeTokens() [0x19091e9]
[2022-10-29 14:29:59.449][770][critical][backtrace] [./source/server/backtrace.h:96] #3: Envoy::Stats::SymbolTable::decodeStrings() [0x1909587]
[2022-10-29 14:29:59.453][770][critical][backtrace] [./source/server/backtrace.h:96] #4: Envoy::Stats::SymbolTable::toString() [0x1909db8]
[2022-10-29 14:29:59.456][770][critical][backtrace] [./source/server/backtrace.h:96] #5: Envoy::Stats::StatNameTagHelper::StatNameTagHelper() [0xdefa47]
[2022-10-29 14:29:59.459][770][critical][backtrace] [./source/server/backtrace.h:96] #6: Envoy::Stats::ThreadLocalStoreImpl::ScopeImpl::safeMakeStat<>() [0xdee351]
[2022-10-29 14:29:59.462][770][critical][backtrace] [./source/server/backtrace.h:96] #7: Envoy::Stats::ThreadLocalStoreImpl::ScopeImpl::counterFromStatNameWithTags() [0xdee077]
[2022-10-29 14:29:59.466][770][critical][backtrace] [./source/server/backtrace.h:96] #8: Envoy::Stats::Utility::counterFromStatNames() [0x190841e]
[2022-10-29 14:29:59.469][770][critical][backtrace] [./source/server/backtrace.h:96] #9: Envoy::Extensions::NetworkFilters::Common::Redis::RedisCommandStats::updateStats() [0xaf6d2a]
[2022-10-29 14:29:59.472][770][critical][backtrace] [./source/server/backtrace.h:96] #10: Envoy::Extensions::NetworkFilters::Common::Redis::Client::ClientImpl::onRespValue() [0xaebcc0]
[2022-10-29 14:29:59.476][770][critical][backtrace] [./source/server/backtrace.h:96] #11: Envoy::Extensions::NetworkFilters::Common::Redis::DecoderImpl::parseSlice() [0xaf44b4]
[2022-10-29 14:29:59.479][770][critical][backtrace] [./source/server/backtrace.h:96] #12: Envoy::Extensions::NetworkFilters::Common::Redis::DecoderImpl::decode() [0xaf352b]

[mattklein123]

Awesome, thank you!