Merge pull request #14070 from markdroth/backoff_api_cleanup

Combine BackOff Begin() and Step() methods.

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);

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));

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));

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);

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<uint32_t>(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<uint32_t>(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; }
 

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

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();