diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index eb5ced4c201..6c29cd82180 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -1158,7 +1158,7 @@ static void maybe_restart_lb_call(glb_lb_policy* glb_policy) { glb_policy->updating_lb_call = false; } else if (!glb_policy->shutting_down) { /* if we aren't shutting down, restart the LB client call after some time */ - grpc_millis next_try = glb_policy->lb_call_backoff->Step(); + grpc_millis next_try = glb_policy->lb_call_backoff->NextAttemptTime(); if (grpc_lb_glb_trace.enabled()) { gpr_log(GPR_DEBUG, "[grpclb %p] Connection to LB server lost...", glb_policy); diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc index 7ea3cdd6e19..1efdc26d566 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc @@ -264,7 +264,7 @@ static void dns_ares_on_resolved_locked(void* arg, grpc_error* error) { } else { const char* msg = grpc_error_string(error); gpr_log(GPR_DEBUG, "dns resolution failed: %s", msg); - grpc_millis next_try = r->backoff->Step(); + grpc_millis next_try = r->backoff->NextAttemptTime(); grpc_millis timeout = next_try - grpc_core::ExecCtx::Get()->Now(); gpr_log(GPR_INFO, "dns resolution failed (will retry): %s", grpc_error_string(error)); diff --git a/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc b/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc index 93a1fe87a28..66a03c5a859 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc @@ -161,7 +161,7 @@ static void dns_on_resolved_locked(void* arg, grpc_error* error) { grpc_resolved_addresses_destroy(r->addresses); grpc_lb_addresses_destroy(addresses); } else { - grpc_millis next_try = r->backoff->Step(); + grpc_millis next_try = r->backoff->NextAttemptTime(); grpc_millis timeout = next_try - grpc_core::ExecCtx::Get()->Now(); gpr_log(GPR_INFO, "dns resolution failed (will retry): %s", grpc_error_string(error)); diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index fe4fcbbb7d3..bb43651d0c3 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -373,6 +373,7 @@ static void continue_connect_locked(grpc_subchannel* c) { args.interested_parties = c->pollset_set; const grpc_millis min_deadline = c->min_connect_timeout_ms + grpc_core::ExecCtx::Get()->Now(); + c->next_attempt_deadline = c->backoff->NextAttemptTime(); args.deadline = std::max(c->next_attempt_deadline, min_deadline); args.channel_args = c->args; grpc_connectivity_state_set(&c->state_tracker, GRPC_CHANNEL_CONNECTING, @@ -418,7 +419,6 @@ static void on_alarm(void* arg, grpc_error* error) { } if (error == GRPC_ERROR_NONE) { gpr_log(GPR_INFO, "Failed to connect to channel, retrying"); - c->next_attempt_deadline = c->backoff->Step(); continue_connect_locked(c); gpr_mu_unlock(&c->mu); } else { @@ -454,7 +454,6 @@ static void maybe_start_connecting_locked(grpc_subchannel* c) { if (!c->backoff_begun) { c->backoff_begun = true; - c->next_attempt_deadline = c->backoff->Begin(); continue_connect_locked(c); } else { GPR_ASSERT(!c->have_alarm); diff --git a/src/core/lib/backoff/backoff.cc b/src/core/lib/backoff/backoff.cc index 41f625a636c..d561fc7460d 100644 --- a/src/core/lib/backoff/backoff.cc +++ b/src/core/lib/backoff/backoff.cc @@ -41,18 +41,20 @@ double generate_uniform_random_number_between(uint32_t* rng_state, double a, const double range = b - a; return a + generate_uniform_random_number(rng_state) * range; } -} // namespace -BackOff::BackOff(const Options& options) : options_(options) { - rng_state_ = static_cast(gpr_now(GPR_CLOCK_REALTIME).tv_nsec); -} +} // namespace -grpc_millis BackOff::Begin() { - current_backoff_ = options_.initial_backoff(); - return current_backoff_ + grpc_core::ExecCtx::Get()->Now(); +BackOff::BackOff(const Options& options) + : options_(options), + rng_state_(static_cast(gpr_now(GPR_CLOCK_REALTIME).tv_nsec)) { + Reset(); } -grpc_millis BackOff::Step() { +grpc_millis BackOff::NextAttemptTime() { + if (initial_) { + initial_ = false; + return current_backoff_ + grpc_core::ExecCtx::Get()->Now(); + } current_backoff_ = (grpc_millis)(std::min(current_backoff_ * options_.multiplier(), (double)options_.max_backoff())); @@ -63,7 +65,10 @@ grpc_millis BackOff::Step() { return next_timeout + grpc_core::ExecCtx::Get()->Now(); } -void BackOff::Reset() { current_backoff_ = options_.initial_backoff(); } +void BackOff::Reset() { + current_backoff_ = options_.initial_backoff(); + initial_ = true; +} void BackOff::SetRandomSeed(uint32_t seed) { rng_state_ = seed; } diff --git a/src/core/lib/backoff/backoff.h b/src/core/lib/backoff/backoff.h index 84ef9b82e41..de30e5268b3 100644 --- a/src/core/lib/backoff/backoff.h +++ b/src/core/lib/backoff/backoff.h @@ -32,14 +32,11 @@ class BackOff { /// Initialize backoff machinery - does not need to be destroyed explicit BackOff(const Options& options); - /// Begin retry loop: returns the deadline to be used for the next attempt, - /// following the backoff strategy. - grpc_millis Begin(); - /// Step a retry loop: returns the deadline to be used for the next attempt, - /// following the backoff strategy. - grpc_millis Step(); - /// Reset the backoff, so the next grpc_backoff_step will be a - /// grpc_backoff_begin. + /// Returns the time at which the next attempt should start. + grpc_millis NextAttemptTime(); + + /// Reset the backoff, so the next value returned by NextAttemptTime() + /// will be the time of the second attempt (rather than the Nth). void Reset(); void SetRandomSeed(unsigned int seed); @@ -80,9 +77,10 @@ class BackOff { private: const Options options_; + uint32_t rng_state_; + bool initial_; /// current delay before retries grpc_millis current_backoff_; - uint32_t rng_state_; }; } // namespace grpc_core diff --git a/test/core/backoff/backoff_test.cc b/test/core/backoff/backoff_test.cc index 7bc4d14ce67..2e61243831a 100644 --- a/test/core/backoff/backoff_test.cc +++ b/test/core/backoff/backoff_test.cc @@ -45,11 +45,11 @@ TEST(BackOffTest, ConstantBackOff) { .set_max_backoff(max_backoff); BackOff backoff(options); - grpc_millis next_attempt_start_time = backoff.Begin(); + grpc_millis next_attempt_start_time = backoff.NextAttemptTime(); EXPECT_EQ(next_attempt_start_time - grpc_core::ExecCtx::Get()->Now(), initial_backoff); for (int i = 0; i < 10000; i++) { - next_attempt_start_time = backoff.Step(); + next_attempt_start_time = backoff.NextAttemptTime(); EXPECT_EQ(next_attempt_start_time - grpc_core::ExecCtx::Get()->Now(), initial_backoff); } @@ -67,7 +67,7 @@ TEST(BackOffTest, MinConnect) { .set_jitter(jitter) .set_max_backoff(max_backoff); BackOff backoff(options); - grpc_millis next = backoff.Begin(); + grpc_millis next = backoff.NextAttemptTime(); EXPECT_EQ(next - grpc_core::ExecCtx::Get()->Now(), initial_backoff); } @@ -86,42 +86,42 @@ TEST(BackOffTest, NoJitterBackOff) { // x_n = 2**i + x_{i-1} ( = 2**(n+1) - 2 ) grpc_core::ExecCtx exec_ctx; grpc_core::ExecCtx::Get()->TestOnlySetNow(0); - grpc_millis next = backoff.Begin(); + grpc_millis next = backoff.NextAttemptTime(); EXPECT_EQ(next, 2); grpc_core::ExecCtx::Get()->TestOnlySetNow(next); - next = backoff.Step(); + next = backoff.NextAttemptTime(); EXPECT_EQ(next, 6); grpc_core::ExecCtx::Get()->TestOnlySetNow(next); - next = backoff.Step(); + next = backoff.NextAttemptTime(); EXPECT_EQ(next, 14); grpc_core::ExecCtx::Get()->TestOnlySetNow(next); - next = backoff.Step(); + next = backoff.NextAttemptTime(); EXPECT_EQ(next, 30); grpc_core::ExecCtx::Get()->TestOnlySetNow(next); - next = backoff.Step(); + next = backoff.NextAttemptTime(); EXPECT_EQ(next, 62); grpc_core::ExecCtx::Get()->TestOnlySetNow(next); - next = backoff.Step(); + next = backoff.NextAttemptTime(); EXPECT_EQ(next, 126); grpc_core::ExecCtx::Get()->TestOnlySetNow(next); - next = backoff.Step(); + next = backoff.NextAttemptTime(); EXPECT_EQ(next, 254); grpc_core::ExecCtx::Get()->TestOnlySetNow(next); - next = backoff.Step(); + next = backoff.NextAttemptTime(); EXPECT_EQ(next, 510); grpc_core::ExecCtx::Get()->TestOnlySetNow(next); - next = backoff.Step(); + next = backoff.NextAttemptTime(); EXPECT_EQ(next, 1022); grpc_core::ExecCtx::Get()->TestOnlySetNow(next); - next = backoff.Step(); + next = backoff.NextAttemptTime(); // Hit the maximum timeout. From this point onwards, retries will increase // only by max timeout. EXPECT_EQ(next, 1535); grpc_core::ExecCtx::Get()->TestOnlySetNow(next); - next = backoff.Step(); + next = backoff.NextAttemptTime(); EXPECT_EQ(next, 2048); grpc_core::ExecCtx::Get()->TestOnlySetNow(next); - next = backoff.Step(); + next = backoff.NextAttemptTime(); EXPECT_EQ(next, 2561); } @@ -141,7 +141,7 @@ TEST(BackOffTest, JitterBackOff) { backoff.SetRandomSeed(0); // force consistent PRNG grpc_core::ExecCtx exec_ctx; - grpc_millis next = backoff.Begin(); + grpc_millis next = backoff.NextAttemptTime(); EXPECT_EQ(next - grpc_core::ExecCtx::Get()->Now(), initial_backoff); grpc_millis expected_next_lower_bound = @@ -150,7 +150,7 @@ TEST(BackOffTest, JitterBackOff) { (grpc_millis)((double)current_backoff * (1 + jitter)); for (int i = 0; i < 10000; i++) { - next = backoff.Step(); + next = backoff.NextAttemptTime(); // next-now must be within (jitter*100)% of the current backoff (which // increases by * multiplier up to max_backoff). const grpc_millis timeout_millis = next - grpc_core::ExecCtx::Get()->Now();