diff --git a/source/common/common/backoff_strategy.cc b/source/common/common/backoff_strategy.cc index 8431b36b73bbc..b8241436aa212 100644 --- a/source/common/common/backoff_strategy.cc +++ b/source/common/common/backoff_strategy.cc @@ -4,22 +4,25 @@ namespace Envoy { JitteredBackOffStrategy::JitteredBackOffStrategy(uint64_t base_interval, uint64_t max_interval, Runtime::RandomGenerator& random) - : base_interval_(base_interval), max_interval_(max_interval), random_(random) { + : base_interval_(base_interval), max_interval_(max_interval), next_interval_(base_interval), + random_(random) { ASSERT(base_interval_ > 0); ASSERT(base_interval_ <= max_interval_); } uint64_t JitteredBackOffStrategy::nextBackOffMs() { - const uint64_t multiplier = (1 << current_retry_) - 1; - const uint64_t base_backoff = multiplier * base_interval_; - if (base_backoff <= max_interval_) { - current_retry_++; + const uint64_t backoff = next_interval_; + ASSERT(backoff > 0); + // Set next_interval_ to max_interval_ if doubling the interval would exceed the max or overflow. + if (next_interval_ < max_interval_ / 2) { + next_interval_ *= 2; + } else { + next_interval_ = max_interval_; } - ASSERT(base_backoff > 0); - return std::min(random_.random() % base_backoff, max_interval_); + return std::min(random_.random() % backoff, max_interval_); } -void JitteredBackOffStrategy::reset() { current_retry_ = 1; } +void JitteredBackOffStrategy::reset() { next_interval_ = base_interval_; } FixedBackOffStrategy::FixedBackOffStrategy(uint64_t interval_ms) : interval_ms_(interval_ms) { ASSERT(interval_ms_ > 0); diff --git a/source/common/common/backoff_strategy.h b/source/common/common/backoff_strategy.h index ed6d9955f09f0..be84ec5e865e8 100644 --- a/source/common/common/backoff_strategy.h +++ b/source/common/common/backoff_strategy.h @@ -33,7 +33,7 @@ class JitteredBackOffStrategy : public BackOffStrategy { private: const uint64_t base_interval_; const uint64_t max_interval_{}; - uint64_t current_retry_{1}; + uint64_t next_interval_; Runtime::RandomGenerator& random_; }; diff --git a/test/common/common/backoff_strategy_test.cc b/test/common/common/backoff_strategy_test.cc index 9d7bc866ed87f..5db265e66bfa5 100644 --- a/test/common/common/backoff_strategy_test.cc +++ b/test/common/common/backoff_strategy_test.cc @@ -30,31 +30,52 @@ TEST(BackOffStrategyTest, JitteredBackOffBasicReset) { EXPECT_EQ(2, jittered_back_off.nextBackOffMs()); // Should start from start } +TEST(BackOffStrategyTest, JitteredBackOffDoesntOverflow) { + NiceMock random; + ON_CALL(random, random()).WillByDefault(Return(std::numeric_limits::max() - 1)); + + JitteredBackOffStrategy jittered_back_off(1, std::numeric_limits::max(), random); + for (int iter = 0; iter < 100; ++iter) { + EXPECT_GE(std::numeric_limits::max(), jittered_back_off.nextBackOffMs()); + } + EXPECT_EQ(std::numeric_limits::max() - 1, jittered_back_off.nextBackOffMs()); +} + TEST(BackOffStrategyTest, JitteredBackOffWithMaxInterval) { NiceMock random; - ON_CALL(random, random()).WillByDefault(Return(1024)); + ON_CALL(random, random()).WillByDefault(Return(9999)); JitteredBackOffStrategy jittered_back_off(5, 100, random); EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); - EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); EXPECT_EQ(9, jittered_back_off.nextBackOffMs()); - EXPECT_EQ(49, jittered_back_off.nextBackOffMs()); - EXPECT_EQ(94, jittered_back_off.nextBackOffMs()); - EXPECT_EQ(94, jittered_back_off.nextBackOffMs()); // Should return Max here + EXPECT_EQ(19, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(39, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(79, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(99, jittered_back_off.nextBackOffMs()); // Should return Max here + EXPECT_EQ(99, jittered_back_off.nextBackOffMs()); } TEST(BackOffStrategyTest, JitteredBackOffWithMaxIntervalReset) { NiceMock random; - ON_CALL(random, random()).WillByDefault(Return(1024)); + ON_CALL(random, random()).WillByDefault(Return(9999)); JitteredBackOffStrategy jittered_back_off(5, 100, random); EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); - EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); EXPECT_EQ(9, jittered_back_off.nextBackOffMs()); - EXPECT_EQ(49, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(19, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(39, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(79, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(99, jittered_back_off.nextBackOffMs()); // Should return Max here + EXPECT_EQ(99, jittered_back_off.nextBackOffMs()); jittered_back_off.reset(); - EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); // Should start from start + EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(9, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(19, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(39, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(79, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(99, jittered_back_off.nextBackOffMs()); // Should return Max here + EXPECT_EQ(99, jittered_back_off.nextBackOffMs()); } TEST(BackOffStrategyTest, FixedBackOffBasicReset) { diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index bedbb8b113453..676c87d8dbae9 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -834,21 +834,21 @@ TEST_F(RouterRetryStateImplTest, Backoff) { setup(request_headers); EXPECT_TRUE(state_->enabled()); - EXPECT_CALL(random_, random()).WillOnce(Return(49)); + EXPECT_CALL(random_, random()).WillOnce(Return(190)); retry_timer_ = new Event::MockTimer(&dispatcher_); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(24), _)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(15), _)); EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryReset(connect_failure_, callback_)); EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); - EXPECT_CALL(random_, random()).WillOnce(Return(149)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(74), _)); + EXPECT_CALL(random_, random()).WillOnce(Return(190)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(40), _)); EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryReset(connect_failure_, callback_)); EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); - EXPECT_CALL(random_, random()).WillOnce(Return(349)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(174), _)); + EXPECT_CALL(random_, random()).WillOnce(Return(190)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(90), _)); EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryReset(connect_failure_, callback_)); EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); @@ -879,19 +879,25 @@ TEST_F(RouterRetryStateImplTest, CustomBackOffInterval) { retry_timer_->invokeCallback(); EXPECT_CALL(random_, random()).WillOnce(Return(350)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(50), _)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(150), _)); EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryReset(connect_failure_, callback_)); EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); EXPECT_CALL(random_, random()).WillOnce(Return(751)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(51), _)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(351), _)); + EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryReset(connect_failure_, callback_)); + EXPECT_CALL(callback_ready_, ready()); + retry_timer_->invokeCallback(); + + EXPECT_CALL(random_, random()).WillOnce(Return(2399)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(799), _)); EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryReset(connect_failure_, callback_)); EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); - EXPECT_CALL(random_, random()).WillOnce(Return(1499)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(1200), _)); + EXPECT_CALL(random_, random()).WillOnce(Return(2399)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(1199), _)); EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryReset(connect_failure_, callback_)); EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); @@ -914,19 +920,25 @@ TEST_F(RouterRetryStateImplTest, CustomBackOffIntervalDefaultMax) { retry_timer_->invokeCallback(); EXPECT_CALL(random_, random()).WillOnce(Return(350)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(50), _)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(150), _)); EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryReset(connect_failure_, callback_)); EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); EXPECT_CALL(random_, random()).WillOnce(Return(751)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(51), _)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(351), _)); + EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryReset(connect_failure_, callback_)); + EXPECT_CALL(callback_ready_, ready()); + retry_timer_->invokeCallback(); + + EXPECT_CALL(random_, random()).WillOnce(Return(2999)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(599), _)); EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryReset(connect_failure_, callback_)); EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); - EXPECT_CALL(random_, random()).WillOnce(Return(1499)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(1000), _)); + EXPECT_CALL(random_, random()).WillOnce(Return(2999)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(999), _)); EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryReset(connect_failure_, callback_)); EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); diff --git a/test/common/upstream/hds_test.cc b/test/common/upstream/hds_test.cc index df7b3ad51aab6..b7a15887d05b5 100644 --- a/test/common/upstream/hds_test.cc +++ b/test/common/upstream/hds_test.cc @@ -285,12 +285,12 @@ TEST_F(HdsTest, TestStreamConnectionFailure) { .WillOnce(Return(nullptr)) .WillOnce(Return(&async_stream_)); - EXPECT_CALL(random_, random()).WillOnce(Return(1000005)).WillRepeatedly(Return(1234567)); + EXPECT_CALL(random_, random()).WillOnce(Return(1000005)).WillRepeatedly(Return(654321)); EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(5), _)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(1567), _)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(2567), _)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(4567), _)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(25567), _)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(321), _)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(2321), _)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(6321), _)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(14321), _)); EXPECT_CALL(async_stream_, sendMessageRaw_(_, _)); // Test connection failure and retry