Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure the pending requests are popped before the callback is called. #7843

Merged
merged 2 commits into from
Aug 7, 2019
Merged

Ensure the pending requests are popped before the callback is called. #7843

merged 2 commits into from
Aug 7, 2019

Conversation

HenryYYang
Copy link
Contributor

Description:
When the redis healthcheck failed, the host can be ejected inline of the onRespValue. Which will result in closing the connection and duplicated call to pop_front(), resulting in the following crash:

#0 0x0000000000e18d68 in typeinfo for Envoy::Event::TimerImpl ()
#1 0x000000000058aca0 in destroyEnvoy::Extensions::NetworkFilters::Common::Redis::Client::ClientImpl::PendingRequest (this=0x31702d8, __p=0x31702e8) at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/ext/new_allocator.h:140
#2 destroyEnvoy::Extensions::NetworkFilters::Common::Redis::Client::ClientImpl::PendingRequest (__a=..., __p=0x31702e8) at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/alloc_traits.h:487
#3 _M_erase (this=0x31702d8, __position=...) at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/stl_list.h:1815
#4 pop_front (this=0x31702d8) at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/stl_list.h:1104
#5 Envoy::Extensions::NetworkFilters::Common::Redis::Client::ClientImpl::onRespValue(std::unique_ptr<Envoy::Extensions::NetworkFilters::Common::Redis::RespValue, std::default_deleteEnvoy::Extensions::NetworkFilters::Common::Redis::RespValue >&&) (this=0x3170200,
value=) at external/envoy/source/extensions/filters/network/common/redis/client_impl.cc:187
#6 0x000000000058c00a in Envoy::Extensions::NetworkFilters::Common::Redis::DecoderImpl::parseSlice (this=0x2df6600, slice=...) at external/envoy/source/extensions/filters/network/common/redis/codec_impl.cc:400
#7 0x000000000058bf6b in Envoy::Extensions::NetworkFilters::Common::Redis::DecoderImpl::decode (this=0x2df6600, data=...) at external/envoy/source/extensions/filters/network/common/redis/codec_impl.cc:201
#8 0x000000000058a787 in Envoy::Extensions::NetworkFilters::Common::Redis::Client::ClientImpl::onData (this=0x3170200, data=...) at external/envoy/source/extensions/filters/network/common/redis/client_impl.cc:109
#9 0x000000000058b27d in Envoy::Extensions::NetworkFilters::Common::Redis::Client::ClientImpl::UpstreamReadFilter::onData (this=, data=...)
at bazel-out/k8-opt/bin/external/envoy/source/extensions/filters/network/common/redis/_virtual_includes/client_lib/extensions/filters/network/common/redis/client_impl.h:81
#10 0x0000000000695ac1 in Envoy::Network::FilterManagerImpl::onContinueReading (this=0x316f428, filter=, buffer_source=...) at external/envoy/source/common/network/filter_manager_impl.cc:65
#11 0x000000000069258c in onRead (this=, read_buffer_size=) at external/envoy/source/common/network/connection_impl.cc:269
#12 Envoy::Network::ConnectionImpl::onReadReady (this=0x316f400) at external/envoy/source/common/network/connection_impl.cc:513
#13 0x0000000000692051 in Envoy::Network::ConnectionImpl::onFileEvent (this=0x316f400, events=) at external/envoy/source/common/network/connection_impl.cc:489
#14 0x000000000068cd85 in operator() (this=0x31702e8, __args=3) at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/std_function.h:706
#15 operator() (this=, what=, arg=0x31702d8) at external/envoy/source/common/event/file_event_impl.cc:65
#16 Envoy::Event::FileEventImpl::assignEvents(unsigned int)::$_0::__invoke(int, short, void*) (what=, arg=0x31702d8) at external/envoy/source/common/event/file_event_impl.cc:49
#17 0x000000000099590d in event_persist_closure (base=, ev=) at /root/.cache/bazel/_bazel_root/e500e327e6981ab742dbacbb6723d21d/sandbox/processwrapper-sandbox/3/execroot/envoy/external/com_github_libevent_libevent/event.c:1645
#18 event_process_active_single_queue (base=0x2838000, activeq=0x272e360, max_to_process=2147483647, endtime=0x0)
at /root/.cache/bazel/_bazel_root/e500e327e6981ab742dbacbb6723d21d/sandbox/processwrapper-sandbox/3/execroot/envoy/external/com_github_libevent_libevent/event.c:1704
#19 0x0000000000993ec0 in event_process_active (base=) at /root/.cache/bazel/_bazel_root/e500e327e6981ab742dbacbb6723d21d/sandbox/processwrapper-sandbox/3/execroot/envoy/external/com_github_libevent_libevent/event.c:1805
#20 event_base_loop (base=0x2838000, flags=) at /root/.cache/bazel/_bazel_root/e500e327e6981ab742dbacbb6723d21d/sandbox/processwrapper-sandbox/3/execroot/envoy/external/com_github_libevent_libevent/event.c:2047
#21 0x000000000064a431 in Envoy::Server::InstanceImpl::run (this=0x27a7000) at external/envoy/source/server/server.cc:520
#22 0x0000000000463fa0 in Envoy::MainCommonBase::run (this=0x27960e8) at external/envoy/source/exe/main_common.cc:102
#23 0x0000000000462ef4 in run (this=0x2796000) at bazel-out/k8-opt/bin/external/envoy/source/exe/_virtual_includes/envoy_main_common_lib/exe/main_common.h:91
#24 main (argc=, argv=0x7ffffea3dc38) at external/envoy/source/exe/main.cc:39

Risk Level: Medium
Testing: Added unit tests
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]
Signed-off-by: Henry Yang hyang@lyft.com

@jmarantz jmarantz changed the title Ensure the pending requests are poped before the callback is called. Ensure the pending requests are popped before the callback is called. Aug 7, 2019
@mattklein123 mattklein123 self-assigned this Aug 7, 2019
mattklein123
mattklein123 requested changes Aug 7, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work Henry! Thanks, LGTM with some small nits.

/wait

source/extensions/filters/network/common/redis/client_impl.cc Outdated Show resolved Hide resolved
test/extensions/filters/network/common/redis/client_impl_test.cc
@@ -810,6 +810,60 @@ TEST_F(RedisClientImplTest, MovedRedirectionNotEnabled) {
client_->close();
}

TEST_F(RedisClientImplTest, RemoveFailedHealthCheck) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you add a small // comment on top of each new test that briefly describes what the test does since it's non-obvious?

@HenryYYang
Fix pr feedback
0c6e2ab 
Signed-off-by: Henry Yang <hyang@lyft.com>
mattklein123
mattklein123 approved these changes Aug 7, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattklein123 mattklein123 merged commit 7c7a6c9 into envoyproxy:master Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@jmarantz jmarantz jmarantz approved these changes

Assignees

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HenryYYang @jmarantz @mattklein123