Post CloudCommandProxy::SendCommandUpdate to accumulate more changes
Single message loop usually updates several properties of command.
Accumulating them we can reduce number of requests to server.
Change-Id: I57f6ea7e15192c6c1a22dafd66b3add62aee846e
Reviewed-on: https://weave-review.googlesource.com/1263
Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/libweave/src/commands/cloud_command_proxy.cc b/libweave/src/commands/cloud_command_proxy.cc
index 931e3e4..9d7a9d1 100644
--- a/libweave/src/commands/cloud_command_proxy.cc
+++ b/libweave/src/commands/cloud_command_proxy.cc
@@ -88,7 +88,12 @@
}
}
// Send out an update request to the server, if needed.
- SendCommandUpdate();
+
+ // Post to accumulate more changes during the current message loop task run.
+ task_runner_->PostDelayedTask(
+ FROM_HERE, base::Bind(&CloudCommandProxy::SendCommandUpdate,
+ backoff_weak_ptr_factory_.GetWeakPtr()),
+ {});
}
void CloudCommandProxy::SendCommandUpdate() {
diff --git a/libweave/src/commands/cloud_command_proxy_unittest.cc b/libweave/src/commands/cloud_command_proxy_unittest.cc
index dbff905..f91e26c 100644
--- a/libweave/src/commands/cloud_command_proxy_unittest.cc
+++ b/libweave/src/commands/cloud_command_proxy_unittest.cc
@@ -16,11 +16,12 @@
#include "src/commands/unittest_utils.h"
#include "src/states/mock_state_change_queue_interface.h"
-using testing::SaveArg;
+using testing::_;
+using testing::DoAll;
using testing::Invoke;
using testing::Return;
using testing::ReturnPointee;
-using testing::_;
+using testing::SaveArg;
namespace weave {
@@ -148,6 +149,7 @@
const char expected[] = "{'state':'done'}";
EXPECT_CALL(cloud_updater_, UpdateCommand(kCmdID, MatchJson(expected), _, _));
command_instance_->SetResults({}, nullptr);
+ task_runner_.RunOnce();
}
TEST_F(CloudCommandProxyTest, DelayedUpdate) {
@@ -169,17 +171,17 @@
// progress={...}
// The first state update is sent immediately, the second should be delayed.
base::Closure on_success;
- EXPECT_CALL(cloud_updater_,
- UpdateCommand(kCmdID, MatchJson("{'state':'inProgress'}"), _, _))
+ EXPECT_CALL(
+ cloud_updater_,
+ UpdateCommand(
+ kCmdID,
+ MatchJson("{'state':'inProgress', 'progress':{'status':'ready'}}"), _,
+ _))
.WillOnce(SaveArg<2>(&on_success));
EXPECT_TRUE(command_instance_->SetProgress(
*CreateDictionaryValue("{'status': 'ready'}"), nullptr));
- // Now simulate the first request completing.
- // The second request should be sent now.
- const char expected[] = "{'progress':{'status':'ready'}}";
- EXPECT_CALL(cloud_updater_, UpdateCommand(kCmdID, MatchJson(expected), _, _));
- on_success.Run();
+ task_runner_.RunOnce();
}
TEST_F(CloudCommandProxyTest, CombineMultiple) {
@@ -203,40 +205,32 @@
}
TEST_F(CloudCommandProxyTest, RetryFailed) {
+ base::Closure on_success;
base::Closure on_error;
- const char expect1[] = "{'state':'inProgress'}";
- EXPECT_CALL(cloud_updater_, UpdateCommand(kCmdID, MatchJson(expect1), _, _))
- .WillOnce(SaveArg<3>(&on_error));
+
+ const char expect[] =
+ "{'state':'inProgress', 'progress': {'status': 'ready'}}";
+ EXPECT_CALL(cloud_updater_, UpdateCommand(kCmdID, MatchJson(expect), _, _))
+ .Times(3)
+ .WillRepeatedly(DoAll(SaveArg<2>(&on_success), SaveArg<3>(&on_error)));
+ auto started = task_runner_.GetClock()->Now();
EXPECT_TRUE(command_instance_->SetProgress(
*CreateDictionaryValue("{'status': 'ready'}"), nullptr));
-
- // Now pretend the first command update request has failed.
- // We should retry with both state and progress fields updated this time,
- // after the initial backoff (which should be 1s in our case).
- base::TimeDelta expected_delay = base::TimeDelta::FromSeconds(1);
+ task_runner_.Run();
on_error.Run();
+ task_runner_.Run();
+ EXPECT_GE(task_runner_.GetClock()->Now() - started,
+ base::TimeDelta::FromSecondsD(0.9));
- // Execute the delayed request. But pretend that it failed too.
- const char expect2[] = R"({
- 'progress': {'status':'ready'},
- 'state':'inProgress'
- })";
- EXPECT_CALL(cloud_updater_, UpdateCommand(kCmdID, MatchJson(expect2), _, _))
- .WillOnce(SaveArg<3>(&on_error));
- task_runner_.RunOnce();
-
- // Now backoff should be 2 seconds.
- expected_delay = base::TimeDelta::FromSeconds(2);
on_error.Run();
+ task_runner_.Run();
+ EXPECT_GE(task_runner_.GetClock()->Now() - started,
+ base::TimeDelta::FromSecondsD(2.9));
- // Retry the task.
- base::Closure on_success;
- EXPECT_CALL(cloud_updater_, UpdateCommand(kCmdID, MatchJson(expect2), _, _))
- .WillOnce(SaveArg<2>(&on_success));
- task_runner_.RunOnce();
-
- // Pretend it succeeds this time.
on_success.Run();
+ task_runner_.Run();
+ EXPECT_GE(task_runner_.GetClock()->Now() - started,
+ base::TimeDelta::FromSecondsD(2.9));
}
TEST_F(CloudCommandProxyTest, GateOnStateUpdates) {
@@ -361,6 +355,7 @@
const char expected[] = "{'state':'done'}";
EXPECT_CALL(cloud_updater_, UpdateCommand(kCmdID, MatchJson(expected), _, _));
command_instance_->SetResults({}, nullptr);
+ task_runner_.RunOnce();
}
TEST_F(CloudCommandProxyTest, NonEmptyStateChangeQueue) {
diff --git a/libweave/src/device_registration_info.cc b/libweave/src/device_registration_info.cc
index c83b7b6..0e3d64f 100644
--- a/libweave/src/device_registration_info.cc
+++ b/libweave/src/device_registration_info.cc
@@ -438,8 +438,8 @@
LOG(INFO) << "Starting notification channel";
// If no TaskRunner assume we're in test.
- if (!task_runner_) {
- LOG(INFO) << "No TaskRunner, not starting notification channel";
+ if (!network_) {
+ LOG(INFO) << "No Network, not starting notification channel";
return;
}
diff --git a/libweave/src/device_registration_info_unittest.cc b/libweave/src/device_registration_info_unittest.cc
index 71c9832..84ee33a 100644
--- a/libweave/src/device_registration_info_unittest.cc
+++ b/libweave/src/device_registration_info_unittest.cc
@@ -8,6 +8,7 @@
#include <base/json/json_writer.h>
#include <base/values.h>
#include <gtest/gtest.h>
+#include <weave/provider/test/fake_task_runner.h>
#include <weave/provider/test/mock_config_store.h>
#include <weave/provider/test/mock_http_client.h>
@@ -123,7 +124,7 @@
std::unique_ptr<Config> config{new Config{&config_store_}};
config_ = config.get();
dev_reg_.reset(new DeviceRegistrationInfo{command_manager_, state_manager_,
- std::move(config), nullptr,
+ std::move(config), &task_runner_,
&http_client_, nullptr});
ReloadDefaults();
@@ -182,6 +183,7 @@
GcdState GetGcdState() const { return dev_reg_->GetGcdState(); }
+ provider::test::FakeTaskRunner task_runner_;
provider::test::MockConfigStore config_store_;
StrictMock<MockHttpClient> http_client_;
base::DictionaryValue data_;
@@ -539,6 +541,11 @@
ASSERT_NE(nullptr, command_);
}
+ void TearDown() override {
+ task_runner_.RunOnce();
+ DeviceRegistrationInfoTest::TearDown();
+ }
+
Command* command_{nullptr};
std::string command_url_;
};
@@ -549,20 +556,13 @@
http::kPatch, command_url_,
HttpClient::Headers{GetAuthHeader(), GetJsonHeader()}, _, _))
.WillOnce(WithArgs<3>(Invoke([](const std::string& data) {
- EXPECT_JSON_EQ(R"({"state":"inProgress"})",
- *CreateDictionaryValue(data));
- base::DictionaryValue json;
- return ReplyWithJson(200, json);
- })))
- .WillOnce(WithArgs<3>(Invoke([](const std::string& data) {
- EXPECT_JSON_EQ(R"({"progress":{"progress":18}})",
+ EXPECT_JSON_EQ(R"({"state":"inProgress", "progress":{"progress":18}})",
*CreateDictionaryValue(data));
base::DictionaryValue json;
return ReplyWithJson(200, json);
})));
EXPECT_TRUE(command_->SetProgress(*CreateDictionaryValue("{'progress':18}"),
nullptr));
- Mock::VerifyAndClearExpectations(&http_client_);
}
TEST_F(DeviceRegistrationInfoUpdateCommandTest, SetResults) {
@@ -571,19 +571,13 @@
http::kPatch, command_url_,
HttpClient::Headers{GetAuthHeader(), GetJsonHeader()}, _, _))
.WillOnce(WithArgs<3>(Invoke([](const std::string& data) {
- EXPECT_JSON_EQ(R"({"results":{"status":"Ok"}})",
+ EXPECT_JSON_EQ(R"({"state":"done", "results":{"status":"Ok"}})",
*CreateDictionaryValue(data));
base::DictionaryValue json;
return ReplyWithJson(200, json);
- })))
- .WillOnce(WithArgs<3>(Invoke([](const std::string& data) {
- EXPECT_JSON_EQ(R"({"state":"done"})", *CreateDictionaryValue(data));
- base::DictionaryValue json;
- return ReplyWithJson(200, json);
})));
EXPECT_TRUE(command_->SetResults(*CreateDictionaryValue("{'status': 'Ok'}"),
nullptr));
- Mock::VerifyAndClearExpectations(&http_client_);
}
TEST_F(DeviceRegistrationInfoUpdateCommandTest, Cancel) {
@@ -598,7 +592,6 @@
return ReplyWithJson(200, json);
})));
EXPECT_TRUE(command_->Cancel(nullptr));
- Mock::VerifyAndClearExpectations(&http_client_);
}
} // namespace weave
