update_manager: Make Prefs Variable async.
The update_engine prefs, while stored in disk, are private daemon
values changed by the daemon only. There was a 5 minutes delay between
changing this value and the update policy checking it again, and there
was a log spam every 5 minutes due to policy re-evaluations.
This patch makes these Prefs-based variables async by implementing an
observer pattern in the PrefsInterface and makes these variables async.
Bug: chromium:367333
Test: Added uniittest. No more log spam every 5 minutes.
Change-Id: I8b3f7072cc87515972c9f5b1ddcc54b865ffe238
diff --git a/update_manager/fake_updater_provider.h b/update_manager/fake_updater_provider.h
index 7d2d3f6..44389f4 100644
--- a/update_manager/fake_updater_provider.h
+++ b/update_manager/fake_updater_provider.h
@@ -106,10 +106,12 @@
"curr_channel", kVariableModePoll};
FakeVariable<std::string> var_new_channel_{ // NOLINT(whitespace/braces)
"new_channel", kVariableModePoll};
- FakeVariable<bool> var_p2p_enabled_{ // NOLINT(whitespace/braces)
- "p2p_enabled", kVariableModePoll};
- FakeVariable<bool> var_cellular_enabled_{ // NOLINT(whitespace/braces)
- "cellular_enabled", kVariableModePoll};
+ FakeVariable<bool> var_p2p_enabled_{// NOLINT(whitespace/braces)
+ "p2p_enabled",
+ kVariableModeAsync};
+ FakeVariable<bool> var_cellular_enabled_{// NOLINT(whitespace/braces)
+ "cellular_enabled",
+ kVariableModeAsync};
FakeVariable<unsigned int>
var_consecutive_failed_update_checks_{ // NOLINT(whitespace/braces)
"consecutive_failed_update_checks", kVariableModePoll};
diff --git a/update_manager/real_updater_provider.cc b/update_manager/real_updater_provider.cc
index 14d60d2..1c2868d 100644
--- a/update_manager/real_updater_provider.cc
+++ b/update_manager/real_updater_provider.cc
@@ -298,25 +298,42 @@
};
// A variable class for reading Boolean prefs values.
-class BooleanPrefVariable : public UpdaterVariableBase<bool> {
+class BooleanPrefVariable
+ : public AsyncCopyVariable<bool>,
+ public chromeos_update_engine::PrefsInterface::ObserverInterface {
public:
- BooleanPrefVariable(const string& name, SystemState* system_state,
- const char* key, bool default_val)
- : UpdaterVariableBase<bool>(name, kVariableModePoll, system_state),
- key_(key), default_val_(default_val) {}
+ BooleanPrefVariable(const string& name,
+ chromeos_update_engine::PrefsInterface* prefs,
+ const char* key,
+ bool default_val)
+ : AsyncCopyVariable<bool>(name),
+ prefs_(prefs),
+ key_(key),
+ default_val_(default_val) {
+ prefs->AddObserver(key, this);
+ OnPrefSet(key);
+ }
+ ~BooleanPrefVariable() {
+ prefs_->RemoveObserver(key_, this);
+ }
private:
- const bool* GetValue(TimeDelta /* timeout */, string* errmsg) override {
+ // Reads the actual value from the Prefs instance and updates the Variable
+ // value.
+ void OnPrefSet(const string& key) override {
bool result = default_val_;
- chromeos_update_engine::PrefsInterface* prefs = system_state()->prefs();
- if (prefs && prefs->Exists(key_) && !prefs->GetBoolean(key_, &result)) {
- if (errmsg)
- *errmsg = string("Could not read boolean pref ") + key_;
- return nullptr;
- }
- return new bool(result);
+ if (prefs_ && prefs_->Exists(key_) && !prefs_->GetBoolean(key_, &result))
+ result = default_val_;
+ // AsyncCopyVariable will take care of values that didn't change.
+ SetValue(result);
}
+ void OnPrefDeleted(const string& key) override {
+ SetValue(default_val_);
+ }
+
+ chromeos_update_engine::PrefsInterface* prefs_;
+
// The Boolean preference key and default value.
const char* const key_;
const bool default_val_;
@@ -415,12 +432,12 @@
var_curr_channel_(new CurrChannelVariable("curr_channel", system_state_)),
var_new_channel_(new NewChannelVariable("new_channel", system_state_)),
var_p2p_enabled_(
- new BooleanPrefVariable("p2p_enabled", system_state_,
+ new BooleanPrefVariable("p2p_enabled", system_state_->prefs(),
chromeos_update_engine::kPrefsP2PEnabled,
false)),
var_cellular_enabled_(
new BooleanPrefVariable(
- "cellular_enabled", system_state_,
+ "cellular_enabled", system_state_->prefs(),
chromeos_update_engine::kPrefsUpdateOverCellularPermission,
false)),
var_consecutive_failed_update_checks_(
diff --git a/update_manager/real_updater_provider_unittest.cc b/update_manager/real_updater_provider_unittest.cc
index 4187427..2f0f9dd 100644
--- a/update_manager/real_updater_provider_unittest.cc
+++ b/update_manager/real_updater_provider_unittest.cc
@@ -24,8 +24,8 @@
#include <update_engine/dbus-constants.h>
#include "update_engine/fake_clock.h"
+#include "update_engine/fake_prefs.h"
#include "update_engine/fake_system_state.h"
-#include "update_engine/mock_prefs.h"
#include "update_engine/mock_update_attempter.h"
#include "update_engine/omaha_request_params.h"
#include "update_engine/update_manager/umtest_utils.h"
@@ -33,14 +33,13 @@
using base::Time;
using base::TimeDelta;
using chromeos_update_engine::FakeClock;
+using chromeos_update_engine::FakePrefs;
using chromeos_update_engine::FakeSystemState;
-using chromeos_update_engine::MockPrefs;
using chromeos_update_engine::OmahaRequestParams;
using std::string;
using std::unique_ptr;
using testing::Return;
using testing::SetArgPointee;
-using testing::StrEq;
using testing::_;
namespace {
@@ -76,30 +75,13 @@
protected:
void SetUp() override {
fake_clock_ = fake_sys_state_.fake_clock();
+ fake_sys_state_.set_prefs(&fake_prefs_);
provider_.reset(new RealUpdaterProvider(&fake_sys_state_));
ASSERT_NE(nullptr, provider_.get());
// Check that provider initializes correctly.
ASSERT_TRUE(provider_->Init());
}
- // Sets up mock expectations for testing a variable that reads a Boolean pref
- // |key|. |key_exists| determines whether the key is present. If it is, then
- // |get_boolean_success| determines whether reading it is successful, and if
- // so |output| is the value being read.
- void SetupReadBooleanPref(const char* key, bool key_exists,
- bool get_boolean_success, bool output) {
- MockPrefs* const mock_prefs = fake_sys_state_.mock_prefs();
- EXPECT_CALL(*mock_prefs, Exists(StrEq(key))).WillOnce(Return(key_exists));
- if (key_exists) {
- auto& get_boolean = EXPECT_CALL(
- *fake_sys_state_.mock_prefs(), GetBoolean(StrEq(key), _));
- if (get_boolean_success)
- get_boolean.WillOnce(DoAll(SetArgPointee<1>(output), Return(true)));
- else
- get_boolean.WillOnce(Return(false));
- }
- }
-
// Sets up mock expectations for testing the update completed time reporting.
// |valid| determines whether the returned time is valid. Returns the expected
// update completed time value.
@@ -120,6 +102,7 @@
FakeSystemState fake_sys_state_;
FakeClock* fake_clock_; // Short for fake_sys_state_.fake_clock()
+ FakePrefs fake_prefs_;
unique_ptr<RealUpdaterProvider> provider_;
};
@@ -389,57 +372,39 @@
}
TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledOkayPrefDoesntExist) {
- SetupReadBooleanPref(chromeos_update_engine::kPrefsP2PEnabled,
- false, false, false);
UmTestUtils::ExpectVariableHasValue(false, provider_->var_p2p_enabled());
}
TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledOkayPrefReadsFalse) {
- SetupReadBooleanPref(chromeos_update_engine::kPrefsP2PEnabled,
- true, true, false);
+ fake_prefs_.SetBoolean(chromeos_update_engine::kPrefsP2PEnabled, false);
UmTestUtils::ExpectVariableHasValue(false, provider_->var_p2p_enabled());
}
-TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledOkayPrefReadsTrue) {
- SetupReadBooleanPref(chromeos_update_engine::kPrefsP2PEnabled,
- true, true, true);
+TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledReadWhenInitialized) {
+ fake_prefs_.SetBoolean(chromeos_update_engine::kPrefsP2PEnabled, true);
+ SetUp();
UmTestUtils::ExpectVariableHasValue(true, provider_->var_p2p_enabled());
}
-TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledFailCannotReadPref) {
- SetupReadBooleanPref(chromeos_update_engine::kPrefsP2PEnabled,
- true, false, false);
- UmTestUtils::ExpectVariableNotSet(provider_->var_p2p_enabled());
+TEST_F(UmRealUpdaterProviderTest, GetP2PEnabledUpdated) {
+ fake_prefs_.SetBoolean(chromeos_update_engine::kPrefsP2PEnabled, false);
+ UmTestUtils::ExpectVariableHasValue(false, provider_->var_p2p_enabled());
+ fake_prefs_.SetBoolean(chromeos_update_engine::kPrefsP2PEnabled, true);
+ UmTestUtils::ExpectVariableHasValue(true, provider_->var_p2p_enabled());
+ fake_prefs_.Delete(chromeos_update_engine::kPrefsP2PEnabled);
+ UmTestUtils::ExpectVariableHasValue(false, provider_->var_p2p_enabled());
}
TEST_F(UmRealUpdaterProviderTest, GetCellularEnabledOkayPrefDoesntExist) {
- SetupReadBooleanPref(
- chromeos_update_engine::kPrefsUpdateOverCellularPermission,
- false, false, false);
- UmTestUtils::ExpectVariableHasValue(false, provider_->var_cellular_enabled());
-}
-
-TEST_F(UmRealUpdaterProviderTest, GetCellularEnabledOkayPrefReadsFalse) {
- SetupReadBooleanPref(
- chromeos_update_engine::kPrefsUpdateOverCellularPermission,
- true, true, false);
UmTestUtils::ExpectVariableHasValue(false, provider_->var_cellular_enabled());
}
TEST_F(UmRealUpdaterProviderTest, GetCellularEnabledOkayPrefReadsTrue) {
- SetupReadBooleanPref(
- chromeos_update_engine::kPrefsUpdateOverCellularPermission,
- true, true, true);
+ fake_prefs_.SetBoolean(
+ chromeos_update_engine::kPrefsUpdateOverCellularPermission, true);
UmTestUtils::ExpectVariableHasValue(true, provider_->var_cellular_enabled());
}
-TEST_F(UmRealUpdaterProviderTest, GetCellularEnabledFailCannotReadPref) {
- SetupReadBooleanPref(
- chromeos_update_engine::kPrefsUpdateOverCellularPermission,
- true, false, false);
- UmTestUtils::ExpectVariableNotSet(provider_->var_cellular_enabled());
-}
-
TEST_F(UmRealUpdaterProviderTest, GetUpdateCompletedTimeOkay) {
Time expected = SetupUpdateCompletedTime(true);
UmTestUtils::ExpectVariableHasValue(expected,