commit 20f005c62b55471d3ed8f6fd55e23564adeb59fe
author Chris Sosa <sosa@chromium.org> Thu Sep 05 13:53:08 2013 -0700
committer Chrome Cron <account-1103494@chromium-review.googlesource.com> Fri Sep 06 20:13:07 2013 +0000
tree 57390460e7e96e8a9ef0d663047073067b755a9f
parent b3dcdb309615b8ec280584319f37b187924b6694

Don't backoff update attempts for interactive checks.

Previous CL only did not defer where deferring is Policy driven while
backoff is state driven. This CL also moves the matching p2p logic
into ShouldBackoffDownload which makes it easier to see and test.

BUG=chromium:274056
TEST=New unittests.

Change-Id: I3acd29c0cf11d855cfd78f77016857f52a6d346c
Reviewed-on: https://chromium-review.googlesource.com/168290
Tested-by: Chris Sosa <sosa@chromium.org>
Reviewed-by: David Zeuthen <zeuthen@chromium.org>
Commit-Queue: Chris Sosa <sosa@chromium.org>

omaha_request_action.cc

diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 1c11a4b..e833759 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -853,20 +853,13 @@
     return;
   }
 
-  // Only back off if we're not using p2p
-  if (params_->use_p2p_for_downloading() && !params_->p2p_url().empty()) {
-    LOG(INFO) << "Payload backoff logic is disabled because download "
-              << "will happen from local peer (via p2p).";
-  } else {
-    if (payload_state->ShouldBackoffDownload()) {
-      output_object.update_exists = false;
-      LOG(INFO) << "Ignoring Omaha updates in order to backoff our retry "
-                << "attempts";
-      completer.set_code(kErrorCodeOmahaUpdateDeferredForBackoff);
-      return;
-    }
+  if (payload_state->ShouldBackoffDownload()) {
+    output_object.update_exists = false;
+    LOG(INFO) << "Ignoring Omaha updates in order to backoff our retry "
+              << "attempts";
+    completer.set_code(kErrorCodeOmahaUpdateDeferredForBackoff);
+    return;
   }
-
   completer.set_code(kErrorCodeSuccess);
 }
 

payload_state.cc

diff --git a/payload_state.cc b/payload_state.cc
index 90dc3d3..162848a 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -295,7 +295,16 @@
                  "Can proceed with the download";
     return false;
   }
-
+  if (system_state_->request_params()->use_p2p_for_downloading() &&
+      !system_state_->request_params()->p2p_url().empty()) {
+    LOG(INFO) << "Payload backoff logic is disabled because download "
+              << "will happen from local peer (via p2p).";
+    return false;
+  }
+  if (system_state_->request_params()->interactive()) {
+    LOG(INFO) << "Payload backoff disabled for interactive update checks.";
+    return false;
+  }
   if (response_.is_delta_payload) {
     // If delta payloads fail, we want to fallback quickly to full payloads as
     // they are more likely to succeed. Exponential backoffs would greatly

payload_state_unittest.cc

diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index d8032fe..b4dfc6d 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -645,6 +645,60 @@
   EXPECT_EQ(0, payload_state.GetUrlSwitchCount());
 }
 
+TEST(PayloadStateTest, NoBackoffInteractiveChecks) {
+  OmahaResponse response;
+  response.is_delta_payload = false;
+  PayloadState payload_state;
+  MockSystemState mock_system_state;
+  OmahaRequestParams params(&mock_system_state);
+  params.Init("", "", true); // is_interactive = True.
+  mock_system_state.set_request_params(&params);
+
+  EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
+  SetupPayloadStateWith2Urls("Hash6437", true, &payload_state, &response);
+
+  // Simulate two failures (enough to cause payload backoff) and check
+  // again that we're ready to re-download without any backoff as this is
+  // an interactive check.
+  payload_state.UpdateFailed(kErrorCodeDownloadMetadataSignatureMismatch);
+  payload_state.UpdateFailed(kErrorCodeDownloadMetadataSignatureMismatch);
+  EXPECT_EQ("http://test", payload_state.GetCurrentUrl());
+  EXPECT_EQ(1, payload_state.GetPayloadAttemptNumber());
+  EXPECT_EQ(1, payload_state.GetFullPayloadAttemptNumber());
+  EXPECT_FALSE(payload_state.ShouldBackoffDownload());
+}
+
+TEST(PayloadStateTest, NoBackoffForP2PUpdates) {
+  OmahaResponse response;
+  response.is_delta_payload = false;
+  PayloadState payload_state;
+  MockSystemState mock_system_state;
+  OmahaRequestParams params(&mock_system_state);
+  params.Init("", "", false); // is_interactive = False.
+  mock_system_state.set_request_params(&params);
+
+  EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
+  SetupPayloadStateWith2Urls("Hash6437", true, &payload_state, &response);
+
+  // Simulate two failures (enough to cause payload backoff) and check
+  // again that we're ready to re-download without any backoff as this is
+  // an interactive check.
+  payload_state.UpdateFailed(kErrorCodeDownloadMetadataSignatureMismatch);
+  payload_state.UpdateFailed(kErrorCodeDownloadMetadataSignatureMismatch);
+  EXPECT_EQ("http://test", payload_state.GetCurrentUrl());
+  EXPECT_EQ(1, payload_state.GetPayloadAttemptNumber());
+  EXPECT_EQ(1, payload_state.GetFullPayloadAttemptNumber());
+  // Set p2p url.
+  params.set_use_p2p_for_downloading(true);
+  params.set_p2p_url("http://mypeer:52909/path/to/file");
+  // Should not backoff for p2p updates.
+  EXPECT_FALSE(payload_state.ShouldBackoffDownload());
+
+  params.set_p2p_url("");
+  // No actual p2p update if no url is provided.
+  EXPECT_TRUE(payload_state.ShouldBackoffDownload());
+}
+
 TEST(PayloadStateTest, NoBackoffForDeltaPayloads) {
   OmahaResponse response;
   response.is_delta_payload = true;