PolicyManager: Schedule re-evaluations based on variable usage.
This patch makes the EvaluationContext re-schedule a policy request
based on the variables used by that method, waiting for the Async
variables and polling the Poll variables on the suggested interval.
In order to use the main loop functions from the EvaluationContext
they were moved to its own file called event_loop.h.
BUG=chromium:340871
TEST=Unit tests added.
Change-Id: Ibfc52e4dfd12c5e1ef87b5ad9cc318f9821dcfdd
Reviewed-on: https://chromium-review.googlesource.com/190424
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
diff --git a/policy_manager/evaluation_context.cc b/policy_manager/evaluation_context.cc
index 062ed26..c756ddf 100644
--- a/policy_manager/evaluation_context.cc
+++ b/policy_manager/evaluation_context.cc
@@ -4,14 +4,99 @@
#include "update_engine/policy_manager/evaluation_context.h"
+#include <base/bind.h>
+
+using base::Closure;
using base::TimeDelta;
namespace chromeos_policy_manager {
+EvaluationContext::~EvaluationContext() {
+ RemoveObserversAndTimeout();
+}
+
+void EvaluationContext::RemoveObserversAndTimeout() {
+ for (auto& it : value_cache_) {
+ if (it.first->GetMode() == kVariableModeAsync)
+ it.first->RemoveObserver(this);
+ }
+ CancelMainLoopEvent(poll_timeout_event_);
+ poll_timeout_event_ = kEventIdNull;
+}
+
TimeDelta EvaluationContext::RemainingTime() const {
// TODO(deymo): Return a timeout based on the elapsed time on the current
// policy request evaluation.
return TimeDelta::FromSeconds(1.);
}
+void EvaluationContext::ValueChanged(BaseVariable* var) {
+ DLOG(INFO) << "ValueChanged for variable " << var->GetName();
+ OnValueChangedOrPollTimeout();
+}
+
+void EvaluationContext::OnPollTimeout() {
+ DLOG(INFO) << "OnPollTimeout() called.";
+ poll_timeout_event_ = kEventIdNull;
+ OnValueChangedOrPollTimeout();
+}
+
+void EvaluationContext::OnValueChangedOrPollTimeout() {
+ RemoveObserversAndTimeout();
+ // Remove the cached values of non-const variables
+ for (auto it = value_cache_.begin(); it != value_cache_.end(); ) {
+ if (it->first->GetMode() == kVariableModeConst) {
+ ++it;
+ } else {
+ it = value_cache_.erase(it);
+ }
+ }
+
+ if (value_changed_callback_.get() != NULL) {
+ value_changed_callback_->Run();
+ value_changed_callback_.reset();
+ }
+}
+
+bool EvaluationContext::RunOnValueChangeOrTimeout(Closure callback) {
+ TimeDelta reeval_timeout;
+ bool reeval_timeout_set = false;
+ bool waiting_for_value_change = false;
+
+ if (value_changed_callback_.get() != NULL) {
+ LOG(ERROR) << "RunOnValueChangeOrTimeout called more than once.";
+ return false;
+ }
+
+ for (auto& it : value_cache_) {
+ switch (it.first->GetMode()) {
+ case kVariableModeAsync:
+ waiting_for_value_change = true;
+ DLOG(INFO) << "Waiting for value on " << it.first->GetName();
+ it.first->AddObserver(this);
+ break;
+ case kVariableModePoll:
+ if (!reeval_timeout_set || reeval_timeout > it.first->GetPollInterval())
+ reeval_timeout = it.first->GetPollInterval();
+ reeval_timeout_set = true;
+ break;
+ case kVariableModeConst:
+ // Ignored.
+ break;
+ }
+ }
+ // Check if the re-evaluation is actually being scheduled. If there are no
+ // events waited for, this function should return false.
+ if (!waiting_for_value_change && !reeval_timeout_set)
+ return false;
+ if (reeval_timeout_set) {
+ poll_timeout_event_ = RunFromMainLoopAfterTimeout(
+ base::Bind(&EvaluationContext::OnPollTimeout, this), // refcounted.
+ reeval_timeout);
+ }
+
+ value_changed_callback_.reset(new Closure(callback));
+ return true;
+}
+
} // namespace chromeos_policy_manager
diff --git a/policy_manager/evaluation_context.h b/policy_manager/evaluation_context.h
index 27998d6..9e8b3c2 100644
--- a/policy_manager/evaluation_context.h
+++ b/policy_manager/evaluation_context.h
@@ -7,19 +7,25 @@
#include <map>
+#include <base/callback.h>
#include <base/memory/ref_counted.h>
+#include <base/time.h>
-#include "update_engine/policy_manager/variable.h"
#include "update_engine/policy_manager/boxed_value.h"
+#include "update_engine/policy_manager/event_loop.h"
+#include "update_engine/policy_manager/variable.h"
namespace chromeos_policy_manager {
// The EvaluationContext class is the interface between a policy implementation
// and the state. The EvaluationContext tracks the variables used by a policy
// request and caches the returned values, owning those cached values.
-class EvaluationContext : public base::RefCounted<EvaluationContext> {
+class EvaluationContext :
+ public base::RefCounted<EvaluationContext>,
+ private BaseVariable::ObserverInterface {
public:
EvaluationContext() {}
+ ~EvaluationContext();
// Returns a pointer to the value returned by the passed variable |var|. The
// EvaluationContext instance keeps the ownership of the returned object. The
@@ -30,7 +36,30 @@
template<typename T>
const T* GetValue(Variable<T>* var);
+ // Schedules the passed |callback| closure to be called when a cached
+ // variable changes its value or a polling interval passes. If none of these
+ // events can happen, for example if there's no cached variable, this method
+ // returns false.
+ //
+ // Right before the passed closure is called the EvaluationContext is
+ // reseted, removing all the non-const cached values.
+ bool RunOnValueChangeOrTimeout(base::Closure callback);
+
private:
+ // Removes all the Observers and timeout callbacks scheduled by
+ // RunOnValueChangeOrTimeout(). This method is idempotent.
+ void RemoveObserversAndTimeout();
+
+ // BaseVariable::ObserverInterface override.
+ void ValueChanged(BaseVariable* var);
+
+ // Called from the main loop when the scheduled poll timeout has passed.
+ void OnPollTimeout();
+
+ // Removes the observers from the used Variables and cancels the poll timeout
+ // and executes the scheduled callback, if any.
+ void OnValueChangedOrPollTimeout();
+
// The remaining time for the current evaluation.
base::TimeDelta RemainingTime() const;
@@ -40,6 +69,13 @@
// The cached values of the called Variables.
ValueCacheMap value_cache_;
+ // A pointer to a copy of the closure passed to RunOnValueChangeOrTimeout().
+ scoped_ptr<base::Closure> value_changed_callback_;
+
+ // The EventId returned by the event loop identifying the timeout callback.
+ // Used to cancel the timeout callback.
+ EventId poll_timeout_event_ = kEventIdNull;
+
DISALLOW_COPY_AND_ASSIGN(EvaluationContext);
};
diff --git a/policy_manager/evaluation_context_unittest.cc b/policy_manager/evaluation_context_unittest.cc
index 6ef6f74..d56b5a2 100644
--- a/policy_manager/evaluation_context_unittest.cc
+++ b/policy_manager/evaluation_context_unittest.cc
@@ -2,30 +2,69 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include <base/memory/scoped_ptr.h>
-#include <gtest/gtest.h>
#include <string>
+#include <base/bind.h>
+#include <base/memory/scoped_ptr.h>
+#include <gtest/gtest.h>
+
#include "update_engine/policy_manager/evaluation_context.h"
+#include "update_engine/policy_manager/fake_variable.h"
#include "update_engine/policy_manager/generic_variables.h"
#include "update_engine/policy_manager/pmtest_utils.h"
-#include "update_engine/policy_manager/fake_variable.h"
+#include "update_engine/test_utils.h"
+using base::Bind;
+using base::TimeDelta;
+using chromeos_update_engine::RunGMainLoopMaxIterations;
+using chromeos_update_engine::RunGMainLoopUntil;
using std::string;
+namespace {
+
+void DoNothing() {}
+
+// Sets the value of the passed pointer to true.
+void SetTrue(bool* value) {
+ *value = true;
+}
+
+bool GetBoolean(bool* value) {
+ return *value;
+}
+
+} // namespace
+
namespace chromeos_policy_manager {
class PmEvaluationContextTest : public ::testing::Test {
public:
- PmEvaluationContextTest() : fake_int_var_("fake_int", kVariableModePoll) {}
+ PmEvaluationContextTest() {}
protected:
virtual void SetUp() {
eval_ctx_ = new EvaluationContext();
}
+ virtual void TearDown() {
+ eval_ctx_ = NULL;
+ // Check that the evaluation context removed all the observers.
+ EXPECT_TRUE(fake_int_var_.observer_list_.empty());
+ EXPECT_TRUE(fake_async_var_.observer_list_.empty());
+ EXPECT_TRUE(fake_const_var_.observer_list_.empty());
+ EXPECT_TRUE(fake_poll_var_.observer_list_.empty());
+ }
+
scoped_refptr<EvaluationContext> eval_ctx_;
- FakeVariable<int> fake_int_var_;
+
+ // FakeVariables used for testing the EvaluationContext. These are required
+ // here to prevent them from going away *before* the EvaluationContext under
+ // test does, which keeps a reference to them.
+ FakeVariable<int> fake_int_var_ = {"fake_int", kVariableModePoll};
+ FakeVariable<string> fake_async_var_ = {"fake_async", kVariableModeAsync};
+ FakeVariable<string> fake_const_var_ = {"fake_const", kVariableModeConst};
+ FakeVariable<string> fake_poll_var_ = {"fake_poll",
+ TimeDelta::FromSeconds(1)};
};
TEST_F(PmEvaluationContextTest, GetValueFails) {
@@ -75,16 +114,15 @@
}
TEST_F(PmEvaluationContextTest, GetValueMixedTypes) {
- FakeVariable<string> fake_string_var_("fake_string", kVariableModePoll);
const int* p_fake_int;
const string* p_fake_string;
fake_int_var_.reset(new int(42));
- fake_string_var_.reset(new string("Hello world!"));
+ fake_poll_var_.reset(new string("Hello world!"));
// Check that the EvaluationContext can handle multiple Variable types. This
// is mostly a compile-time check due to the template nature of this method.
p_fake_int = eval_ctx_->GetValue(&fake_int_var_);
- p_fake_string = eval_ctx_->GetValue(&fake_string_var_);
+ p_fake_string = eval_ctx_->GetValue(&fake_poll_var_);
PMTEST_ASSERT_NOT_NULL(p_fake_int);
EXPECT_EQ(42, *p_fake_int);
@@ -93,4 +131,76 @@
EXPECT_EQ("Hello world!", *p_fake_string);
}
+// Test that we don't schedule an event if there's no variable to wait for.
+TEST_F(PmEvaluationContextTest, RunOnValueChangeOrTimeoutWithoutVariablesTest) {
+ fake_const_var_.reset(new string("Hello world!"));
+ EXPECT_EQ(*eval_ctx_->GetValue(&fake_const_var_), "Hello world!");
+
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&DoNothing)));
+}
+
+// Test that we don't schedule an event if there's no variable to wait for.
+TEST_F(PmEvaluationContextTest, RunOnValueChangeOrTimeoutWithVariablesTest) {
+ fake_async_var_.reset(new string("Async value"));
+ eval_ctx_->GetValue(&fake_async_var_);
+
+ bool value = false;
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&SetTrue, &value)));
+ // Check that the scheduled callback isn't run until we signal a ValueChaged.
+ RunGMainLoopMaxIterations(100);
+ EXPECT_FALSE(value);
+
+ fake_async_var_.NotifyValueChanged();
+ EXPECT_FALSE(value);
+ // Ensure that the scheduled callback isn't run until we are back on the main
+ // loop.
+ RunGMainLoopMaxIterations(100);
+ EXPECT_TRUE(value);
+}
+
+// Test that we don't re-schedule the events if we are attending one.
+TEST_F(PmEvaluationContextTest, RunOnValueChangeOrTimeoutCalledTwiceTest) {
+ fake_async_var_.reset(new string("Async value"));
+ eval_ctx_->GetValue(&fake_async_var_);
+
+ bool value = false;
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&SetTrue, &value)));
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&SetTrue, &value)));
+
+ // The scheduled event should still work.
+ fake_async_var_.NotifyValueChanged();
+ RunGMainLoopMaxIterations(100);
+ EXPECT_TRUE(value);
+}
+
+// Test that we clear the events when destroying the EvaluationContext.
+TEST_F(PmEvaluationContextTest, RemoveObserversAndTimeoutTest) {
+ fake_async_var_.reset(new string("Async value"));
+ eval_ctx_->GetValue(&fake_async_var_);
+
+ bool value = false;
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&SetTrue, &value)));
+ eval_ctx_ = NULL;
+
+ // This should not trigger the callback since the EvaluationContext waiting
+ // for it is gone, and it should have remove all its observers.
+ fake_async_var_.NotifyValueChanged();
+ RunGMainLoopMaxIterations(100);
+ EXPECT_FALSE(value);
+}
+
+// Test that we don't schedule an event if there's no variable to wait for.
+TEST_F(PmEvaluationContextTest, RunOnValueChangeOrTimeoutRunsFromTimeoutTest) {
+ fake_poll_var_.reset(new string("Polled value"));
+ eval_ctx_->GetValue(&fake_poll_var_);
+
+ bool value = false;
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&SetTrue, &value)));
+ // Check that the scheduled callback isn't run until the timeout occurs.
+ RunGMainLoopMaxIterations(10);
+ EXPECT_FALSE(value);
+ RunGMainLoopUntil(10000, Bind(&GetBoolean, &value));
+ EXPECT_TRUE(value);
+}
+
} // namespace chromeos_policy_manager
diff --git a/policy_manager/event_loop.cc b/policy_manager/event_loop.cc
new file mode 100644
index 0000000..dc6eb30
--- /dev/null
+++ b/policy_manager/event_loop.cc
@@ -0,0 +1,51 @@
+// Copyright (c) 2014 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "update_engine/policy_manager/event_loop.h"
+
+#include <cmath>
+
+using base::Closure;
+
+namespace {
+
+// Called by the GLib's main loop when is time to call the callback scheduled
+// with RunFromMainLopp() and similar functions. The pointer to the callback
+// passed when scheduling it is passed to this functions as a gpointer on
+// |user_data|.
+gboolean OnRanFromMainLoop(gpointer user_data) {
+ Closure* callback_p = reinterpret_cast<Closure*>(user_data);
+ callback_p->Run();
+ delete callback_p;
+ return FALSE; // Removes the source since a callback can only be called once.
+}
+
+} // namespace
+
+namespace chromeos_policy_manager {
+
+EventId RunFromMainLoop(const Closure& callback) {
+ Closure* callback_p = new Closure(callback);
+ return g_idle_add_full(G_PRIORITY_DEFAULT,
+ OnRanFromMainLoop,
+ reinterpret_cast<gpointer>(callback_p),
+ NULL);
+}
+
+EventId RunFromMainLoopAfterTimeout(
+ const Closure& callback,
+ base::TimeDelta timeout) {
+ Closure* callback_p = new Closure(callback);
+ return g_timeout_add_seconds(static_cast<guint>(ceil(timeout.InSecondsF())),
+ OnRanFromMainLoop,
+ reinterpret_cast<gpointer>(callback_p));
+}
+
+bool CancelMainLoopEvent(EventId event) {
+ if (event != kEventIdNull)
+ return g_source_remove(event);
+ return false;
+}
+
+} // namespace chromeos_policy_manager
diff --git a/policy_manager/event_loop.h b/policy_manager/event_loop.h
new file mode 100644
index 0000000..a8474ba
--- /dev/null
+++ b/policy_manager/event_loop.h
@@ -0,0 +1,37 @@
+// Copyright (c) 2014 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// TODO(deymo): These functions interact with the glib's main loop. This should
+// be replaced by the libbase main loop once the process is migrated to that
+// main loop.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_EVENT_LOOP_H_
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_EVENT_LOOP_H_
+
+#include <glib.h>
+
+#include <base/callback.h>
+#include <base/time.h>
+
+namespace chromeos_policy_manager {
+
+typedef guint EventId;
+
+// A null EventId doesn't idenify any valid event.
+static constexpr EventId kEventIdNull = 0;
+
+// Schedules the passed |callback| to run from the GLib's main loop after a
+// timeout if it is given.
+EventId RunFromMainLoop(const base::Closure& callback);
+EventId RunFromMainLoopAfterTimeout(const base::Closure& callback,
+ base::TimeDelta timeout);
+
+// Removes the pending call |event| from the main loop. The value passed is the
+// one returned by the functions RunFromMainLoop*() when the call was scheduled.
+// Returns whether the event was found and removed.
+bool CancelMainLoopEvent(EventId event);
+
+} // namespace chromeos_policy_manager
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_EVENT_LOOP_H_
diff --git a/policy_manager/event_loop_unittest.cc b/policy_manager/event_loop_unittest.cc
new file mode 100644
index 0000000..e568594
--- /dev/null
+++ b/policy_manager/event_loop_unittest.cc
@@ -0,0 +1,62 @@
+// Copyright (c) 2014 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "update_engine/policy_manager/event_loop.h"
+
+#include <base/bind.h>
+#include <gtest/gtest.h>
+
+#include "update_engine/test_utils.h"
+
+using base::Bind;
+using base::TimeDelta;
+using chromeos_update_engine::RunGMainLoopMaxIterations;
+using chromeos_update_engine::RunGMainLoopUntil;
+
+namespace {
+
+// Sets the value of the passed pointer to true.
+void SetTrue(bool* value) {
+ *value = true;
+}
+
+bool GetBoolean(bool* value) {
+ return *value;
+}
+
+} // namespace
+
+namespace chromeos_policy_manager {
+
+class EventLoopTest : public ::testing::Test {};
+
+TEST(EventLoopTest, RunFromMainLoopTest) {
+ bool called = false;
+ EventId ev = RunFromMainLoop(Bind(SetTrue, &called));
+ EXPECT_NE(0, ev);
+ RunGMainLoopMaxIterations(100);
+ EXPECT_TRUE(called);
+}
+
+// Tests that we can cancel events right after we schedule them.
+TEST(EventLoopTest, RunFromMainLoopCancelTest) {
+ bool called = false;
+ EventId ev = RunFromMainLoop(Bind(SetTrue, &called));
+ EXPECT_NE(0, ev);
+ EXPECT_TRUE(CancelMainLoopEvent(ev));
+ RunGMainLoopMaxIterations(100);
+ EXPECT_FALSE(called);
+}
+
+TEST(EventLoopTest, RunFromMainLoopAfterTimeoutTest) {
+ bool called = false;
+ EventId ev = RunFromMainLoopAfterTimeout(Bind(SetTrue, &called),
+ TimeDelta::FromSeconds(1));
+ EXPECT_NE(0, ev);
+ RunGMainLoopUntil(10000, Bind(GetBoolean, &called));
+ // Check that the main loop finished before the 10 seconds timeout.
+ EXPECT_TRUE(called);
+}
+
+} // namespace chromeos_policy_manager
diff --git a/policy_manager/fake_variable.h b/policy_manager/fake_variable.h
index bd113a3..36c3848 100644
--- a/policy_manager/fake_variable.h
+++ b/policy_manager/fake_variable.h
@@ -5,6 +5,8 @@
#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_FAKE_VARIABLE_H_
#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_FAKE_VARIABLE_H_
+#include <string>
+
#include <base/memory/scoped_ptr.h>
#include "update_engine/policy_manager/variable.h"
@@ -16,8 +18,10 @@
template<typename T>
class FakeVariable : public Variable<T> {
public:
- explicit FakeVariable(const std::string& name, VariableMode mode)
+ FakeVariable(const std::string& name, VariableMode mode)
: Variable<T>(name, mode) {}
+ FakeVariable(const std::string& name, base::TimeDelta poll_interval)
+ : Variable<T>(name, poll_interval) {}
virtual ~FakeVariable() {}
// Sets the next value of this variable to the passed |p_value| pointer. Once
@@ -27,6 +31,11 @@
ptr_.reset(p_value);
}
+ // Make the NotifyValueChanged() public for FakeVariables.
+ void NotifyValueChanged() {
+ Variable<T>::NotifyValueChanged();
+ }
+
protected:
// Variable<T> overrides.
// Returns the pointer set with reset(). The ownership of the object is passed
diff --git a/policy_manager/policy_manager-inl.h b/policy_manager/policy_manager-inl.h
index 4317e77..f0d4937 100644
--- a/policy_manager/policy_manager-inl.h
+++ b/policy_manager/policy_manager-inl.h
@@ -5,9 +5,12 @@
#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_POLICY_MANAGER_INL_H_
#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_POLICY_MANAGER_INL_H_
+#include <string>
+
#include <base/bind.h>
#include "update_engine/policy_manager/evaluation_context.h"
+#include "update_engine/policy_manager/event_loop.h"
namespace chromeos_policy_manager {
@@ -35,7 +38,6 @@
return status;
}
-
template<typename T, typename R, typename... Args>
void PolicyManager::OnPolicyReadyToEvaluate(
scoped_refptr<EvaluationContext> ec,
@@ -49,14 +51,20 @@
callback.Run(status, result);
return;
}
- // Re-schedule the policy request.
- // TODO(deymo): Use the information gathered on the EvaluationContext to
- // hook on proper events from changes on the State in order to re-evaluate
- // the policy after some period of time.
+ // Re-schedule the policy request based on used variables.
base::Closure closure = base::Bind(
&PolicyManager::OnPolicyReadyToEvaluate<T, R, Args...>,
base::Unretained(this), ec, callback, policy_method, args...);
- RunFromMainLoopAfterTimeout(closure, base::TimeDelta::FromSeconds(20));
+
+ if (!ec->RunOnValueChangeOrTimeout(closure)) {
+ // The policy method didn't use any non-const variable nor there's any
+ // time-based event that will change the status of evaluation. We call the
+ // callback with EvalStatus::kAskMeAgainLater.
+ LOG(ERROR) << "Policy implementation didn't use any non-const variable "
+ "but returned kAskMeAgainLater.";
+ callback.Run(EvalStatus::kAskMeAgainLater, result);
+ return;
+ }
}
template<typename T, typename R, typename... Args>
diff --git a/policy_manager/policy_manager.cc b/policy_manager/policy_manager.cc
index 1357707..4f19f99 100644
--- a/policy_manager/policy_manager.cc
+++ b/policy_manager/policy_manager.cc
@@ -13,8 +13,8 @@
template <typename T>
bool InitProvider(scoped_ptr<T>* handle_ptr, T* provider) {
- handle_ptr->reset(provider);
- return handle_ptr->get() && (*handle_ptr)->Init();
+ handle_ptr->reset(provider);
+ return handle_ptr->get() && (*handle_ptr)->Init();
}
bool PolicyManager::Init(State* state) {
@@ -27,28 +27,4 @@
return true;
}
-void PolicyManager::RunFromMainLoop(const Closure& callback) {
- Closure* callback_p = new base::Closure(callback);
- g_idle_add_full(G_PRIORITY_DEFAULT,
- OnRanFromMainLoop,
- reinterpret_cast<gpointer>(callback_p),
- NULL);
-}
-
-void PolicyManager::RunFromMainLoopAfterTimeout(
- const Closure& callback,
- base::TimeDelta timeout) {
- Closure* callback_p = new Closure(callback);
- g_timeout_add_seconds(timeout.InSeconds(),
- OnRanFromMainLoop,
- reinterpret_cast<gpointer>(callback_p));
-}
-
-gboolean PolicyManager::OnRanFromMainLoop(gpointer user_data) {
- Closure* callback_p = reinterpret_cast<Closure*>(user_data);
- callback_p->Run();
- delete callback_p;
- return FALSE; // Removes the source since a callback can only be called once.
-}
-
} // namespace chromeos_policy_manager
diff --git a/policy_manager/policy_manager.h b/policy_manager/policy_manager.h
index 33bb89c..4e4cc68 100644
--- a/policy_manager/policy_manager.h
+++ b/policy_manager/policy_manager.h
@@ -5,8 +5,6 @@
#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_POLICY_MANAGER_H_
#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_POLICY_MANAGER_H_
-#include <glib.h>
-
#include <base/callback.h>
#include <base/memory/ref_counted.h>
#include <base/memory/scoped_ptr.h>
@@ -51,7 +49,9 @@
//
// If the policy implementation should block, returning a
// EvalStatus::kAskMeAgainLater status the policy manager will re-evaluate the
- // policy until another status is returned.
+ // policy until another status is returned. If the policy implementation based
+ // its return value solely on const variables, the callback will be called
+ // with the EvalStatus::kAskMeAgainLater status.
template<typename T, typename R, typename... Args>
void AsyncPolicyRequest(
base::Callback<void(EvalStatus, const R& result)> callback,
@@ -64,18 +64,6 @@
FRIEND_TEST(PmPolicyManagerTest, PolicyRequestDoesntBlock);
FRIEND_TEST(PmPolicyManagerTest, AsyncPolicyRequestDelaysEvaluation);
- // Schedules the passed |callback| to run from the GLib's main loop after a
- // timeout if it is given.
- static void RunFromMainLoop(const base::Closure& callback);
- static void RunFromMainLoopAfterTimeout(const base::Closure& callback,
- base::TimeDelta timeout);
-
- // Called by the GLib's main loop when is time to call the callback scheduled
- // with RunFromMainLopp() and similar functions. The pointer to the callback
- // passed when scheduling it is passed to this functions as a gpointer on
- // |user_data|.
- static gboolean OnRanFromMainLoop(gpointer user_data);
-
// EvaluatePolicy() evaluates the passed |policy_method| method on the current
// policy with the given |args| arguments. If the method fails, the default
// policy is used instead.
diff --git a/policy_manager/variable.h b/policy_manager/variable.h
index b00eca4..7111a99 100644
--- a/policy_manager/variable.h
+++ b/policy_manager/variable.h
@@ -9,9 +9,13 @@
#include <list>
#include <string>
+#include <base/bind.h>
+#include <base/logging.h>
#include <base/time.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
+#include "update_engine/policy_manager/event_loop.h"
+
namespace chromeos_policy_manager {
// The VariableMode specifies important behavior of the variable in terms of
@@ -40,15 +44,20 @@
class BaseVariable {
public:
// Interface for observing changes on variable value.
- class Observer {
+ class ObserverInterface {
public:
- virtual ~Observer() {}
+ virtual ~ObserverInterface() {}
// Called when the value on the variable changes.
virtual void ValueChanged(BaseVariable* variable) = 0;
};
- virtual ~BaseVariable() {}
+ virtual ~BaseVariable() {
+ if (!observer_list_.empty()) {
+ LOG(WARNING) << "Variable " << name_ << " deleted with "
+ << observer_list_.size() << " observers.";
+ }
+ }
// Returns the variable name as a string.
const std::string& GetName() const {
@@ -69,14 +78,14 @@
// Adds and removes observers for value changes on the variable. This only
// works for kVariableAsync variables since the other modes don't track value
// changes. Adding the same observer twice has no effect.
- virtual void AddObserver(BaseVariable::Observer* observer) {
+ virtual void AddObserver(BaseVariable::ObserverInterface* observer) {
if (std::find(observer_list_.begin(), observer_list_.end(), observer) ==
observer_list_.end()) {
observer_list_.push_back(observer);
}
}
- virtual void RemoveObserver(BaseVariable::Observer* observer) {
+ virtual void RemoveObserver(BaseVariable::ObserverInterface* observer) {
observer_list_.remove(observer);
}
@@ -93,11 +102,14 @@
// Calls ValueChanged on all the observers.
void NotifyValueChanged() {
- for (auto& observer : observer_list_)
- observer->ValueChanged(this);
+ for (auto& observer : observer_list_) {
+ RunFromMainLoop(base::Bind(&BaseVariable::ObserverInterface::ValueChanged,
+ base::Unretained(observer), this));
+ }
}
private:
+ friend class PmEvaluationContextTest;
friend class PmBaseVariableTest;
FRIEND_TEST(PmBaseVariableTest, RepeatedObserverTest);
FRIEND_TEST(PmBaseVariableTest, NotifyValueChangedTest);
@@ -122,7 +134,7 @@
const base::TimeDelta poll_interval_;
// The list of value changes observers.
- std::list<BaseVariable::Observer*> observer_list_;
+ std::list<BaseVariable::ObserverInterface*> observer_list_;
};
// Interface to a Policy Manager variable of a given type. Implementation
diff --git a/policy_manager/variable_unittest.cc b/policy_manager/variable_unittest.cc
index 2a6824a..81e1e59 100644
--- a/policy_manager/variable_unittest.cc
+++ b/policy_manager/variable_unittest.cc
@@ -2,15 +2,18 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include "update_engine/policy_manager/variable.h"
+
#include <vector>
#include <gtest/gtest.h>
-#include "update_engine/policy_manager/variable.h"
+#include "update_engine/test_utils.h"
using base::TimeDelta;
using std::string;
using std::vector;
+using chromeos_update_engine::RunGMainLoopMaxIterations;
namespace chromeos_policy_manager {
@@ -25,7 +28,7 @@
virtual ~DefaultVariable() {}
protected:
- virtual const T* GetValue(base::TimeDelta /* timeout */,
+ virtual const T* GetValue(TimeDelta /* timeout */,
string* /* errmsg */) {
return new T();
}
@@ -59,7 +62,7 @@
EXPECT_EQ(var.GetPollInterval(), TimeDelta::FromMinutes(3));
}
-class BaseVariableObserver : public BaseVariable::Observer {
+class BaseVariableObserver : public BaseVariable::ObserverInterface {
public:
void ValueChanged(BaseVariable* variable) {
calls_.push_back(variable);
@@ -88,18 +91,21 @@
var.AddObserver(&observer1);
// Simulate a value change on the variable's implementation.
var.NotifyValueChanged();
+ ASSERT_EQ(0, observer1.calls_.size());
+ RunGMainLoopMaxIterations(100);
- ASSERT_EQ(observer1.calls_.size(), 1);
+ ASSERT_EQ(1, observer1.calls_.size());
// Check that the observer is called with the right argument.
- EXPECT_EQ(observer1.calls_[0], &var);
+ EXPECT_EQ(&var, observer1.calls_[0]);
BaseVariableObserver observer2;
var.AddObserver(&observer2);
var.NotifyValueChanged();
+ RunGMainLoopMaxIterations(100);
// Check that all the observers are called.
- EXPECT_EQ(observer1.calls_.size(), 2);
- EXPECT_EQ(observer2.calls_.size(), 1);
+ EXPECT_EQ(2, observer1.calls_.size());
+ EXPECT_EQ(1, observer2.calls_.size());
}
} // namespace chromeos_policy_manager