fix(common): polling policy clones initial state

google/cloud/CMakeLists.txt

@@ -232,6 +232,7 @@ if (BUILD_TESTING)
         kms_key_name_test.cc
         log_test.cc
         options_test.cc
+        polling_policy_test.cc
         project_test.cc
         status_or_test.cc
         status_test.cc

google/cloud/google_cloud_cpp_common_unit_tests.bzl

@@ -50,6 +50,7 @@ google_cloud_cpp_common_unit_tests = [
     "kms_key_name_test.cc",
     "log_test.cc",
     "options_test.cc",
+    "polling_policy_test.cc",
     "project_test.cc",
     "status_or_test.cc",
     "status_test.cc",

google/cloud/polling_policy.h

@@ -15,6 +15,8 @@
 #ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_POLLING_POLICY_H
 #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_POLLING_POLICY_H
 
+#include "google/cloud/internal/backoff_policy.h"
+#include "google/cloud/internal/retry_policy.h"
 #include "google/cloud/status.h"
 #include "google/cloud/version.h"
 #include <chrono>
@@ -65,32 +67,51 @@ class PollingPolicy {
    * @return true if the failure should be treated as transient and the polling
    *     loop should continue.
    */
-  virtual bool OnFailure(google::cloud::Status const& status) = 0;
+  virtual bool OnFailure(Status const& status) = 0;
 
   /**
    * How long should the polling loop wait before trying again.
    */
   virtual std::chrono::milliseconds WaitPeriod() = 0;
 };
 
+/**
+ * Construct a polling policy from existing Retry and Backoff policies.
+ *
+ * A polling policy can be built by composing a retry and backoff policy. For
+ * example, to create a polling policy that "retries N times, waiting a fixed
+ * period between retries" you could compose the "try N times" retry policy with
+ * the "wait a fixed period between retries" backoff policy.
+ *
+ * This class makes it easier to create such composed polling policies.
+ *
+ * @tparam Retry the retry policy used to limit the number of errors or the
+ *     total duration of the polling loop.
+ * @tparam Backoff the backoff policy used to control how often the
+ *     library polls.
+ */
 template <typename Retry, typename Backoff>
 class GenericPollingPolicy : public PollingPolicy {
  public:
   GenericPollingPolicy(Retry retry_policy, Backoff backoff_policy)
-      : retry_policy_(std::move(retry_policy)),
-        backoff_policy_(std::move(backoff_policy)) {}
+      : retry_prototype_(std::move(retry_policy)),
+        backoff_prototype_(std::move(backoff_policy)),
+        retry_clone_(maybe_deref(retry_prototype_).clone()),
+        backoff_clone_(maybe_deref(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_));
   }
 
-  bool OnFailure(google::cloud::Status const& status) override {
-    return maybe_deref(retry_policy_).OnFailure(status);
+  bool OnFailure(Status const& status) override {
+    return retry_clone_->OnFailure(status);
   }
 
   std::chrono::milliseconds WaitPeriod() override {
-    return maybe_deref(backoff_policy_).OnCompletion();
+    return backoff_clone_->OnCompletion();
   }
   //@}
 
@@ -104,8 +125,11 @@ class GenericPollingPolicy : public PollingPolicy {
     return *v;
   }
 
-  Retry retry_policy_;
-  Backoff backoff_policy_;
+  Retry retry_prototype_;
+  Backoff backoff_prototype_;
+
+  std::unique_ptr<internal::RetryPolicy> retry_clone_;
+  std::unique_ptr<internal::BackoffPolicy> backoff_clone_;
 };
 
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END

google/cloud/polling_policy_test.cc

@@ -0,0 +1,154 @@
+// Copyright 2022 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "google/cloud/polling_policy.h"
+#include "google/cloud/testing_util/check_predicate_becomes_false.h"
+#include <gtest/gtest.h>
+#include <chrono>
+
+namespace google {
+namespace cloud {
+namespace {
+
+struct TestRetryablePolicy {
+  static bool IsPermanentFailure(google::cloud::Status const& s) {
+    return !s.ok() &&
+           (s.code() == google::cloud::StatusCode::kPermissionDenied);
+  }
+};
+
+Status TransientError() { return Status(StatusCode::kUnavailable, ""); }
+Status PermanentError() { return Status(StatusCode::kPermissionDenied, ""); }
+
+using ms = std::chrono::milliseconds;
+using RetryPolicyForTest =
+    ::google::cloud::internal::TraitBasedRetryPolicy<TestRetryablePolicy>;
+using LimitedTimeRetryPolicyForTest =
+    ::google::cloud::internal::LimitedTimeRetryPolicy<TestRetryablePolicy>;
+using LimitedErrorCountRetryPolicyForTest =
+    ::google::cloud::internal::LimitedErrorCountRetryPolicy<
+        TestRetryablePolicy>;
+
+auto const kLimitedTimeTestPeriod = ms(50);
+auto const kLimitedTimeTolerance = ms(10);
+
+/**
+ * @test Verify that a polling policy configured to run for 50ms works
+ * correctly.
+ */
+void CheckLimitedTime(PollingPolicy& tested) {
+  google::cloud::testing_util::CheckPredicateBecomesFalse(
+      [&tested] { return tested.OnFailure(TransientError()); },
+      std::chrono::system_clock::now() + kLimitedTimeTestPeriod,
+      kLimitedTimeTolerance);
+}
+
+/// @test A simple test for the GenericPollingPolicy.
+TEST(GenericPollingPolicy, Simple) {
+  LimitedTimeRetryPolicyForTest retry(kLimitedTimeTestPeriod);
+  internal::ExponentialBackoffPolicy backoff(ms(10), ms(100), 2.0);
+  GenericPollingPolicy<LimitedTimeRetryPolicyForTest,
+                       internal::ExponentialBackoffPolicy>
+      tested(retry, backoff);
+  CheckLimitedTime(tested);
+  auto delay = tested.WaitPeriod();
+  EXPECT_LE(ms(10), delay);
+  EXPECT_GE(ms(20), delay);
+}
+
+/// @test Ensure that we accept shared_ptr types.
+TEST(GenericPollingPolicy, SimpleWithPointers) {
+  LimitedTimeRetryPolicyForTest retry(kLimitedTimeTestPeriod);
+  internal::ExponentialBackoffPolicy backoff(ms(10), ms(100), 2.0);
+  GenericPollingPolicy<std::shared_ptr<RetryPolicyForTest>,
+                       std::shared_ptr<internal::BackoffPolicy>>
+      tested(retry.clone(), backoff.clone());
+  CheckLimitedTime(tested);
+  auto delay = tested.WaitPeriod();
+  EXPECT_LE(ms(10), delay);
+  EXPECT_GE(ms(20), delay);
+}
+
+/// @test Test cloning for GenericPollingPolicy.
+TEST(GenericPollingPolicy, Clone) {
+  LimitedErrorCountRetryPolicyForTest retry(1);
+  internal::ExponentialBackoffPolicy backoff(ms(10), ms(100), 2.0);
+  GenericPollingPolicy<LimitedErrorCountRetryPolicyForTest,
+                       internal::ExponentialBackoffPolicy>
+      original(retry, backoff);
+  EXPECT_TRUE(original.OnFailure(TransientError()));
+  EXPECT_FALSE(original.OnFailure(TransientError()));
+  auto delay = original.WaitPeriod();
+  EXPECT_LE(ms(10), delay);
+  EXPECT_GE(ms(20), delay);
+
+  // Ensure the initial state of the policy is cloned, not the current state.
+  auto clone = original.clone();
+  EXPECT_TRUE(clone->OnFailure(TransientError()));
+  EXPECT_FALSE(clone->OnFailure(TransientError()));
+  delay = clone->WaitPeriod();
+  EXPECT_LE(ms(10), delay);
+  EXPECT_GE(ms(20), delay);
+}
+
+/// @test Test cloning for GenericPollingPolicy, with shared_ptr inputs.
+TEST(GenericPollingPolicy, CloneWithPointers) {
+  LimitedErrorCountRetryPolicyForTest retry(1);
+  internal::ExponentialBackoffPolicy backoff(ms(10), ms(100), 2.0);
+  GenericPollingPolicy<std::shared_ptr<RetryPolicyForTest>,
+                       std::shared_ptr<internal::BackoffPolicy>>
+      original(retry.clone(), backoff.clone());
+  EXPECT_TRUE(original.OnFailure(TransientError()));
+  EXPECT_FALSE(original.OnFailure(TransientError()));
+  auto delay = original.WaitPeriod();
+  EXPECT_LE(ms(10), delay);
+  EXPECT_GE(ms(20), delay);
+
+  // Ensure the initial state of the policy is cloned, not the current state.
+  auto clone = original.clone();
+  EXPECT_TRUE(clone->OnFailure(TransientError()));
+  EXPECT_FALSE(clone->OnFailure(TransientError()));
+  delay = clone->WaitPeriod();
+  EXPECT_LE(ms(10), delay);
+  EXPECT_GE(ms(20), delay);
+}
+
+/// @test Verify that non-retryable errors cause an immediate failure.
+TEST(GenericPollingPolicy, OnNonRetryable) {
+  LimitedTimeRetryPolicyForTest retry(kLimitedTimeTestPeriod);
+  internal::ExponentialBackoffPolicy backoff(ms(10), ms(100), 2.0);
+  GenericPollingPolicy<LimitedTimeRetryPolicyForTest,
+                       internal::ExponentialBackoffPolicy>
+      tested(retry, backoff);
+  EXPECT_FALSE(tested.OnFailure(PermanentError()));
+}
+
+/// @test Verify that the backoff policy's wait period is used.
+TEST(GenericPollingPolicy, WaitPeriod) {
+  LimitedTimeRetryPolicyForTest retry(kLimitedTimeTestPeriod);
+  internal::ExponentialBackoffPolicy backoff(ms(10), ms(100), 2.0);
+  GenericPollingPolicy<LimitedTimeRetryPolicyForTest,
+                       internal::ExponentialBackoffPolicy>
+      tested(retry, backoff);
+  auto delay = tested.WaitPeriod();
+  EXPECT_LE(ms(10), delay);
+  EXPECT_GE(ms(20), delay);
+  delay = tested.WaitPeriod();
+  EXPECT_LE(ms(20), delay);
+  EXPECT_GE(ms(40), delay);
+}
+
+}  // namespace
+}  // namespace cloud
+}  // namespace google