Track bytes received across multiple update files
When downloading the packages that comprise a multi-package or multi-app
update, the UpdateAttempter receives BytesReceived() callbacks with
bytes_received resetting to 0 for each file. This causes the progress
calculations to be incorrect.
This change tracks the total of the previously downloaded packages
within the DownloadAction, so that it properly tracks. Resumed
downloads will jump ahead over skipped data, when the payload is
incremented.
Bug: 65451460
Tests: Added unit tests to directly test the accumulation and the the
transition from the previous state to UpdateStatus::DOWNLOADING when the
first bytes are received.
Change-Id: I3b540df16b9a664b09f53ee3ec962e2cbc8adf1b
(cherry picked from commit d6f869dbd9952be8a926e80c4f1e172845ab8d5f)
diff --git a/payload_consumer/download_action.cc b/payload_consumer/download_action.cc
index c3a5016..cd25962 100644
--- a/payload_consumer/download_action.cc
+++ b/payload_consumer/download_action.cc
@@ -173,6 +173,7 @@
install_plan_.Dump();
bytes_received_ = 0;
+ bytes_received_previous_payloads_ = 0;
bytes_total_ = 0;
for (const auto& payload : install_plan_.payloads)
bytes_total_ += payload.size;
@@ -317,8 +318,10 @@
}
bytes_received_ += length;
+ uint64_t bytes_downloaded_total =
+ bytes_received_previous_payloads_ + bytes_received_;
if (delegate_ && download_active_) {
- delegate_->BytesReceived(length, bytes_received_, bytes_total_);
+ delegate_->BytesReceived(length, bytes_downloaded_total, bytes_total_);
}
if (writer_ && !writer_->Write(bytes, length, &code_)) {
if (code_ != ErrorCode::kSuccess) {
@@ -349,13 +352,16 @@
void DownloadAction::TransferComplete(HttpFetcher* fetcher, bool successful) {
if (writer_) {
LOG_IF(WARNING, writer_->Close() != 0) << "Error closing the writer.";
- writer_ = nullptr;
+ if (delta_performer_.get() == writer_) {
+ // no delta_performer_ in tests, so leave the test writer in place
+ writer_ = nullptr;
+ }
}
download_active_ = false;
ErrorCode code =
successful ? ErrorCode::kSuccess : ErrorCode::kDownloadTransferError;
- if (code == ErrorCode::kSuccess && delta_performer_.get()) {
- if (!payload_->already_applied)
+ if (code == ErrorCode::kSuccess) {
+ if (delta_performer_ && !payload_->already_applied)
code = delta_performer_->VerifyPayload(payload_->hash, payload_->size);
if (code != ErrorCode::kSuccess) {
LOG(ERROR) << "Download of " << install_plan_.download_url
@@ -365,10 +371,12 @@
CloseP2PSharingFd(true);
} else if (payload_ < &install_plan_.payloads.back() &&
system_state_->payload_state()->NextPayload()) {
+ LOG(INFO) << "Incrementing to next payload";
// No need to reset if this payload was already applied.
- if (!payload_->already_applied)
+ if (delta_performer_ && !payload_->already_applied)
DeltaPerformer::ResetUpdateProgress(prefs_, false);
// Start downloading next payload.
+ bytes_received_previous_payloads_ += payload_->size;
payload_++;
install_plan_.download_url =
system_state_->payload_state()->GetCurrentUrl();
diff --git a/payload_consumer/download_action.h b/payload_consumer/download_action.h
index d0e6000..786db20 100644
--- a/payload_consumer/download_action.h
+++ b/payload_consumer/download_action.h
@@ -166,7 +166,8 @@
// For reporting status to outsiders
DownloadActionDelegate* delegate_;
- uint64_t bytes_received_{0};
+ uint64_t bytes_received_{0}; // per file/range
+ uint64_t bytes_received_previous_payloads_{0};
uint64_t bytes_total_{0};
bool download_active_{false};
diff --git a/payload_consumer/download_action_unittest.cc b/payload_consumer/download_action_unittest.cc
index 7d3ac6c..f42b1d8 100644
--- a/payload_consumer/download_action_unittest.cc
+++ b/payload_consumer/download_action_unittest.cc
@@ -41,6 +41,7 @@
#include "update_engine/common/utils.h"
#include "update_engine/fake_p2p_manager_configuration.h"
#include "update_engine/fake_system_state.h"
+#include "update_engine/mock_file_writer.h"
#include "update_engine/payload_consumer/mock_download_action.h"
#include "update_engine/update_manager/fake_update_manager.h"
@@ -56,6 +57,7 @@
using testing::AtLeast;
using testing::InSequence;
using testing::Return;
+using testing::SetArgPointee;
using testing::_;
class DownloadActionTest : public ::testing::Test { };
@@ -242,6 +244,92 @@
false); // use_download_delegate
}
+TEST(DownloadActionTest, MultiPayloadProgressTest) {
+ std::vector<brillo::Blob> payload_datas;
+ // the first payload must be the largest, as it's the actual payload used by
+ // the MockHttpFetcher for all downloaded data.
+ payload_datas.emplace_back(4 * kMockHttpFetcherChunkSize + 256);
+ payload_datas.emplace_back(2 * kMockHttpFetcherChunkSize);
+ brillo::FakeMessageLoop loop(nullptr);
+ loop.SetAsCurrent();
+ FakeSystemState fake_system_state;
+ EXPECT_CALL(*fake_system_state.mock_payload_state(), NextPayload())
+ .WillOnce(Return(true));
+
+ MockFileWriter mock_file_writer;
+ EXPECT_CALL(mock_file_writer, Close()).WillRepeatedly(Return(0));
+ EXPECT_CALL(mock_file_writer, Write(_, _, _))
+ .WillRepeatedly(
+ DoAll(SetArgPointee<2>(ErrorCode::kSuccess), Return(true)));
+
+ InstallPlan install_plan;
+ uint64_t total_expected_download_size{0};
+ for (const auto& data : payload_datas) {
+ uint64_t size = data.size();
+ install_plan.payloads.push_back(
+ {.size = size, .type = InstallPayloadType::kFull});
+ total_expected_download_size += size;
+ }
+ ObjectFeederAction<InstallPlan> feeder_action;
+ feeder_action.set_obj(install_plan);
+ MockPrefs prefs;
+ MockHttpFetcher* http_fetcher = new MockHttpFetcher(
+ payload_datas[0].data(), payload_datas[0].size(), nullptr);
+ // takes ownership of passed in HttpFetcher
+ DownloadAction download_action(&prefs,
+ fake_system_state.boot_control(),
+ fake_system_state.hardware(),
+ &fake_system_state,
+ http_fetcher);
+ download_action.SetTestFileWriter(&mock_file_writer);
+ BondActions(&feeder_action, &download_action);
+ MockDownloadActionDelegate download_delegate;
+ {
+ InSequence s;
+ download_action.set_delegate(&download_delegate);
+ // these are hand-computed based on the payloads specified above
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ kMockHttpFetcherChunkSize,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ kMockHttpFetcherChunkSize * 2,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ kMockHttpFetcherChunkSize * 3,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ kMockHttpFetcherChunkSize * 4,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(256,
+ kMockHttpFetcherChunkSize * 4 + 256,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ kMockHttpFetcherChunkSize * 5 + 256,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ total_expected_download_size,
+ total_expected_download_size));
+ }
+ ActionProcessor processor;
+ processor.EnqueueAction(&feeder_action);
+ processor.EnqueueAction(&download_action);
+
+ loop.PostTask(
+ FROM_HERE,
+ base::Bind(
+ [](ActionProcessor* processor) { processor->StartProcessing(); },
+ base::Unretained(&processor)));
+ loop.Run();
+ EXPECT_FALSE(loop.PendingTasks());
+}
+
namespace {
class TerminateEarlyTestProcessorDelegate : public ActionProcessorDelegate {
public:
