PpdProvider v3: improve method deferral

This change
* modifies PpdProvider to fail all outstanding deferred callbacks each
  time it fails to obtain a metadata locale,
* adds a slowdown mechanism to its subsequent attempts to obtain a
  metadata locale,
* modifies PpdProvider to fail the oldest deferred method with
  PpdProvider::CallbackResultCode::SERVER_ERROR (rather than
  INTERNAL_ERROR) to reflect that methods are being enqueued faster
  than the hypothetical server can respond, and
* updates the method deferral unit tests to match.

    --gtest_filter='PpdProviderTest.FailsOldestQueued*'

Bug: chromium:888189
Test: chromeos_unittests \
Change-Id: Ib3ffbc81e8174f5222a7c55535ff9fc25bc7823d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2303979
Commit-Queue: Kalvin Lee <kdlee@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791556}

chromeos/printing/ppd_provider_v3.cc

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <algorithm>
 #include <string>
 #include <utility>
 #include <vector>
@@ -12,10 +13,12 @@
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
+#include "base/time/time.h"
 #include "chromeos/printing/ppd_cache.h"
 #include "chromeos/printing/ppd_metadata_manager.h"
 #include "chromeos/printing/ppd_provider_v3.h"
 #include "chromeos/printing/printer_config_cache.h"
