Add RetryPolicy to PreviewsProber

Adds retry functionality with
* random guid on every URL
* linear and exponential backoff

Bug: 977603
Change-Id: I02ba45ea578e75a1a750b8f42d77959edb1fe187
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677369Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672608}

chrome/browser/previews/previews_prober.cc

@@ -4,6 +4,11 @@
 
 #include "chrome/browser/previews/previews_prober.h"
 
+#include <math.h>
+
+#include "base/bind.h"
+#include "base/guid.h"
+#include "base/time/default_tick_clock.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_status_code.h"
@@ -23,16 +28,53 @@ std::string HttpMethodToString(PreviewsProber::HttpMethod http_method) {
   }
 }
 
+// Computes the time delta for a given Backoff algorithm, a base interval, and
+// the count of how many attempts have been made thus far.
+base::TimeDelta ComputeNextTimeDeltaForBackoff(PreviewsProber::Backoff backoff,
+                                               base::TimeDelta base_interval,
+                                               size_t attempts_so_far) {
+  switch (backoff) {
+    case PreviewsProber::Backoff::kLinear:
+      return base_interval;
+    case PreviewsProber::Backoff::kExponential:
+      return base_interval * pow(2, attempts_so_far);
+  }
+}
+
 }  // namespace
 
+PreviewsProber::RetryPolicy::RetryPolicy() = default;
+PreviewsProber::RetryPolicy::~RetryPolicy() = default;
+PreviewsProber::RetryPolicy::RetryPolicy(PreviewsProber::RetryPolicy const&) =
+    default;
+
+PreviewsProber::PreviewsProber(
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+    const std::string& name,
+    const GURL& url,
+    const HttpMethod http_method,
+    const RetryPolicy& retry_policy)
+    : PreviewsProber(url_loader_factory,
+                     name,
+                     url,
+                     http_method,
+                     retry_policy,
+                     base::DefaultTickClock::GetInstance()) {}
+
 PreviewsProber::PreviewsProber(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     const std::string& name,
     const GURL& url,
-    const HttpMethod http_method)
+    const HttpMethod http_method,
+    const RetryPolicy& retry_policy,
+    const base::TickClock* tick_clock)
     : name_(name),
       url_(url),
       http_method_(http_method),
+      retry_policy_(retry_policy),
+      successive_retry_count_(0),
+      retry_timer_(nullptr),
+      tick_clock_(tick_clock),
       is_active_(false),
       last_probe_status_(base::nullopt),
       url_loader_factory_(url_loader_factory) {}
@@ -44,16 +86,27 @@ void PreviewsProber::SendNowIfInactive() {
 
   if (is_active_)
     return;
+
+  successive_retry_count_ = 0;
   CreateAndStartURLLoader();
 }
 
 void PreviewsProber::CreateAndStartURLLoader() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(!is_active_);
+  DCHECK(!is_active_ || successive_retry_count_ > 0);
+  DCHECK(!retry_timer_ || !retry_timer_->IsRunning());
   DCHECK(!url_loader_);
 
   is_active_ = true;
 
+  GURL url = url_;
+  if (retry_policy_.use_random_urls) {
+    std::string query = "guid=" + base::GenerateGUID();
+    GURL::Replacements replacements;
+    replacements.SetQuery(query.c_str(), url::Component(0, query.length()));
+    url = url.ReplaceComponents(replacements);
+  }
+
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("previews_prober", R"(
         semantics {
@@ -77,7 +130,7 @@ void PreviewsProber::CreateAndStartURLLoader() {
           policy_exception_justification: "Not implemented."
         })");
   auto request = std::make_unique<network::ResourceRequest>();
-  request->url = url_;
+  request->url = url;
   request->method = HttpMethodToString(http_method_);
   request->load_flags = net::LOAD_DISABLE_CACHE;
   request->allow_credentials = false;
@@ -85,6 +138,7 @@ void PreviewsProber::CreateAndStartURLLoader() {
   // TODO(crbug/977603): Set retry options.
   url_loader_ =
       network::SimpleURLLoader::Create(std::move(request), traffic_annotation);
+  url_loader_->SetAllowHttpErrorResults(true);
 
   url_loader_->DownloadToString(
       url_loader_factory_.get(),
@@ -103,11 +157,51 @@ void PreviewsProber::OnURLLoadComplete(
   }
 
   // TODO(crbug/977603): Replace with delegate check.
-  last_probe_status_ =
+  bool was_successful =
       url_loader_->NetError() == net::OK && response_code == net::HTTP_OK;
 
   url_loader_.reset();
+
+  if (was_successful) {
+    ProcessProbeSuccess();
+    return;
+  }
+  ProcessProbeFailure();
+}
+
+void PreviewsProber::ProcessProbeFailure() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!retry_timer_ || !retry_timer_->IsRunning());
+  DCHECK(is_active_);
+
+  last_probe_status_ = false;
+
+  if (retry_policy_.max_retries > successive_retry_count_) {
+    base::TimeDelta interval = ComputeNextTimeDeltaForBackoff(
+        retry_policy_.backoff, retry_policy_.base_interval,
+        successive_retry_count_);
+
+    retry_timer_ = std::make_unique<base::OneShotTimer>(tick_clock_);
+    // base::Unretained is safe because |retry_timer_| is owned by this.
+    retry_timer_->Start(FROM_HERE, interval,
+                        base::BindOnce(&PreviewsProber::CreateAndStartURLLoader,
+                                       base::Unretained(this)));
+
+    successive_retry_count_++;
+    return;
+  }
+
+  is_active_ = false;
+}
+
+void PreviewsProber::ProcessProbeSuccess() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!retry_timer_ || !retry_timer_->IsRunning());
+  DCHECK(is_active_);
+
   is_active_ = false;
