fix(common): backoff policy clones initial state (#7696)

When calling .clone() on an exponential backoff policy, copy the initial state instead of the current state. This may result in a change of behavior, but we see it as a bug that ought to be fixed.

CHANGELOG.md

@@ -90,6 +90,12 @@
 
 ## v1.35.0 - TBD
 
+### [Common Libraries](https://github.com/googleapis/google-cloud-cpp/blob/main/google/cloud/README.md)
+
+* Backoff policies are now cloned from their initial state, instead of their
+  current state. Any accumulated delay will be reset to its initial value in the
+  clone. The previous behavior was a bug, and thus it has been fixed. ([#7696](https://github.com/googleapis/google-cloud-cpp/pull/7696))
+
 ## v1.34.0 - 2021-12
 
 ### [BigQuery](https://github.com/googleapis/google-cloud-cpp/blob/main/google/cloud/bigquery/README.md) [IAM](https://github.com/googleapis/google-cloud-cpp/blob/main/google/cloud/iam/README.md)

google/cloud/bigtable/rpc_backoff_policy.cc

@@ -30,7 +30,8 @@ ExponentialBackoffPolicy::ExponentialBackoffPolicy(
 }
 
 std::unique_ptr<RPCBackoffPolicy> ExponentialBackoffPolicy::clone() const {
-  return std::unique_ptr<RPCBackoffPolicy>(new ExponentialBackoffPolicy(*this));
+  return std::unique_ptr<RPCBackoffPolicy>(
+      new ExponentialBackoffPolicy(initial_delay_, maximum_delay_));
 }
 
 void ExponentialBackoffPolicy::Setup(grpc::ClientContext&) const {}

google/cloud/bigtable/rpc_backoff_policy.h

@@ -87,9 +87,14 @@ class ExponentialBackoffPolicy : public RPCBackoffPolicy {
  public:
   // NOLINTNEXTLINE(google-explicit-constructor)
   ExponentialBackoffPolicy(internal::RPCPolicyParameters defaults);
-  template <typename DurationT1, typename DurationT2>
-  ExponentialBackoffPolicy(DurationT1 initial_delay, DurationT2 maximum_delay)
-      : impl_(initial_delay / 2, maximum_delay, 2.0) {}
+  template <typename Rep1, typename Period1, typename Rep2, typename Period2>
+  ExponentialBackoffPolicy(std::chrono::duration<Rep1, Period1> initial_delay,
+                           std::chrono::duration<Rep2, Period2> maximum_delay)
+      : initial_delay_(std::chrono::duration_cast<std::chrono::microseconds>(
+            initial_delay)),
+        maximum_delay_(std::chrono::duration_cast<std::chrono::microseconds>(
+            maximum_delay)),
+        impl_(initial_delay_ / 2, maximum_delay_, 2.0) {}
 
   std::unique_ptr<RPCBackoffPolicy> clone() const override;
   void Setup(grpc::ClientContext& context) const override;
@@ -99,6 +104,9 @@ class ExponentialBackoffPolicy : public RPCBackoffPolicy {
   std::chrono::milliseconds OnCompletion(grpc::Status const& status) override;
 
  private:
+  std::chrono::microseconds initial_delay_;
+  std::chrono::microseconds maximum_delay_;
+
   using Impl = ::google::cloud::internal::ExponentialBackoffPolicy;
   Impl impl_;
 };

google/cloud/bigtable/rpc_backoff_policy_test.cc

@@ -52,6 +52,10 @@ TEST(ExponentialBackoffRetryPolicy, Clone) {
 
   EXPECT_GE(10_ms, tested->OnCompletion(CreateTransientError()));
   EXPECT_LE(10_ms, tested->OnCompletion(CreateTransientError()));
+
+  // Ensure the initial state of the policy is cloned, not the current state.
+  tested = tested->clone();
+  EXPECT_GE(10_ms, tested->OnCompletion(CreateTransientError()));
 }
 
 /// @test Test for testing randomness for 2 objects of

google/cloud/internal/backoff_policy.cc

@@ -21,7 +21,8 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 namespace internal {
 
 std::unique_ptr<BackoffPolicy> ExponentialBackoffPolicy::clone() const {
-  return absl::make_unique<ExponentialBackoffPolicy>(*this);
+  return absl::make_unique<ExponentialBackoffPolicy>(initial_delay_,
+                                                     maximum_delay_, scaling_);
 }
 
 std::chrono::milliseconds ExponentialBackoffPolicy::OnCompletion() {

google/cloud/internal/backoff_policy.h

@@ -126,9 +126,9 @@ class ExponentialBackoffPolicy : public BackoffPolicy {
   ExponentialBackoffPolicy(std::chrono::duration<Rep1, Period1> initial_delay,
                            std::chrono::duration<Rep2, Period2> maximum_delay,
                            double scaling)
-      : current_delay_range_(
-            std::chrono::duration_cast<std::chrono::microseconds>(
-                2 * initial_delay)),
+      : 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) {
@@ -143,14 +143,16 @@ class ExponentialBackoffPolicy : public BackoffPolicy {
   //    know specifically which one is at fault)
   //  - We want uncorrelated data streams for each copy anyway.
   ExponentialBackoffPolicy(ExponentialBackoffPolicy const& rhs) noexcept
-      : current_delay_range_(rhs.current_delay_range_),
+      : initial_delay_(rhs.initial_delay_),
+        current_delay_range_(rhs.current_delay_range_),
         maximum_delay_(rhs.maximum_delay_),
         scaling_(rhs.scaling_) {}
 
   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_;
   double scaling_;

google/cloud/internal/backoff_policy_test.cc

@@ -88,6 +88,12 @@ TEST(ExponentialBackoffPolicy, Clone) {
   delay = tested->OnCompletion();
   EXPECT_LE(ms(25), delay);
   EXPECT_GE(ms(50), delay);
+
+  // Ensure the initial state of the policy is cloned, not the current state.
+  tested = tested->clone();
+  delay = tested->OnCompletion();
+  EXPECT_LE(ms(10), delay);
+  EXPECT_GE(ms(20), delay);
 }
 
 /// @test Test for testing randomness for 2 objects of