fix: correct exponential backoff ranges

google/cloud/internal/backoff_policy.cc

@@ -26,7 +26,7 @@ std::unique_ptr<BackoffPolicy> ExponentialBackoffPolicy::clone() const {
 
 std::chrono::milliseconds ExponentialBackoffPolicy::OnCompletion() {
   using std::chrono::duration_cast;
-  using std::chrono::microseconds;
+  using microseconds = std::chrono::duration<double, std::micro>;
   using std::chrono::milliseconds;
   // We do not want to copy the seed in `clone()` because then all operations
   // will have the same sequence of backoffs. Nor do we want to use a shared
@@ -41,16 +41,21 @@ std::chrono::milliseconds ExponentialBackoffPolicy::OnCompletion() {
   if (!generator_) {
     generator_ = google::cloud::internal::MakeDefaultPRNG();
   }
-  std::uniform_int_distribution<microseconds::rep> rng_distribution(
-      current_delay_range_.count() / 2, current_delay_range_.count());
+
+  if (current_delay_end_ >= maximum_delay_) {
+    current_delay_start_ = std::max(maximum_delay_ / scaling_, initial_delay_);
+    current_delay_end_ = maximum_delay_;
+  }
+
+  std::uniform_real_distribution<microseconds::rep> rng_distribution(
+      current_delay_start_.count(), current_delay_end_.count());
   // Randomized sleep period because it is possible that after some time all
   // client have same sleep period if we use only exponential backoff policy.
   auto delay = microseconds(rng_distribution(*generator_));
-  current_delay_range_ = microseconds(static_cast<microseconds::rep>(
-      static_cast<double>(current_delay_range_.count()) * scaling_));
-  if (current_delay_range_ >= maximum_delay_) {
-    current_delay_range_ = maximum_delay_;
-  }
+
+  current_delay_start_ = current_delay_end_;
+  current_delay_end_ *= scaling_;
+
   return duration_cast<milliseconds>(delay);
 }
 

google/cloud/internal/backoff_policy.h

@@ -101,7 +101,7 @@ class ExponentialBackoffPolicy : public BackoffPolicy {
    * @endcode
    *
    * @param initial_delay how long to wait after the first (unsuccessful)
-   *     operation.
+   *     operation and the minimum value for the delay between operations.
    * @param maximum_delay the maximum value for the delay between operations.
    * @param scaling how fast does the delay increase between iterations.
    *
@@ -126,12 +126,15 @@ class ExponentialBackoffPolicy : public BackoffPolicy {
   ExponentialBackoffPolicy(std::chrono::duration<Rep1, Period1> initial_delay,
                            std::chrono::duration<Rep2, Period2> maximum_delay,
                            double scaling)
-      : initial_delay_(std::chrono::duration_cast<std::chrono::microseconds>(
-            initial_delay)),
-        current_delay_range_(2 * initial_delay_),
-        maximum_delay_(std::chrono::duration_cast<std::chrono::microseconds>(
-            maximum_delay)),
-        scaling_(scaling) {
+      : initial_delay_(
+            std::chrono::duration_cast<
+                std::chrono::duration<double, std::micro>>(initial_delay)),
+        maximum_delay_(
+            std::chrono::duration_cast<
+                std::chrono::duration<double, std::micro>>(maximum_delay)),
+        scaling_(scaling),
+        current_delay_start_(initial_delay_),
+        current_delay_end_(scaling_ * initial_delay_) {
     if (scaling_ <= 1.0) {
       google::cloud::internal::ThrowInvalidArgument(
           "scaling factor must be > 1.0");
@@ -144,18 +147,20 @@ class ExponentialBackoffPolicy : public BackoffPolicy {
   //  - We want uncorrelated data streams for each copy anyway.
   ExponentialBackoffPolicy(ExponentialBackoffPolicy const& rhs) noexcept
       : initial_delay_(rhs.initial_delay_),
-        current_delay_range_(rhs.current_delay_range_),
         maximum_delay_(rhs.maximum_delay_),
-        scaling_(rhs.scaling_) {}
+        scaling_(rhs.scaling_),
+        current_delay_start_(rhs.current_delay_start_),
+        current_delay_end_(rhs.current_delay_end_) {}
 
   std::unique_ptr<BackoffPolicy> clone() const override;
   std::chrono::milliseconds OnCompletion() override;
 
  private:
-  std::chrono::microseconds initial_delay_;
-  std::chrono::microseconds current_delay_range_;
-  std::chrono::microseconds maximum_delay_;
+  std::chrono::duration<double, std::micro> initial_delay_;
+  std::chrono::duration<double, std::micro> maximum_delay_;
   double scaling_;
+  std::chrono::duration<double, std::micro> current_delay_start_;
+  std::chrono::duration<double, std::micro> current_delay_end_;
   absl::optional<DefaultPRNG> generator_;
 };
 

google/cloud/internal/backoff_policy_test.cc

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include "google/cloud/internal/backoff_policy.h"
+#include "google/cloud/testing_util/chrono_output.h"
 #include <gmock/gmock.h>
 #include <chrono>
 #include <vector>
@@ -41,6 +42,27 @@ TEST(ExponentialBackoffPolicy, Simple) {
   EXPECT_GE(ms(100), delay);
 }
 
+/// @test Verify the initial and maximum delay are respected.
+TEST(ExponentialBackoffPolicy, RespectMinimumAndMaximumDelay) {
+  ExponentialBackoffPolicy tested(ms(10), ms(12), 2.0);
+
+  auto delay = tested.OnCompletion();
+  EXPECT_LE(ms(10), delay);
+  EXPECT_GE(ms(12), delay);
+  delay = tested.OnCompletion();
+  EXPECT_LE(ms(10), delay);
+  EXPECT_GE(ms(12), delay);
+}
+
+/// @test Verify the delay range is determined by the scaling factor.
+TEST(ExponentialBackoffPolicy, DetermineRangeUsingScalingFactor) {
+  ExponentialBackoffPolicy tested(ms(1000), ms(2000), 1.001);
+
+  auto delay = tested.OnCompletion();
+  EXPECT_LE(ms(1000), delay);
+  EXPECT_GE(ms(1001), delay);
+}
+
 /// @test Verify that the scaling factor is validated.
 TEST(ExponentialBackoffPolicy, ValidateScaling) {
 #if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
@@ -62,13 +84,13 @@ TEST(ExponentialBackoffPolicy, DifferentParameters) {
 
   auto delay = tested.OnCompletion();
   EXPECT_LE(ms(100), delay) << "delay=" << delay.count() << "ms";
-  EXPECT_GE(ms(200), delay) << "delay=" << delay.count() << "ms";
+  EXPECT_GE(ms(150), delay) << "delay=" << delay.count() << "ms";
   delay = tested.OnCompletion();
   EXPECT_LE(ms(150), delay) << "delay=" << delay.count() << "ms";
-  EXPECT_GE(ms(300), delay) << "delay=" << delay.count() << "ms";
+  EXPECT_GE(ms(225), delay) << "delay=" << delay.count() << "ms";
   delay = tested.OnCompletion();
   EXPECT_LE(ms(225), delay) << "delay=" << delay.count() << "ms";
-  EXPECT_GE(ms(450), delay) << "delay=" << delay.count() << "ms";
+  EXPECT_GE(ms(338), delay) << "delay=" << delay.count() << "ms";
 }
 
 /// @test Test cloning for ExponentialBackoffPolicy.