+#include "net/base/backoff_entry.h"
 
 namespace chromeos {
 namespace {
@@ -26,6 +29,24 @@ namespace {
 // See also: struct MethodDeferralContext
 constexpr size_t kMethodDeferralLimit = 20;
 
+// Backoff policy for retrying
+// PpdProviderImpl::TryToGetMetadataManagerLocale(). Specifies that we
+// *  perform the first retry with a 1s delay,
+// *  double the retry delay thereafter, and
+// *  cap the retry delay at 32s.
+//
+// We perform backoff to prevent the PpdProviderImpl from running at
+// full sequence speed if it continuously fails to obtain a metadata
+// locale.
+constexpr net::BackoffEntry::Policy kBackoffPolicy{
+    /*num_errors_to_ignore=*/0,
+    /*initial_delay_ms=*/1000,
+    /*multiply_factor=*/2.0,
+    /*jitter_factor=*/0.0,
+    /*maximum_backoff_ms=*/32000LL,
+    /*entry_lifetime_ms=*/-1LL,
+    /*always_use_initial_delay=*/true};
+
 // Helper struct for PpdProviderImpl. Allows PpdProviderImpl to defer
 // its public method calls, which PpdProviderImpl will do when the
 // PpdMetadataManager is not ready to deal with locale-sensitive PPD
@@ -38,7 +59,7 @@ constexpr size_t kMethodDeferralLimit = 20;
 //    prevent infinite re-enqueueing of public methods once the queue
 //    is full.
 struct MethodDeferralContext {
-  MethodDeferralContext() = default;
+  MethodDeferralContext() : backoff_entry(&kBackoffPolicy) {}
   ~MethodDeferralContext() = default;
 
   // This struct is not copyable.
@@ -59,6 +80,13 @@ struct MethodDeferralContext {
     current_method_is_being_failed = false;
   }
 
+  // Fails all |deferred_methods| synchronously.
+  void FailAllEnqueuedMethods() {
+    while (!deferred_methods.empty()) {
+      FailOneEnqueuedMethod();
+    }
+  }
+
   // Dequeues and posts all |deferred_methods| onto our sequence.
   void FlushAndPostAll() {
     while (!deferred_methods.empty()) {
@@ -79,6 +107,10 @@ struct MethodDeferralContext {
   bool current_method_is_being_failed = false;
 
   base::queue<base::OnceCallback<void()>> deferred_methods;
+
+  // Implements retry backoff for
+  // PpdProviderImpl::TryToGetMetadataManagerLocale().
+  net::BackoffEntry backoff_entry;
 };
 
 // This class implements the PpdProvider interface for the v3 metadata
@@ -99,6 +131,7 @@ class PpdProviderImpl : public PpdProvider {
         file_task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
             {base::TaskPriority::USER_VISIBLE, base::MayBlock(),
              base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})) {
+    // Immediately attempts to obtain a metadata locale.
     TryToGetMetadataManagerLocale();
   }
 
@@ -109,7 +142,7 @@ class PpdProviderImpl : public PpdProvider {
     if (deferral_context_) {
       if (deferral_context_->current_method_is_being_failed) {
         auto failure_cb = base::BindOnce(
-            std::move(cb), PpdProvider::CallbackResultCode::INTERNAL_ERROR,
+            std::move(cb), PpdProvider::CallbackResultCode::SERVER_ERROR,
             std::vector<std::string>());
         base::SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                          std::move(failure_cb));
@@ -170,7 +203,7 @@ class PpdProviderImpl : public PpdProvider {
     if (deferral_context_) {
       if (deferral_context_->current_method_is_being_failed) {
         auto failure_cb = base::BindOnce(
-            std::move(cb), PpdProvider::CallbackResultCode::INTERNAL_ERROR, "",
+            std::move(cb), PpdProvider::CallbackResultCode::SERVER_ERROR, "",
             "");
         base::SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                          std::move(failure_cb));
@@ -218,7 +251,20 @@ class PpdProviderImpl : public PpdProvider {
   // Callback fed to PpdMetadataManager::GetLocale().
   void OnMetadataManagerLocaleGotten(bool succeeded) {
     if (!succeeded) {
-      TryToGetMetadataManagerLocale();
+      // Uh-oh, we concretely failed to get a metadata locale. We should
+      // fail all outstanding deferred methods and let callers retry as
+      // they see fit.
+      deferral_context_->FailAllEnqueuedMethods();
+
+      // Inform the BackoffEntry of our failure; let it adjust the
+      // retry delay.
+      deferral_context_->backoff_entry.InformOfRequest(false);
+
+      base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+          FROM_HERE,
+          base::BindOnce(&PpdProviderImpl::TryToGetMetadataManagerLocale,
+                         weak_factory_.GetWeakPtr()),
+          deferral_context_->backoff_entry.GetTimeUntilRelease());
       return;
     }
     deferral_context_->FlushAndPostAll();

chromeos/printing/ppd_provider_v3_unittest.cc

@@ -115,8 +115,8 @@ class PpdProviderTest : public ::testing::Test {
   };
 
   PpdProviderTest()
-      : task_environment_(base::test::TaskEnvironment::MainThreadType::IO) {
-  }
+      : task_environment_(base::test::TaskEnvironment::MainThreadType::IO,
+                          base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
 
   void SetUp() override {
     ASSERT_TRUE(ppd_cache_temp_dir_.CreateUniqueTempDir());
@@ -390,13 +390,36 @@ TEST_F(PpdProviderTest, FailsOldestQueuedResolveManufacturers) {
   auto provider =
       CreateProvider({"en", PpdCacheRunLocation::kInBackgroundThreads,
                       PropagateLocaleToMetadataManager::kDoNotPropagate});
+
+  // The moment we constructed a provider, it will have requested a
+  // metadata locale. But we don't want it to ever obtain one: we
+  // want to fill up the deferral queue and test that behavior, without
+  // ever hitting the success _or_ failure cases in getting the metadata
+  // locale.
+  //
+  // The pending task is the FakePrinterConfigCache calling back to
+  // indicate failure. We have to take this one, but before we do, we
+  // can ask the FakePrinterConfigCache to pretend to be an infinitely
+  // slow server for future calls.
+  ASSERT_EQ(1UL, task_environment_.GetPendingMainThreadTaskCount());
+  provider_backdoor_.manager_config_cache->DiscardFetchRequestFor(
+      "metadata_v3/locales.json");
+  task_environment_.FastForwardUntilNoTasksRemain();
+
   for (int i = kMethodDeferralLimitForTesting; i >= 0; i--) {
     provider->ResolveManufacturers(base::BindOnce(
         &PpdProviderTest::CaptureResolveManufacturers, base::Unretained(this)));
   }
-  task_environment_.RunUntilIdle();
+
+  // The for loop above should have overflowed the deferral queue by
+  // a factor of one: the oldest call to ResolveManufacturers() should
+  // have been forced out and asked to fail, and we expect it to be
+  // sitting on the sequence right now.
+  ASSERT_EQ(1UL, task_environment_.GetPendingMainThreadTaskCount());
+  task_environment_.FastForwardUntilNoTasksRemain();
+
   ASSERT_EQ(1UL, captured_resolve_manufacturers_.size());
-  EXPECT_EQ(PpdProvider::CallbackResultCode::INTERNAL_ERROR,
+  EXPECT_EQ(PpdProvider::CallbackResultCode::SERVER_ERROR,
             captured_resolve_manufacturers_[0].first);
 }
 
@@ -407,15 +430,38 @@ TEST_F(PpdProviderTest, FailsOldestQueuedReverseLookup) {
   auto provider =
       CreateProvider({"en", PpdCacheRunLocation::kInBackgroundThreads,
                       PropagateLocaleToMetadataManager::kDoNotPropagate});
+
+  // The moment we constructed a provider, it will have requested a
+  // metadata locale. But we don't want it to ever obtain one: we
+  // want to fill up the deferral queue and test that behavior, without
+  // ever hitting the success _or_ failure cases in getting the metadata
+  // locale.
+  //
+  // The pending task is the FakePrinterConfigCache calling back to
+  // indicate failure. We have to take this one, but before we do, we
+  // can ask the FakePrinterConfigCache to pretend to be an infinitely
+  // slow server for future calls.
+  ASSERT_EQ(1UL, task_environment_.GetPendingMainThreadTaskCount());
+  provider_backdoor_.manager_config_cache->DiscardFetchRequestFor(
+      "metadata_v3/locales.json");
+  task_environment_.FastForwardUntilNoTasksRemain();
+
   for (int i = kMethodDeferralLimitForTesting; i >= 0; i--) {
     provider->ReverseLookup(
         "some effective-make-and-model string",
         base::BindOnce(&PpdProviderTest::CaptureReverseLookup,
                        base::Unretained(this)));
   }
-  task_environment_.RunUntilIdle();
+
+  // The for loop above should have overflowed the deferral queue by
+  // a factor of one: the oldest call to ReverseLookup() should have
+  // been forced out and asked to fail, and we expect it to be sitting
+  // on the sequence right now.
+  ASSERT_EQ(1UL, task_environment_.GetPendingMainThreadTaskCount());
+  task_environment_.FastForwardUntilNoTasksRemain();
+
   ASSERT_EQ(1UL, captured_reverse_lookup_.size());
-  EXPECT_EQ(PpdProvider::CallbackResultCode::INTERNAL_ERROR,
+  EXPECT_EQ(PpdProvider::CallbackResultCode::SERVER_ERROR,
             captured_reverse_lookup_[0].code);
 }