From b1cade4c8b7792546fb075c2ea83cbb3321ec73c Mon Sep 17 00:00:00 2001 From: Henry Yang Date: Tue, 6 Aug 2019 16:45:03 -0700 Subject: [PATCH 1/2] Ensure the pending requests are poped before the callback is called. Signed-off-by: Henry Yang --- .../network/common/redis/client_impl.cc | 15 +++--- .../network/common/redis/client_impl_test.cc | 54 +++++++++++++++++++ 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc index 20bc5f02079a..547e622bbb7e 100644 --- a/source/extensions/filters/network/common/redis/client_impl.cc +++ b/source/extensions/filters/network/common/redis/client_impl.cc @@ -160,15 +160,20 @@ void ClientImpl::onEvent(Network::ConnectionEvent event) { void ClientImpl::onRespValue(RespValuePtr&& value) { ASSERT(!pending_requests_.empty()); PendingRequest& request = pending_requests_.front(); + bool canceled = request.canceled_; + PoolCallbacks& callbacks = request.callbacks_; - if (request.canceled_) { + // We need to ensure the request is pop before calling the callbacks, since the callbacks might + // result in closing the connection. + pending_requests_.pop_front(); + if (canceled) { host_->cluster().stats().upstream_rq_cancelled_.inc(); } else if (config_.enableRedirection() && (value->type() == Common::Redis::RespType::Error)) { std::vector err = StringUtil::splitToken(value->asString(), " ", false); bool redirected = false; if (err.size() == 3) { if (err[0] == RedirectionResponse::get().MOVED || err[0] == RedirectionResponse::get().ASK) { - redirected = request.callbacks_.onRedirection(*value); + redirected = callbacks.onRedirection(*value); if (redirected) { host_->cluster().stats().upstream_internal_redirect_succeeded_total_.inc(); } else { @@ -177,14 +182,12 @@ void ClientImpl::onRespValue(RespValuePtr&& value) { } } if (!redirected) { - request.callbacks_.onResponse(std::move(value)); + callbacks.onResponse(std::move(value)); } } else { - request.callbacks_.onResponse(std::move(value)); + callbacks.onResponse(std::move(value)); } - pending_requests_.pop_front(); - // If there are no remaining ops in the pipeline we need to disable the timer. // Otherwise we boost the timer since we are receiving responses and there are more to flush // out. diff --git a/test/extensions/filters/network/common/redis/client_impl_test.cc b/test/extensions/filters/network/common/redis/client_impl_test.cc index fd7d9ec9b7e5..a82b0d7ab907 100644 --- a/test/extensions/filters/network/common/redis/client_impl_test.cc +++ b/test/extensions/filters/network/common/redis/client_impl_test.cc @@ -810,6 +810,60 @@ TEST_F(RedisClientImplTest, MovedRedirectionNotEnabled) { client_->close(); } +TEST_F(RedisClientImplTest, RemoveFailedHealthCheck) { + InSequence s; + + setup(); + + Common::Redis::RespValue request1; + MockPoolCallbacks callbacks1; + EXPECT_CALL(*encoder_, encode(Ref(request1), _)); + EXPECT_CALL(*flush_timer_, enabled()).WillOnce(Return(false)); + PoolRequest* handle1 = client_->makeRequest(request1, callbacks1); + EXPECT_NE(nullptr, handle1); + + onConnected(); + + Common::Redis::RespValuePtr response1(new Common::Redis::RespValue()); + // Each call should result in either onResponse or onFailure, never both. + EXPECT_CALL(callbacks1, onFailure()).Times(0); + EXPECT_CALL(callbacks1, onResponse_(Ref(response1))) + .WillOnce(Invoke([&](Common::Redis::RespValuePtr&) { + // The health checker might fail the active health check based on the response content, and + // result in removing the host and closing the client. + client_->close(); + })); + EXPECT_CALL(*connect_or_op_timer_, disableTimer()).Times(2); + EXPECT_CALL(host_->outlier_detector_, + putResult(Upstream::Outlier::Result::EXT_ORIGIN_REQUEST_SUCCESS, _)); + callbacks_->onRespValue(std::move(response1)); +} + +TEST_F(RedisClientImplTest, RemoveFailedHost) { + InSequence s; + + setup(); + + NiceMock connection_callbacks; + client_->addConnectionCallbacks(connection_callbacks); + + Common::Redis::RespValue request1; + MockPoolCallbacks callbacks1; + EXPECT_CALL(*encoder_, encode(Ref(request1), _)); + EXPECT_CALL(*flush_timer_, enabled()).WillOnce(Return(false)); + PoolRequest* handle1 = client_->makeRequest(request1, callbacks1); + EXPECT_NE(nullptr, handle1); + + onConnected(); + + EXPECT_CALL(host_->outlier_detector_, + putResult(Upstream::Outlier::Result::LOCAL_ORIGIN_CONNECT_FAILED, _)); + EXPECT_CALL(callbacks1, onFailure()).WillOnce(Invoke([&]() { client_->close(); })); + EXPECT_CALL(*connect_or_op_timer_, disableTimer()); + EXPECT_CALL(connection_callbacks, onEvent(Network::ConnectionEvent::RemoteClose)); + upstream_connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); +} + TEST(RedisClientFactoryImplTest, Basic) { ClientFactoryImpl factory; Upstream::MockHost::MockCreateConnectionData conn_info; From 0c6e2ab881f8b0fa1a3adc7e865190ba7580b11f Mon Sep 17 00:00:00 2001 From: Henry Yang Date: Wed, 7 Aug 2019 12:10:42 -0700 Subject: [PATCH 2/2] Fix pr feedback Signed-off-by: Henry Yang --- source/extensions/filters/network/common/redis/client_impl.cc | 4 ++-- .../filters/network/common/redis/client_impl_test.cc | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc index 547e622bbb7e..192177dca7a1 100644 --- a/source/extensions/filters/network/common/redis/client_impl.cc +++ b/source/extensions/filters/network/common/redis/client_impl.cc @@ -160,10 +160,10 @@ void ClientImpl::onEvent(Network::ConnectionEvent event) { void ClientImpl::onRespValue(RespValuePtr&& value) { ASSERT(!pending_requests_.empty()); PendingRequest& request = pending_requests_.front(); - bool canceled = request.canceled_; + const bool canceled = request.canceled_; PoolCallbacks& callbacks = request.callbacks_; - // We need to ensure the request is pop before calling the callbacks, since the callbacks might + // We need to ensure the request is popped before calling the callback, since the callback might // result in closing the connection. pending_requests_.pop_front(); if (canceled) { diff --git a/test/extensions/filters/network/common/redis/client_impl_test.cc b/test/extensions/filters/network/common/redis/client_impl_test.cc index a82b0d7ab907..064c740dacc6 100644 --- a/test/extensions/filters/network/common/redis/client_impl_test.cc +++ b/test/extensions/filters/network/common/redis/client_impl_test.cc @@ -811,6 +811,8 @@ TEST_F(RedisClientImplTest, MovedRedirectionNotEnabled) { } TEST_F(RedisClientImplTest, RemoveFailedHealthCheck) { + // This test simulates a health check response signaling traffic should be drained from the host. + // As a result, the health checker will close the client in the call back. InSequence s; setup(); @@ -840,6 +842,8 @@ TEST_F(RedisClientImplTest, RemoveFailedHealthCheck) { } TEST_F(RedisClientImplTest, RemoveFailedHost) { + // This test simulates a health check request failed due to remote host closing the connection. + // As a result the health checker will close the client in the call back. InSequence s; setup();