diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index e741ec56383dc..67d022cb043b5 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -1,5 +1,7 @@ #include "source/common/grpc/async_client_manager_impl.h" +#include + #include "envoy/config/core/v3/grpc_service.pb.h" #include "envoy/stats/scope.h" @@ -204,13 +206,18 @@ void AsyncClientManagerImpl::RawAsyncClientCache::evictEntriesAndResetEvictionTi // Evict all the entries that have expired. while (!lru_list_.empty()) { MonotonicTime next_expire = lru_list_.back().accessed_time_ + EntryTimeoutInterval; - if (now >= next_expire) { + std::chrono::seconds time_to_next_expire_sec = + std::chrono::duration_cast(next_expire - now); + // since 'now' and 'next_expire' are in nanoseconds, the following condition is to + // check if the difference between them is less than 1 second. If we don't do this, the + // timer will be enabled with 0 seconds, which will cause the timer to fire immediately. + // This will cause cpu spike. + if (time_to_next_expire_sec.count() <= 0) { // Erase the expired entry. lru_map_.erase(lru_list_.back().config_with_hash_key_); lru_list_.pop_back(); } else { - cache_eviction_timer_->enableTimer( - std::chrono::duration_cast(next_expire - now)); + cache_eviction_timer_->enableTimer(time_to_next_expire_sec); return; } } diff --git a/test/common/grpc/async_client_manager_impl_test.cc b/test/common/grpc/async_client_manager_impl_test.cc index 36dae3f1001e9..85eef1cfd94f4 100644 --- a/test/common/grpc/async_client_manager_impl_test.cc +++ b/test/common/grpc/async_client_manager_impl_test.cc @@ -1,3 +1,4 @@ +#include #include #include "envoy/config/core/v3/grpc_service.pb.h" @@ -111,6 +112,59 @@ TEST_F(RawAsyncClientCacheTest, GetExpiredButNotEvictedCacheEntry) { EXPECT_EQ(client_cache_.getCache(config_with_hash_key).get(), nullptr); } +class RawAsyncClientCacheTestBusyLoop : public testing::Test { +public: + RawAsyncClientCacheTestBusyLoop() { + timer_ = new Event::MockTimer(); + EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Invoke([this](Event::TimerCb) { + return timer_; + })); + client_cache_ = std::make_unique(dispatcher_); + EXPECT_CALL(*timer_, enableTimer(testing::Not(std::chrono::milliseconds(0)), _)) + .Times(testing::AtLeast(1)); + } + + void waitForMilliSeconds(int ms) { + for (int i = 0; i < ms; i++) { + time_system_.advanceTimeAndRun(std::chrono::milliseconds(1), dispatcher_, + Event::Dispatcher::RunType::NonBlock); + } + } + +protected: + Event::SimulatedTimeSystem time_system_; + NiceMock dispatcher_; + Event::MockTimer* timer_; + std::unique_ptr client_cache_; +}; + +TEST_F(RawAsyncClientCacheTestBusyLoop, MultipleCacheEntriesEvictionBusyLoop) { + envoy::config::core::v3::GrpcService grpc_service; + RawAsyncClientSharedPtr foo_client = std::make_shared(); + // two entries are added to the cache + for (int i = 1; i <= 2; i++) { + grpc_service.mutable_envoy_grpc()->set_cluster_name(std::to_string(i)); + GrpcServiceConfigWithHashKey config_with_hash_key(grpc_service); + client_cache_->setCache(config_with_hash_key, foo_client); + } + // waiting for 49.2 secs to make sure that for the entry which is not accessed, time to expire is + // less than 1 second, ~0.8 secs + waitForMilliSeconds(49200); + + // Access first cache entry to so that evictEntriesAndResetEvictionTimer() gets called. + // Since we are getting first entry, access time of first entry will be updated to current time. + grpc_service.mutable_envoy_grpc()->set_cluster_name(std::to_string(1)); + GrpcServiceConfigWithHashKey config_with_hash_key_1(grpc_service); + EXPECT_EQ(client_cache_->getCache(config_with_hash_key_1).get(), foo_client.get()); + + // Verifying that though the time to expire for second entry ~0.8 sec, it is considered as expired + // to avoid the busy loop which could happen if timer gets enabled with 0(0.8 rounded off to 0) + // duration. + grpc_service.mutable_envoy_grpc()->set_cluster_name(std::to_string(2)); + GrpcServiceConfigWithHashKey config_with_hash_key_2(grpc_service); + EXPECT_EQ(client_cache_->getCache(config_with_hash_key_2).get(), nullptr); +} + class AsyncClientManagerImplTest : public testing::Test { public: AsyncClientManagerImplTest()