+  last_probe_status_ = true;
+  successive_retry_count_ = 0;
 }
 
 base::Optional<bool> PreviewsProber::LastProbeWasSuccessful() const {

chrome/browser/previews/previews_prober.h

@@ -12,6 +12,8 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/optional.h"
 #include "base/sequence_checker.h"
+#include "base/time/tick_clock.h"
+#include "base/timer/timer.h"
 #include "url/gurl.h"
 
 namespace network {
@@ -24,6 +26,40 @@ class SharedURLLoaderFactory;
 // available or accessible by Chrome.
 class PreviewsProber {
  public:
+  // This enum describes the different algorithms that can be used to calculate
+  // a time delta between probe events like retries or timeout ttl.
+  enum class Backoff {
+    // Use the same time delta for each event.
+    kLinear,
+
+    // Use an exponentially increasing time delta, base 2.
+    kExponential,
+  };
+
+  struct RetryPolicy {
+    RetryPolicy();
+    RetryPolicy(const RetryPolicy& other);
+    ~RetryPolicy();
+
+    // The maximum number of retries (not including the original probe) to
+    // attempt.
+    size_t max_retries = 3;
+
+    // How to compute the time interval between successive retries.
+    Backoff backoff = Backoff::kLinear;
+
+    // Time between probes as the base value. For example, given |backoff|:
+    //   LINEAR: |base_interval| between the end of last probe and start of next
+    //           probe.
+    //   EXPONENTIAL: (|base_interval| * 2 ^ |successive_retry_count_|) between
+    //                the end of last retry and start of next retry.
+    base::TimeDelta base_interval = base::TimeDelta();
+
+    // If true, this attaches a random GUID query param to the URL of every
+    // probe, including the first probe.
+    bool use_random_urls = false;
+  };
+
   enum class HttpMethod {
     kGet,
     kHead,
@@ -33,7 +69,8 @@ class PreviewsProber {
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       const std::string& name,
       const GURL& url,
-      HttpMethod http_method);
+      HttpMethod http_method,
+      const RetryPolicy& retry_policy);
   ~PreviewsProber();
 
   // Sends a probe now if the prober is currently inactive. If the probe is
@@ -43,9 +80,24 @@ class PreviewsProber {
   // Returns the successfulness of the last probe, if there was one.
   base::Optional<bool> LastProbeWasSuccessful() const;
 
+  // True if probes are being attempted, including retries.
+  bool is_active() const { return is_active_; }
+
+ protected:
+  // Exposes |tick_clock| for testing.
+  PreviewsProber(
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+      const std::string& name,
+      const GURL& url,
+      HttpMethod http_method,
+      const RetryPolicy& retry_policy,
+      const base::TickClock* tick_clock);
+
  private:
   void CreateAndStartURLLoader();
   void OnURLLoadComplete(std::unique_ptr<std::string> response_body);
+  void ProcessProbeFailure();
+  void ProcessProbeSuccess();
 
   // The name given to this prober instance, used in metrics, prefs, and traffic
   // annotations.
@@ -57,6 +109,19 @@ class PreviewsProber {
   // The HTTP method used for probing.
   const HttpMethod http_method_;
 
+  // The retry policy to use in this prober.
+  const RetryPolicy retry_policy_;
+
+  // The number of retries that have been attempted. This count does not include
+  // the original probe.
+  size_t successive_retry_count_;
+
+  // If a retry is being attempted, this will be running until the next attempt.
+  std::unique_ptr<base::OneShotTimer> retry_timer_;
+
+  // The tick clock used for |retry_timer_|.
+  const base::TickClock* tick_clock_;
+
   // Whether the prober is currently sending probes.
   bool is_active_;
 

chrome/browser/previews/previews_prober_unittest.cc

@@ -20,37 +20,97 @@ const GURL kTestUrl("https://test.com");
 const char kName[] = "testing";
 }  // namespace
 
+class TestPreviewsProber : public PreviewsProber {
+ public:
+  TestPreviewsProber(
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+      const std::string& name,
+      const GURL& url,
+      const HttpMethod http_method,
+      const RetryPolicy& retry_policy,
+      const base::TickClock* tick_clock)
+      : PreviewsProber(url_loader_factory,
+                       name,
+                       url,
+                       http_method,
+                       retry_policy,
+                       tick_clock) {}
+};
+
 class PreviewsProberTest : public testing::Test {
  public:
   PreviewsProberTest()
-      : test_shared_loader_factory_(
+      : scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME),
+        test_shared_loader_factory_(
             base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
                 &test_url_loader_factory_)) {}
 
   std::unique_ptr<PreviewsProber> NewProber() {
-    return std::make_unique<PreviewsProber>(test_shared_loader_factory_, kName,
-                                            kTestUrl,
-                                            PreviewsProber::HttpMethod::kGet);
+    return std::make_unique<TestPreviewsProber>(
+        test_shared_loader_factory_, kName, kTestUrl,
+        PreviewsProber::HttpMethod::kGet, PreviewsProber::RetryPolicy(),
+        scoped_task_environment_.GetMockTickClock());
+  }
+
+  std::unique_ptr<PreviewsProber> NewProberWithRetryPolicy(
+      const PreviewsProber::RetryPolicy& retry_policy) {
+    return std::make_unique<TestPreviewsProber>(
+        test_shared_loader_factory_, kName, kTestUrl,
+        PreviewsProber::HttpMethod::kGet, retry_policy,
+        scoped_task_environment_.GetMockTickClock());
   }
 
   void RunUntilIdle() { scoped_task_environment_.RunUntilIdle(); }
 
-  void SetResponse(net::HttpStatusCode http_status, net::Error net_error) {
+  void FastForward(base::TimeDelta delta) {
+    scoped_task_environment_.FastForwardBy(delta);
+  }
+
+  void MakeResponseAndWait(net::HttpStatusCode http_status,
+                           net::Error net_error) {
+    network::TestURLLoaderFactory::PendingRequest* request =
+        test_url_loader_factory_.GetPendingRequest(0);
+
+    ASSERT_EQ(request->request.url.host(), kTestUrl.host());
+    ASSERT_EQ(request->request.url.scheme(), kTestUrl.scheme());
+
     network::ResourceResponseHead head =
         network::CreateResourceResponseHead(http_status);
     network::URLLoaderCompletionStatus status(net_error);
-    test_url_loader_factory_.AddResponse(kTestUrl, head, "content", status);
+    test_url_loader_factory_.AddResponse(request->request.url, head, "content",
+                                         status);
+    RunUntilIdle();
+    // Clear responses in the network service so we can inspect the next request
+    // that comes in before it is responded to.
+    ClearResponses();
   }
 
-  void VerifyRequest() {
+  void ClearResponses() { test_url_loader_factory_.ClearResponses(); }
+
+  void VerifyNoRequests() {
+    EXPECT_EQ(test_url_loader_factory_.NumPending(), 0);
+  }
+
+  void VerifyRequest(bool expect_random_guid = false) {
     ASSERT_EQ(test_url_loader_factory_.NumPending(), 1);
 
     network::TestURLLoaderFactory::PendingRequest* request =
         test_url_loader_factory_.GetPendingRequest(0);
-    EXPECT_EQ(request->request.url, kTestUrl);
     EXPECT_EQ(request->request.method, "GET");
     EXPECT_EQ(request->request.load_flags, net::LOAD_DISABLE_CACHE);
     EXPECT_FALSE(request->request.allow_credentials);
+    if (expect_random_guid) {
+      EXPECT_NE(request->request.url, kTestUrl);
+      EXPECT_TRUE(request->request.url.query().find("guid=") !=
+                  std::string::npos);
+      EXPECT_EQ(request->request.url.query().length(),
+                5U /* len("guid=") */ + 36U /* len(hex guid with hyphens) */);
+      // We don't check for the randomness of successive GUIDs on the assumption
+      // base::GenerateGUID() is always correct.
+    } else {
+      EXPECT_EQ(request->request.url, kTestUrl);
+    }
   }
 
  private:
@@ -66,9 +126,9 @@ TEST_F(PreviewsProberTest, OK) {
   prober->SendNowIfInactive();
   VerifyRequest();
 
-  SetResponse(net::HTTP_OK, net::OK);
-  RunUntilIdle();
+  MakeResponseAndWait(net::HTTP_OK, net::OK);
   EXPECT_TRUE(prober->LastProbeWasSuccessful().value());
+  EXPECT_FALSE(prober->is_active());
 }
 
 TEST_F(PreviewsProberTest, MultipleStart) {
@@ -90,9 +150,9 @@ TEST_F(PreviewsProberTest, NetError) {
   prober->SendNowIfInactive();
   VerifyRequest();
 
-  SetResponse(net::HTTP_OK, net::ERR_FAILED);
-  RunUntilIdle();
+  MakeResponseAndWait(net::HTTP_OK, net::ERR_FAILED);
   EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_FALSE(prober->is_active());
 }
 
 TEST_F(PreviewsProberTest, HttpError) {
@@ -102,7 +162,94 @@ TEST_F(PreviewsProberTest, HttpError) {
   prober->SendNowIfInactive();
   VerifyRequest();
 
-  SetResponse(net::HTTP_NOT_FOUND, net::OK);
-  RunUntilIdle();
+  MakeResponseAndWait(net::HTTP_NOT_FOUND, net::OK);
+  EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_FALSE(prober->is_active());
+}
+
+TEST_F(PreviewsProberTest, RandomGUID) {
+  PreviewsProber::RetryPolicy retry_policy;
+  retry_policy.use_random_urls = true;
+  retry_policy.max_retries = 0;
+
+  std::unique_ptr<PreviewsProber> prober =
+      NewProberWithRetryPolicy(retry_policy);
+  EXPECT_EQ(prober->LastProbeWasSuccessful(), base::nullopt);
+
+  prober->SendNowIfInactive();
+  VerifyRequest(true /* expect_random_guid */);
+
+  MakeResponseAndWait(net::HTTP_OK, net::ERR_FAILED);
+  EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_FALSE(prober->is_active());
+}
+
+TEST_F(PreviewsProberTest, RetryLinear) {
+  PreviewsProber::RetryPolicy retry_policy;
+  retry_policy.max_retries = 2;
+  retry_policy.backoff = PreviewsProber::Backoff::kLinear;
+  retry_policy.base_interval = base::TimeDelta::FromMilliseconds(1000);
+
+  std::unique_ptr<PreviewsProber> prober =
+      NewProberWithRetryPolicy(retry_policy);
+  EXPECT_EQ(prober->LastProbeWasSuccessful(), base::nullopt);
+
+  prober->SendNowIfInactive();
+  VerifyRequest();
+  MakeResponseAndWait(net::HTTP_OK, net::ERR_FAILED);
+  EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_TRUE(prober->is_active());
+
+  // First retry.
+  FastForward(base::TimeDelta::FromMilliseconds(999));
+  VerifyNoRequests();
+  FastForward(base::TimeDelta::FromMilliseconds(1));
+  VerifyRequest();
+  MakeResponseAndWait(net::HTTP_OK, net::ERR_FAILED);
+  EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_TRUE(prober->is_active());
+
+  // Second retry should be another 1000ms later and be the final one.
+  FastForward(base::TimeDelta::FromMilliseconds(999));
+  VerifyNoRequests();
+  FastForward(base::TimeDelta::FromMilliseconds(1));
+  VerifyRequest();
+  MakeResponseAndWait(net::HTTP_OK, net::ERR_FAILED);
+  EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_FALSE(prober->is_active());
+}
+
+TEST_F(PreviewsProberTest, RetryExponential) {
+  PreviewsProber::RetryPolicy retry_policy;
+  retry_policy.max_retries = 2;
+  retry_policy.backoff = PreviewsProber::Backoff::kExponential;
+  retry_policy.base_interval = base::TimeDelta::FromMilliseconds(1000);
+
+  std::unique_ptr<PreviewsProber> prober =
+      NewProberWithRetryPolicy(retry_policy);
+  EXPECT_EQ(prober->LastProbeWasSuccessful(), base::nullopt);
+
+  prober->SendNowIfInactive();
+  VerifyRequest();
+  MakeResponseAndWait(net::HTTP_OK, net::ERR_FAILED);
+  EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_TRUE(prober->is_active());
+
+  // First retry.
+  FastForward(base::TimeDelta::FromMilliseconds(999));
+  VerifyNoRequests();
+  FastForward(base::TimeDelta::FromMilliseconds(1));
+  VerifyRequest();
+  MakeResponseAndWait(net::HTTP_OK, net::ERR_FAILED);
+  EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_TRUE(prober->is_active());
+
+  // Second retry should be another 2000ms later and be the final one.
+  FastForward(base::TimeDelta::FromMilliseconds(1999));
+  VerifyNoRequests();
+  FastForward(base::TimeDelta::FromMilliseconds(1));
+  VerifyRequest();
+  MakeResponseAndWait(net::HTTP_OK, net::ERR_FAILED);
   EXPECT_FALSE(prober->LastProbeWasSuccessful().value());
+  EXPECT_FALSE(prober->is_active());
 }