Classify errors and advance URL index according to the error code.
In CL https://gerrit.chromium.org/gerrit/39638, we always incremented
the URL index irrespective of the error code. That would cause the first
URL to be given up too quickly in favor of the second one even for
transient errors such as when user closes a lid and reopens after some
time.
The right behavior in this case is to just count those failures towards
the URL and only after repeated failures with no progress should we
advance the URL index.
This CL implements this logic and completes the multiple URL-related
work items outlined in the design doc.
BUG=chromium-os:37206
TEST=Tested all uses cases on my ZGB. Added and updated unit tests.
Change-Id: Ida0cfbfeb9bfab732144049d1b27e3b8958bc252
Reviewed-on: https://gerrit.chromium.org/gerrit/39885
Commit-Queue: Jay Srinivasan <jaysri@chromium.org>
Reviewed-by: Jay Srinivasan <jaysri@chromium.org>
Tested-by: Jay Srinivasan <jaysri@chromium.org>
diff --git a/payload_state.h b/payload_state.h
index 6861e70..cc90cf4 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -5,27 +5,29 @@
#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_PAYLOAD_STATE_H__
#define CHROMEOS_PLATFORM_UPDATE_ENGINE_PAYLOAD_STATE_H__
-#include "base/file_path.h"
-
-#include "update_engine/action_processor.h"
+#include "update_engine/payload_state_interface.h"
#include "update_engine/prefs_interface.h"
namespace chromeos_update_engine {
-// Forward declaration here because we get a circular dependency if
-// we include omaha_request_action.h directly.
-struct OmahaResponse;
-
// Encapsulates all the payload state required for download. This includes the
// state necessary for handling multiple URLs in Omaha response, the back-off
// state, etc. All state is persisted so that we use the most recently saved
// value when resuming the update_engine process. All state is also cached in
// memory so that we ensure we always make progress based on last known good
// state even when there's any issue in reading/writing from the file system.
-class PayloadState {
+class PayloadState : public PayloadStateInterface {
public:
- PayloadState() : prefs_(NULL), num_urls_(0), url_index_(0) {}
+ PayloadState()
+ : prefs_(NULL),
+ payload_attempt_number_(0),
+ num_urls_(0),
+ url_index_(0),
+ url_failure_count_(0),
+ max_failure_count_per_url_(0) {}
+
+ virtual ~PayloadState() {}
// Initializes a payload state object using |prefs| for storing the
// persisted state. It also performs the initial loading of all persisted
@@ -34,63 +36,112 @@
// on this object.
bool Initialize(PrefsInterface* prefs);
- // Logs the current payload state.
- void LogPayloadState();
- // Sets the internal payload state based on the given Omaha response. This
- // response could be the same or different from the one for which we've stored
- // the internal state. If it's different, then this method resets all the
- // internal state corresponding to the old response. Since the Omaha response
- // has a lot of fields that are not related to payload state, it uses only
- // a subset of the fields in the Omaha response to compare equality.
- void SetResponse(const OmahaResponse& response);
+ // Implementation of PayloadStateInterface methods.
+ virtual void SetResponse(const OmahaResponse& response);
+ virtual void DownloadComplete();
+ virtual void DownloadProgress(size_t count);
+ virtual void UpdateFailed(ActionExitCode error);
- // Updates the payload state when the current update attempt has failed.
- void UpdateFailed(ActionExitCode error);
-
- // Returns the internally stored subset of the response state as a string.
- // This is logically a private method, but exposed here for unit tests.
- std::string GetResponse() {
+ virtual inline std::string GetResponse() {
return response_;
}
- // Returns the current URL index.
- uint32_t GetUrlIndex() {
+ virtual inline uint32_t GetPayloadAttemptNumber() {
+ return payload_attempt_number_;
+ }
+
+ virtual inline uint32_t GetUrlIndex() {
return url_index_;
}
+ virtual inline uint32_t GetUrlFailureCount() {
+ return url_failure_count_;
+ }
+
private:
- // Sets the stored response_ value from the currently persisted value for
- // the response. Returns the same value.
- std::string LoadResponse();
+ // Logs the current payload state.
+ void LogPayloadState();
- // Sets the url_index_ value from the currently persisted value for
- // URL index. Returns the same value.
- uint32_t LoadUrlIndex();
+ // Increments the payload attempt number which governs the back-off behavior
+ // at the time of the next update check.
+ void IncrementPayloadAttemptNumber();
- // Sets the current URL index.
+ // Advances the current URL index to the next available one. If all URLs have
+ // been exhausted during the current payload download attempt (as indicated
+ // by the payload attempt number), then it will increment the payload attempt
+ // number and wrap around again with the first URL in the list.
+ void IncrementUrlIndex();
+
+ // Increments the failure count of the current URL. If the configured max
+ // failure count is reached for this URL, it advances the current URL index
+ // to the next URL and resets the failure count for that URL.
+ void IncrementFailureCount();
+
+ // Initializes the current response from the persisted state.
+ void LoadResponse();
+
+ // Initializes the payload attempt number from the persisted state.
+ void LoadPayloadAttemptNumber();
+
+ // Sets the payload attempt number to the given value. Also persists the
+ // value being set so that we resume from the same value in case of a process
+ // restart.
+ void SetPayloadAttemptNumber(uint32_t payload_attempt_number);
+
+ // Initializes the current URL index from the persisted state.
+ void LoadUrlIndex();
+
+ // Sets the current URL index to the given value. Also persists the value
+ // being set so that we resume from the same value in case of a process
+ // restart.
void SetUrlIndex(uint32_t url_index);
+ // Initializes the current URL's failure count from the persisted stae.
+ void LoadUrlFailureCount();
+
+ // Sets the current URL's failure count to the given value. Also persists the
+ // value being set so that we resume from the same value in case of a process
+ // restart.
+ void SetUrlFailureCount(uint32_t url_failure_count);
+
// Interface object with which we read/write persisted state. This must
// be set by calling the Initialize method before calling any other method.
PrefsInterface* prefs_;
- // Cached value of the latest subset of the Omaha response with which we're
- // working off currently. This value is persisted so we load it off the next
- // time when update_engine restarts. The rest of the state in this class will
- // be cleared when we set a new response.
+ // This stores a subset of the current response from Omaha. Each update to
+ // this value is persisted so we resume from the same value in case of a
+ // process restart.
std::string response_;
- // The number of urls in the current response. Not persisted.
+ // The number of times we've tried to download the payload in full. This is
+ // incremented each time we download the payload in full successsfully or
+ // when we exhaust all failure limits for all URLs and are about to wrap
+ // around back to the first URL. Each update to this value is persisted so
+ // we resume from the same value in case of a process restart.
+ uint32_t payload_attempt_number_;
+
+ // The number of urls in the current response. This value is not persisted,
+ // as we will always get a response from Omaha before we need this value.
uint32_t num_urls_;
- // Cached value of the index of the current URL, to be used in case we are
- // unable to read from the persisted store for any reason. This type is
- // different from the one in the accessor methods because PrefsInterface
- // supports only int64_t but we want to provide a stronger abstraction of
- // uint32_t.
+ // The index of the current URL. This type is different from the one in the
+ // accessor methods because PrefsInterface supports only int64_t but we want
+ // to provide a stronger abstraction of uint32_t. Each update to this value
+ // is persisted so we resume from the same value in case of a process
+ // restart.
int64_t url_index_;
+ // The count of failures encountered in the current attempt to download using
+ // the current URL (specified by url_index_). Each update to this value is
+ // persisted so we resume from the same value in case of a process restart.
+ int64_t url_failure_count_;
+
+ // The max failure count per url configured in the current response. This
+ // value is not persisted, as we will always get a response from Omaha before
+ // we need this value.
+ uint32_t max_failure_count_per_url_;
+
DISALLOW_COPY_AND_ASSIGN(PayloadState);
};