PpdProvider v3: rework method deferral

This change modifies method deferral in PpdProvider. The existing
behavior is to
* try getting a metadata locale immediately at time of construction
  and to
* retry getting the metadata locale on failure in perpetuity (albeit
  with a backoff) until it succeeds.

This change modifies the behavior to instead
* wait until a locale-sensitive method of PpdProvider is called to
  get a metadata locale and
* not to retry getting the metadata locale automatically, electing
  to wait for the next time a locale-sensitive method is called.

Bug: chromium:888189
Test: chromeos_unittests --gtest_filter='PpdProviderTest.*'
Change-Id: I40fd135a8adecbc1dc3216ef370c4623fcb7195f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2391772
Commit-Queue: Kalvin Lee <kdlee@chromium.org>
Reviewed-by: Luum Habtemariam <luum@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808233}

chromeos/printing/ppd_provider_v3.cc

@@ -25,7 +25,6 @@
 #include "chromeos/printing/printer_config_cache.h"
 #include "chromeos/printing/printer_configuration.h"
 #include "chromeos/printing/printing_constants.h"
-#include "net/base/backoff_entry.h"
 #include "net/base/filename_util.h"
 
 namespace chromeos {
@@ -37,24 +36,6 @@ 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};
-
 // Age limit for time-sensitive API calls. Typically denotes "Please
 // respond with data no older than kMaxDataAge." Arbitrarily chosen.
 constexpr base::TimeDelta kMaxDataAge = base::TimeDelta::FromMinutes(30LL);
