AU: Update status to DOWNLOADING only after receiving some bytes from server.
This ensures that users don't see an update download notification until
after a download has successfully started.
Also, added some DownloadActionDelegate unit tests.
BUG=5822
TEST=unit tests, gmerged to device, made sure updates happened and
notifications received
Change-Id: I96912dcd98a53e9bd7eecc63dab704f959a06441
Review URL: http://codereview.chromium.org/3131022
diff --git a/download_action.cc b/download_action.cc
index 5ecb99e..5096703 100644
--- a/download_action.cc
+++ b/download_action.cc
@@ -74,6 +74,9 @@
return;
}
}
+ if (delegate_) {
+ delegate_->SetDownloadStatus(true); // Set to active.
+ }
http_fetcher_->BeginTransfer(install_plan_.download_url);
}
@@ -82,6 +85,9 @@
CHECK_EQ(writer_->Close(), 0);
writer_ = NULL;
http_fetcher_->TerminateTransfer();
+ if (delegate_) {
+ delegate_->SetDownloadStatus(false); // Set to inactive.
+ }
}
void DownloadAction::ReceivedBytes(HttpFetcher *fetcher,
@@ -116,6 +122,9 @@
CHECK_EQ(writer_->Close(), 0) << errno;
writer_ = NULL;
}
+ if (delegate_) {
+ delegate_->SetDownloadStatus(false); // Set to inactive.
+ }
ActionExitCode code =
successful ? kActionCodeSuccess : kActionCodeDownloadTransferError;
if (code == kActionCodeSuccess) {
diff --git a/download_action.h b/download_action.h
index e1e8aeb..a46124f 100644
--- a/download_action.h
+++ b/download_action.h
@@ -35,10 +35,15 @@
class DownloadActionDelegate {
public:
- // Called before any bytes are received and periodically after
- // bytes are received.
- // bytes_received is the number of bytes downloaded thus far.
- // total is the number of bytes expected.
+ // Called right before starting the download with |active| set to
+ // true. Called after completing the download with |active| set to
+ // false.
+ virtual void SetDownloadStatus(bool active) = 0;
+
+ // Called periodically after bytes are received. This method will be
+ // invoked only if the download is active. |bytes_received| is the
+ // number of bytes downloaded thus far. |total| is the number of
+ // bytes expected.
virtual void BytesReceived(uint64_t bytes_received, uint64_t total) = 0;
};
@@ -106,11 +111,11 @@
// Used to find the hash of the bytes downloaded
OmahaHashCalculator omaha_hash_calculator_;
-
+
// For reporting status to outsiders
DownloadActionDelegate* delegate_;
uint64_t bytes_received_;
-
+
DISALLOW_COPY_AND_ASSIGN(DownloadAction);
};
diff --git a/download_action_unittest.cc b/download_action_unittest.cc
index 4e11626..d63ae2d 100644
--- a/download_action_unittest.cc
+++ b/download_action_unittest.cc
@@ -5,6 +5,7 @@
#include <string>
#include <vector>
#include <glib.h>
+#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include "update_engine/action_pipe.h"
#include "update_engine/download_action.h"
@@ -17,10 +18,19 @@
using std::string;
using std::vector;
+using testing::_;
+using testing::AtLeast;
+using testing::InSequence;
class DownloadActionTest : public ::testing::Test { };
namespace {
+class DownloadActionDelegateMock : public DownloadActionDelegate {
+ public:
+ MOCK_METHOD1(SetDownloadStatus, void(bool active));
+ MOCK_METHOD2(BytesReceived, void(uint64_t bytes_received, uint64_t total));
+};
+
class DownloadActionTestProcessorDelegate : public ActionProcessorDelegate {
public:
explicit DownloadActionTestProcessorDelegate(ActionExitCode expected_code)
@@ -73,7 +83,9 @@
return FALSE;
}
-void TestWithData(const vector<char>& data, bool hash_test) {
+void TestWithData(const vector<char>& data,
+ bool hash_test,
+ bool use_download_delegate) {
GMainLoop *loop = g_main_loop_new(g_main_context_default(), FALSE);
// TODO(adlr): see if we need a different file for build bots
@@ -96,7 +108,14 @@
data.size()));
download_action.SetTestFileWriter(&writer);
BondActions(&feeder_action, &download_action);
-
+ DownloadActionDelegateMock download_delegate;
+ if (use_download_delegate) {
+ InSequence s;
+ download_action.set_delegate(&download_delegate);
+ EXPECT_CALL(download_delegate, SetDownloadStatus(true)).Times(1);
+ EXPECT_CALL(download_delegate, BytesReceived(_, _)).Times(AtLeast(1));
+ EXPECT_CALL(download_delegate, SetDownloadStatus(false)).Times(1);
+ }
DownloadActionTestProcessorDelegate delegate(
hash_test ? kActionCodeDownloadHashMismatchError : kActionCodeSuccess);
delegate.loop_ = loop;
@@ -117,7 +136,7 @@
vector<char> small;
const char* foo = "foo";
small.insert(small.end(), foo, foo + strlen(foo));
- TestWithData(small, false);
+ TestWithData(small, false, true);
}
TEST(DownloadActionTest, LargeTest) {
@@ -130,14 +149,21 @@
else
c++;
}
- TestWithData(big, false);
+ TestWithData(big, false, true);
}
TEST(DownloadActionTest, BadHashTest) {
vector<char> small;
const char* foo = "foo";
small.insert(small.end(), foo, foo + strlen(foo));
- TestWithData(small, true);
+ TestWithData(small, true, true);
+}
+
+TEST(DownloadActionTest, NoDownloadDelegateTest) {
+ vector<char> small;
+ const char* foo = "foofoo";
+ small.insert(small.end(), foo, foo + strlen(foo));
+ TestWithData(small, false, false);
}
namespace {
@@ -158,9 +184,7 @@
return FALSE;
}
-} // namespace {}
-
-TEST(DownloadActionTest, TerminateEarlyTest) {
+void TestTerminateEarly(bool use_download_delegate) {
GMainLoop *loop = g_main_loop_new(g_main_context_default(), FALSE);
vector<char> data(kMockHttpFetcherChunkSize + kMockHttpFetcherChunkSize / 2);
@@ -176,6 +200,13 @@
feeder_action.set_obj(install_plan);
DownloadAction download_action(new MockHttpFetcher(&data[0], data.size()));
download_action.SetTestFileWriter(&writer);
+ DownloadActionDelegateMock download_delegate;
+ if (use_download_delegate) {
+ InSequence s;
+ download_action.set_delegate(&download_delegate);
+ EXPECT_CALL(download_delegate, SetDownloadStatus(true)).Times(1);
+ EXPECT_CALL(download_delegate, SetDownloadStatus(false)).Times(1);
+ }
TerminateEarlyTestProcessorDelegate delegate;
delegate.loop_ = loop;
ActionProcessor processor;
@@ -196,6 +227,16 @@
EXPECT_EQ(kMockHttpFetcherChunkSize, resulting_file_size);
}
+} // namespace {}
+
+TEST(DownloadActionTest, TerminateEarlyTest) {
+ TestTerminateEarly(true);
+}
+
+TEST(DownloadActionTest, TerminateEarlyNoDownloadDelegateTest) {
+ TestTerminateEarly(false);
+}
+
class DownloadActionTestAction;
template<>
diff --git a/update_attempter.cc b/update_attempter.cc
index a099d7c..2adb245 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -121,6 +121,7 @@
metrics_lib_(metrics_lib),
priority_(utils::kProcessPriorityNormal),
manage_priority_source_(NULL),
+ download_active_(false),
status_(UPDATE_STATUS_IDLE),
download_progress_(0.0),
last_checked_time_(0),
@@ -319,7 +320,11 @@
}
// Find out which action completed.
if (type == OmahaResponseHandlerAction::StaticType()) {
- SetStatusAndNotify(UPDATE_STATUS_DOWNLOADING);
+ // Note that the status will be updated to DOWNLOADING when some
+ // bytes get actually downloaded from the server and the
+ // BytesReceived callback is invoked. This avoids notifying the
+ // user that a download has started in cases when the server and
+ // the client are unable to initiate the download.
OmahaResponseHandlerAction* omaha_response_handler_action =
dynamic_cast<OmahaResponseHandlerAction*>(action);
CHECK(omaha_response_handler_action);
@@ -347,8 +352,13 @@
NOTIMPLEMENTED();
}
+void UpdateAttempter::SetDownloadStatus(bool active) {
+ download_active_ = active;
+ LOG(INFO) << "Download status: " << (active ? "active" : "inactive");
+}
+
void UpdateAttempter::BytesReceived(uint64_t bytes_received, uint64_t total) {
- if (status_ != UPDATE_STATUS_DOWNLOADING) {
+ if (!download_active_) {
LOG(ERROR) << "BytesReceived called while not downloading.";
return;
}
diff --git a/update_attempter.h b/update_attempter.h
index 1993321..0f5d605 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -87,7 +87,8 @@
// UPDATED_NEED_REBOOT. Returns true on sucess, false otherwise.
bool RebootIfNeeded();
- // DownloadActionDelegate method
+ // DownloadActionDelegate methods
+ void SetDownloadStatus(bool active);
void BytesReceived(uint64_t bytes_received, uint64_t total);
private:
@@ -152,6 +153,10 @@
// The process priority management timeout source.
GSource* manage_priority_source_;
+ // Set to true if an update download is active (and BytesReceived
+ // will be called), set to false otherwise.
+ bool download_active_;
+
// For status:
UpdateStatus status_;
double download_progress_;
diff --git a/update_engine_client.cc b/update_engine_client.cc
index e25d313..a55820a 100644
--- a/update_engine_client.cc
+++ b/update_engine_client.cc
@@ -234,7 +234,7 @@
// Wait for an update to complete.
if (FLAGS_update) {
- LOG(INFO) << "Waiting for update the complete.";
+ LOG(INFO) << "Waiting for update to complete.";
CompleteUpdate(); // Should never return.
return 1;
}