fix(bigtable): polling policy clones initial state (#7854)

`bigtable::GenericPollingPolicy` now holds onto its prototype, and makes clones from it. So calling `.clone()` copies the initial state of the policy, not the current state.

google/cloud/bigtable/polling_policy.h

@@ -107,40 +107,54 @@ class PollingPolicy {
 template <typename Retry = LimitedTimeRetryPolicy,
           typename Backoff = ExponentialBackoffPolicy>
 class GenericPollingPolicy : public PollingPolicy {
+  static_assert(std::is_base_of<RPCRetryPolicy, Retry>::value,
+                "Retry must derive from RPCRetryPolicy");
+  static_assert(std::is_base_of<RPCBackoffPolicy, Backoff>::value,
+                "Backoff must derive from RPCBackoffPolicy");
+
  public:
   explicit GenericPollingPolicy(internal::RPCPolicyParameters defaults)
-      : rpc_retry_policy_(Retry(defaults)),
-        rpc_backoff_policy_(Backoff(defaults)) {}
+      : retry_prototype_(Retry(defaults)),
+        backoff_prototype_(Backoff(defaults)),
+        retry_clone_(retry_prototype_.clone()),
+        backoff_clone_(backoff_prototype_.clone()) {}
   GenericPollingPolicy(Retry retry, Backoff backoff)
-      : rpc_retry_policy_(std::move(retry)),
-        rpc_backoff_policy_(std::move(backoff)) {}
+      : retry_prototype_(std::move(retry)),
+        backoff_prototype_(std::move(backoff)),
+        retry_clone_(retry_prototype_.clone()),
+        backoff_clone_(backoff_prototype_.clone()) {}
 
   std::unique_ptr<PollingPolicy> clone() const override {
-    return std::unique_ptr<PollingPolicy>(new GenericPollingPolicy(*this));
+    return std::unique_ptr<PollingPolicy>(
+        new GenericPollingPolicy<Retry, Backoff>(retry_prototype_,
+                                                 backoff_prototype_));
   }
 
   void Setup(grpc::ClientContext& context) override {
-    rpc_retry_policy_.Setup(context);
-    rpc_backoff_policy_.Setup(context);
+    retry_clone_->Setup(context);
+    backoff_clone_->Setup(context);
   }
 
   bool IsPermanentError(google::cloud::Status const& status) override {
     return RPCRetryPolicy::IsPermanentFailure(status);
   }
 
   bool OnFailure(google::cloud::Status const& status) override {
-    return rpc_retry_policy_.OnFailure(status);
+    return retry_clone_->OnFailure(status);
   }
 
   bool Exhausted() override { return !OnFailure(google::cloud::Status()); }
 
   std::chrono::milliseconds WaitPeriod() override {
-    return rpc_backoff_policy_.OnCompletion(grpc::Status::OK);
+    return backoff_clone_->OnCompletion(grpc::Status::OK);
   }
 
  private:
-  Retry rpc_retry_policy_;
-  Backoff rpc_backoff_policy_;
+  Retry retry_prototype_;
+  Backoff backoff_prototype_;
+
+  std::unique_ptr<RPCRetryPolicy> retry_clone_;
+  std::unique_ptr<RPCBackoffPolicy> backoff_clone_;
 };
 
 std::unique_ptr<PollingPolicy> DefaultPollingPolicy(

google/cloud/bigtable/polling_policy_test.cc

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include "google/cloud/bigtable/polling_policy.h"
+#include "google/cloud/bigtable/rpc_retry_policy.h"
 #include "google/cloud/grpc_error_delegate.h"
 #include "google/cloud/status.h"
 #include "google/cloud/testing_util/check_predicate_becomes_false.h"
@@ -38,8 +39,6 @@ auto const kLimitedTimeTolerance = 10_ms;
 /**
  * @test Verify that a polling policy configured to run for 50ms
  * works correctly.
- *
- * This eliminates some amount of code duplication in the following tests.
  */
 void CheckLimitedTime(PollingPolicy& tested) {
   testing_util::CheckPredicateBecomesFalse(
@@ -61,11 +60,18 @@ TEST(GenericPollingPolicy, Simple) {
 
 /// @test Test cloning for LimitedTimeRetryPolicy.
 TEST(GenericPollingPolicy, Clone) {
-  LimitedTimeRetryPolicy retry(kLimitedTimeTestPeriod);
+  auto const transient = Status(StatusCode::kUnavailable, "try again");
+
+  LimitedErrorCountRetryPolicy retry(1);
   ExponentialBackoffPolicy backoff(internal::kBigtableLimits);
-  GenericPollingPolicy<> original(retry, backoff);
-  auto tested = original.clone();
-  CheckLimitedTime(*tested);
+  GenericPollingPolicy<LimitedErrorCountRetryPolicy> original(retry, backoff);
+  EXPECT_TRUE(original.OnFailure(transient));
+  EXPECT_FALSE(original.OnFailure(transient));
+
+  // Ensure the initial state of the policy is cloned, not the current state.
+  auto clone = original.clone();
+  EXPECT_TRUE(clone->OnFailure(transient));
+  EXPECT_FALSE(clone->OnFailure(transient));
 }
 
 /// @test Verify that non-retryable errors cause an immediate failure.
@@ -93,6 +99,19 @@ TEST(GenericPollingPolicy, IsPermanentError) {
       grpc::Status(grpc::StatusCode::UNAVAILABLE, "")));
 }
 
+/// @test Verify that the backoff policy's wait period is used.
+TEST(GenericPollingPolicy, WaitPeriod) {
+  LimitedTimeRetryPolicy retry(kLimitedTimeTestPeriod);
+  ExponentialBackoffPolicy backoff(10_ms, 50_ms);
+  GenericPollingPolicy<> tested(retry, backoff);
+  EXPECT_GE(10_ms, tested.WaitPeriod());
+  EXPECT_LE(10_ms, tested.WaitPeriod());
+
+  // Ensure the initial state of the policy is cloned, not the current state.
+  auto clone = tested.clone();
+  EXPECT_GE(10_ms, clone->WaitPeriod());
+}
+
 }  // anonymous namespace
 }  // namespace bigtable
 }  // namespace cloud