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