@@ -107,7 +88,7 @@ std::string PpdPathInServingRoot(base::StringPiece ppd_basename) {
 //    prevent infinite re-enqueueing of public methods once the queue
 //    is full.
 struct MethodDeferralContext {
-  MethodDeferralContext() : backoff_entry(&kBackoffPolicy) {}
+  MethodDeferralContext() = default;
   ~MethodDeferralContext() = default;
 
   // This struct is not copyable.
@@ -146,6 +127,9 @@ struct MethodDeferralContext {
 
   bool IsFull() { return deferred_methods.size() >= kMethodDeferralLimit; }
 
+  // Whether an attempt to get the metadata locale is ongoing.
+  bool metadata_locale_fetch_is_ongoing = false;
+
   // This bool is checked during execution of a queue-able public method
   // of PpdProviderImpl. If it is true, then
   // 1. the current queue-able public method was previously enqueued,
@@ -155,10 +139,6 @@ 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
@@ -179,8 +159,6 @@ 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();
   }
 
   void ResolveManufacturers(ResolveManufacturersCallback cb) override {
@@ -201,6 +179,8 @@ class PpdProviderImpl : public PpdProvider {
         deferral_context_->FailOneEnqueuedMethod();
         DCHECK(!deferral_context_->IsFull());
       }
+      TryToGetMetadataManagerLocale();
+
       base::OnceCallback<void()> this_method =
           base::BindOnce(&PpdProviderImpl::ResolveManufacturers,
                          weak_factory_.GetWeakPtr(), std::move(cb));
@@ -329,6 +309,8 @@ class PpdProviderImpl : public PpdProvider {
         deferral_context_->FailOneEnqueuedMethod();
         DCHECK(!deferral_context_->IsFull());
       }
+      TryToGetMetadataManagerLocale();
+
       base::OnceCallback<void()> this_method = base::BindOnce(
           &PpdProviderImpl::ReverseLookup, weak_factory_.GetWeakPtr(),
           effective_make_and_model, std::move(cb));
@@ -416,17 +398,21 @@ class PpdProviderImpl : public PpdProvider {
     return file_contents;
   }
 
-  // Readies |metadata_manager_| to call methods which require a
-  // successful callback from PpdMetadataManager::GetLocale().
+  // Requests that |metadata_manager_| obtain a metadata locale so that
+  // |this| can call its locale-sensitive methods.
   //
   // |this| is largely useless if its |metadata_manager_| is not ready
   // to traffick in locale-sensitive PPD metadata, so we want this
   // method to eventually succeed.
   void TryToGetMetadataManagerLocale() {
+    if (deferral_context_->metadata_locale_fetch_is_ongoing) {
+      return;
+    }
     auto callback =
         base::BindOnce(&PpdProviderImpl::OnMetadataManagerLocaleGotten,
                        weak_factory_.GetWeakPtr());
     metadata_manager_->GetLocale(std::move(callback));
+    deferral_context_->metadata_locale_fetch_is_ongoing = true;
   }
 
   // Evaluates true if our |version_| falls within the bounds set by
@@ -444,21 +430,12 @@ class PpdProviderImpl : public PpdProvider {
 
   // Callback fed to PpdMetadataManager::GetLocale().
   void OnMetadataManagerLocaleGotten(bool succeeded) {
+    deferral_context_->metadata_locale_fetch_is_ongoing = false;
     if (!succeeded) {
       // 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

@@ -249,6 +249,24 @@ class PpdProviderTest : public ::testing::Test {
   void DiscardResolvePpd(PpdProvider::CallbackResultCode code,
                          const std::string& contents) {}
 
+  // Calls the ResolveManufacturer() method of the |provider| and
+  // waits for its completion. Ignores the returned string values and
+  // returns whether the result code was
+  // PpdProvider::CallbackResultCode::SUCCESS.
+  bool SuccessfullyResolveManufacturers(PpdProvider* provider) {
+    base::RunLoop run_loop;
+    PpdProvider::CallbackResultCode code;
+    provider->ResolveManufacturers(base::BindLambdaForTesting(
+        [&run_loop, &code](
+            PpdProvider::CallbackResultCode result_code,
+            const std::vector<std::string>& unused_manufacturers) {
+          code = result_code;
+          run_loop.QuitClosure().Run();
+        }));
+    run_loop.Run();
+    return code == PpdProvider::CallbackResultCode::SUCCESS;
+  }
+
  protected:
   // List of relevant endpoint for this FakeServer
   std::vector<std::pair<std::string, std::string>> server_contents() const {
@@ -455,20 +473,11 @@ TEST_F(PpdProviderTest, FailsOldestQueuedResolveManufacturers) {
       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());
+  // Prevents the provider from ever getting a metadata locale.
+  // We want it to stall out, forcing it to perpetually defer method
+  // calls to ResolveManufacturers().
   provider_backdoor_.manager_config_cache->DiscardFetchRequestFor(
       "metadata_v3/locales.json");
-  task_environment_.FastForwardUntilNoTasksRemain();
 
   for (int i = kMethodDeferralLimitForTesting; i >= 0; i--) {
     provider->ResolveManufacturers(base::BindOnce(
@@ -495,20 +504,11 @@ TEST_F(PpdProviderTest, FailsOldestQueuedReverseLookup) {
       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());
+  // Prevents the provider from ever getting a metadata locale.
+  // We want it to stall out, forcing it to perpetually defer method
+  // calls to ReverseLookup().
   provider_backdoor_.manager_config_cache->DiscardFetchRequestFor(
       "metadata_v3/locales.json");
-  task_environment_.FastForwardUntilNoTasksRemain();
 
   for (int i = kMethodDeferralLimitForTesting; i >= 0; i--) {
     provider->ReverseLookup(
@@ -536,15 +536,6 @@ TEST_F(PpdProviderTest, ManufacturersFetch) {
                       PropagateLocaleToMetadataManager::kDoNotPropagate});
   StartFakePpdServer();
 
-  // The provider attempted to get a metadata locale as soon as it was
-  // constructed. The pending task on the sequence is a callback from
-  // the FakePrinterConfigCache indicating that it failed to get a
-  // metadata locale. We'll let that fail harmelssly; having called
-  // StartFakePpdServer(), the retry attempt will allow the provider to
-  // successfully obtain a metadata locale.
-  ASSERT_EQ(1UL, task_environment_.GetPendingMainThreadTaskCount());
-  task_environment_.FastForwardUntilNoTasksRemain();
-
   // Issue two requests at the same time, both should be resolved properly.
   provider->ResolveManufacturers(base::BindOnce(
       &PpdProviderTest::CaptureResolveManufacturers, base::Unretained(this)));
@@ -568,26 +559,13 @@ TEST_F(PpdProviderTest, ManufacturersFetchNoServer) {
       CreateProvider({"en", PpdCacheRunLocation::kInBackgroundThreads,
                       PropagateLocaleToMetadataManager::kDoNotPropagate});
 
-  // Unlike the above, there is no breaking out of the failure loop in
-  // which the provider attempts to fetch a metadata locale and the
-  // fake server indicates that none are forthcoming.
-  //
-  // Since the provider never stops trying to get the metadata locale,
-  // it's not appropriate to use things like RunUntilIdle() or
-  // FastForwardUntilNoTasksRemain().
-  base::RunLoop run_loop;
-
   // Issue two requests at the same time, both should resolve properly
   // (though they will fail).
   provider->ResolveManufacturers(base::BindOnce(
       &PpdProviderTest::CaptureResolveManufacturers, base::Unretained(this)));
-  provider->ResolveManufacturers(base::BindLambdaForTesting(
-      [this, &run_loop](PpdProvider::CallbackResultCode code,
-                        const std::vector<std::string>& empty_manufacturers) {
-        this->CaptureResolveManufacturers(code, empty_manufacturers);
-        run_loop.QuitClosure().Run();
-      }));
-  run_loop.Run();
+  provider->ResolveManufacturers(base::BindOnce(
+      &PpdProviderTest::CaptureResolveManufacturers, base::Unretained(this)));
+  task_environment_.FastForwardUntilNoTasksRemain();
 
   ASSERT_EQ(2UL, captured_resolve_manufacturers_.size());
   EXPECT_EQ(PpdProvider::SERVER_ERROR,
@@ -718,11 +696,10 @@ TEST_F(PpdProviderTest, ResolvePrinters) {
                       PropagateLocaleToMetadataManager::kDoPropagate});
   StartFakePpdServer();
 
-  // Same as the contents in test fixture's server_contents() above,
-  // but allows us to bypass PpdProvider::ResolveManufacturers().
+  // Required setup calls to advance past PpdProvider's method deferral.
   ASSERT_TRUE(provider_backdoor_.metadata_manager->SetManufacturersForTesting(
       kDefaultManufacturersJson));
-  task_environment_.RunUntilIdle();
+  ASSERT_TRUE(SuccessfullyResolveManufacturers(provider.get()));
 
   provider->ResolvePrinters(
       "Manufacturer A", base::BindOnce(&PpdProviderTest::CaptureResolvePrinters,
@@ -765,11 +742,10 @@ TEST_F(PpdProviderTest, ResolvePrintersBadReference) {
                       PropagateLocaleToMetadataManager::kDoPropagate});
   StartFakePpdServer();
 
-  // Same as the contents in test fixture's server_contents() above,
-  // but allows us to bypass PpdProvider::ResolveManufacturers().
+  // Required setup calls to advance past PpdProvider's method deferral.
   ASSERT_TRUE(provider_backdoor_.metadata_manager->SetManufacturersForTesting(
       kDefaultManufacturersJson));
-  task_environment_.RunUntilIdle();
+  ASSERT_TRUE(SuccessfullyResolveManufacturers(provider.get()));
 
   provider->ResolvePrinters(
       "bogus_doesnt_exist",
@@ -786,11 +762,10 @@ TEST_F(PpdProviderTest, ResolvePrintersNoServer) {
       CreateProvider({"en", PpdCacheRunLocation::kInBackgroundThreads,
                       PropagateLocaleToMetadataManager::kDoPropagate});
 
-  // Same as the contents in test fixture's server_contents() above,
-  // but allows us to bypass PpdProvider::ResolveManufacturers().
+  // Required setup calls to advance past PpdProvider's method deferral.
   ASSERT_TRUE(provider_backdoor_.metadata_manager->SetManufacturersForTesting(
       kDefaultManufacturersJson));
-  task_environment_.RunUntilIdle();
+  ASSERT_TRUE(SuccessfullyResolveManufacturers(provider.get()));
 
   provider->ResolvePrinters(
       "Manufacturer A", base::BindOnce(&PpdProviderTest::CaptureResolvePrinters,