update_engine: NextPayload() resets URL index
This change fixes a bug/assumption that |PayloadState| used to make in
regards to URL index related to payloads. When a URL index is
incremented, there is no gurauntee that subsequent payloads will have
the same number of candidate URLs, hence it is critical to reset the URL
index back to 0 for subsequent payloads. This fix also allows candidate
URLs to not be skipped over for multi-package/payload request/responses.
The max number of times a URL is allowed to fail is reduced from 10 to 3
to allow preferred URLs to always be used as the intial URL for payloads.
BUG=chromium:928805
TEST=FEATURES=test emerge-$B update_engine # filter PayloadStateTest
Change-Id: I67732b2b7da08f580d1b554fd85eb06b3bf1f761
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2188552
Tested-by: Jae Hoon Kim <kimjae@chromium.org>
Commit-Queue: Andrew Lassalle <andrewlassalle@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
diff --git a/omaha_request_action.h b/omaha_request_action.h
index 623a704..30b3d22 100644
--- a/omaha_request_action.h
+++ b/omaha_request_action.h
@@ -68,12 +68,12 @@
public HttpFetcherDelegate {
public:
static const int kPingTimeJump = -2;
- // We choose this value of 10 as a heuristic for a work day in trying
+ // We choose this value of 3 as a heuristic for a work day in trying
// each URL, assuming we check roughly every 45 mins. This is a good time to
- // wait - neither too long nor too little - so we don't give up the preferred
- // URLs that appear earlier in list too quickly before moving on to the
- // fallback ones.
- static const int kDefaultMaxFailureCountPerUrl = 10;
+ // wait so we don't give up the preferred URLs, but allow using the URL that
+ // appears earlier in list for every payload before resorting to the fallback
+ // URLs in the candiate URL list.
+ static const int kDefaultMaxFailureCountPerUrl = 3;
// If staging is enabled, set the maximum wait time to 28 days, since that is
// the predetermined wait time for staging.
diff --git a/payload_state.cc b/payload_state.cc
index 355552e..5facdff 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -469,9 +469,7 @@
void PayloadState::IncrementUrlIndex() {
size_t next_url_index = url_index_ + 1;
- size_t max_url_size = 0;
- for (const auto& urls : candidate_urls_)
- max_url_size = std::max(max_url_size, urls.size());
+ size_t max_url_size = candidate_urls_[payload_index_].size();
if (next_url_index < max_url_size) {
LOG(INFO) << "Incrementing the URL index for next attempt";
SetUrlIndex(next_url_index);
@@ -902,6 +900,7 @@
bool PayloadState::NextPayload() {
if (payload_index_ + 1 >= candidate_urls_.size())
return false;
+ SetUrlIndex(0);
SetPayloadIndex(payload_index_ + 1);
return true;
}
diff --git a/payload_state.h b/payload_state.h
index 5ef1220..bfe2cf0 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -156,6 +156,7 @@
FRIEND_TEST(PayloadStateTest, RollbackHappened);
FRIEND_TEST(PayloadStateTest, RollbackVersion);
FRIEND_TEST(PayloadStateTest, UpdateSuccessWithWipedPrefs);
+ FRIEND_TEST(PayloadStateTest, NextPayloadResetsUrlIndex);
// Helper called when an attempt has begun, is called by
// UpdateResumed(), UpdateRestarted() and Rollback().
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index 869c24e..4a0afcf 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -1655,4 +1655,31 @@
EXPECT_EQ(null_time, payload_state.GetP2PFirstAttemptTimestamp());
}
+TEST(PayloadStateTest, NextPayloadResetsUrlIndex) {
+ PayloadState payload_state;
+ FakeSystemState fake_system_state;
+ EXPECT_TRUE(payload_state.Initialize(&fake_system_state));
+
+ OmahaResponse response;
+ response.packages.push_back(
+ {.payload_urls = {"http://test1a", "http://test2a"},
+ .size = 123456789,
+ .metadata_size = 58123,
+ .metadata_signature = "msign",
+ .hash = "hash"});
+ response.packages.push_back({.payload_urls = {"http://test1b"},
+ .size = 123456789,
+ .metadata_size = 58123,
+ .metadata_signature = "msign",
+ .hash = "hash"});
+ payload_state.SetResponse(response);
+
+ EXPECT_EQ(payload_state.GetCurrentUrl(), "http://test1a");
+ payload_state.IncrementUrlIndex();
+ EXPECT_EQ(payload_state.GetCurrentUrl(), "http://test2a");
+
+ EXPECT_TRUE(payload_state.NextPayload());
+ EXPECT_EQ(payload_state.GetCurrentUrl(), "http://test1b");
+}
+
} // namespace chromeos_update_engine