UM: Add an expiration timeout for async policy requests.
This enforces a timeout on AsyncPolicyRequest() calls. The timeout
applies to the overall period an async policy takes to be evaluated. If
a timeout fires, the update manager defers straight to evaluating the
default policy.
The actual expiration handling is done by the EvaluationContext,
incorporating it as part of its deferred reevaluation logic. This allows
us to not add (and manage) a separate main loop event for the expiration
timeout.
Unit tests were added to ensure that normal/timeout events are scheduled
and removed correctly, and that evaluation context expiration is
enforced.
BUG=chromium:384094
TEST=Unit tests.
Change-Id: Ia40e0ac3d8ab68eed043013cc930d699f3c3db93
Reviewed-on: https://chromium-review.googlesource.com/205895
Tested-by: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
diff --git a/update_manager/evaluation_context-inl.h b/update_manager/evaluation_context-inl.h
index 45fb0b6..3255138 100644
--- a/update_manager/evaluation_context-inl.h
+++ b/update_manager/evaluation_context-inl.h
@@ -25,7 +25,8 @@
// Get the value from the variable if not found on the cache.
std::string errmsg;
- const T* result = var->GetValue(RemainingTime(), &errmsg);
+ const T* result = var->GetValue(RemainingTime(evaluation_monotonic_deadline_),
+ &errmsg);
if (result == nullptr) {
LOG(WARNING) << "Error reading Variable " << var->GetName() << ": \""
<< errmsg << "\"";
diff --git a/update_manager/evaluation_context.cc b/update_manager/evaluation_context.cc
index da69ced..eab0fea 100644
--- a/update_manager/evaluation_context.cc
+++ b/update_manager/evaluation_context.cc
@@ -4,6 +4,7 @@
#include "update_engine/update_manager/evaluation_context.h"
+#include <algorithm>
#include <string>
#include <base/bind.h>
@@ -21,9 +22,11 @@
namespace chromeos_update_manager {
EvaluationContext::EvaluationContext(ClockInterface* clock,
- TimeDelta evaluation_timeout)
+ TimeDelta evaluation_timeout,
+ TimeDelta expiration_timeout)
: clock_(clock),
evaluation_timeout_(evaluation_timeout),
+ expiration_monotonic_deadline_(MonotonicDeadline(expiration_timeout)),
weak_ptr_factory_(this) {
ResetEvaluation();
}
@@ -37,31 +40,42 @@
if (it.first->GetMode() == kVariableModeAsync)
it.first->RemoveObserver(this);
}
- CancelMainLoopEvent(poll_timeout_event_);
- poll_timeout_event_ = kEventIdNull;
+ CancelMainLoopEvent(timeout_event_);
+ timeout_event_ = kEventIdNull;
}
-TimeDelta EvaluationContext::RemainingTime() const {
- return evaluation_monotonic_deadline_ - clock_->GetMonotonicTime();
+TimeDelta EvaluationContext::RemainingTime(Time monotonic_deadline) const {
+ if (monotonic_deadline.is_max())
+ return TimeDelta::Max();
+ TimeDelta remaining = monotonic_deadline - clock_->GetMonotonicTime();
+ return std::max(remaining, TimeDelta());
+}
+
+Time EvaluationContext::MonotonicDeadline(TimeDelta timeout) {
+ return (timeout.is_max() ? Time::Max() :
+ clock_->GetMonotonicTime() + timeout);
}
void EvaluationContext::ValueChanged(BaseVariable* var) {
- DLOG(INFO) << "ValueChanged for variable " << var->GetName();
- OnValueChangedOrPollTimeout();
+ DLOG(INFO) << "ValueChanged() called for variable " << var->GetName();
+ OnValueChangedOrTimeout();
}
-void EvaluationContext::OnPollTimeout() {
- DLOG(INFO) << "OnPollTimeout() called.";
- poll_timeout_event_ = kEventIdNull;
- OnValueChangedOrPollTimeout();
+void EvaluationContext::OnTimeout() {
+ DLOG(INFO) << "OnTimeout() called due to "
+ << (timeout_marks_expiration_ ? "expiration" : "poll interval");
+ timeout_event_ = kEventIdNull;
+ is_expired_ = timeout_marks_expiration_;
+ OnValueChangedOrTimeout();
}
-void EvaluationContext::OnValueChangedOrPollTimeout() {
+void EvaluationContext::OnValueChangedOrTimeout() {
RemoveObserversAndTimeout();
- // Copy the callback handle locally, allowing the callback code to reset it.
- scoped_ptr<Closure> callback(value_changed_callback_.release());
- if (callback.get() != NULL)
+ // Copy the callback handle locally, allowing it to be reassigned.
+ scoped_ptr<Closure> callback(callback_.release());
+
+ if (callback.get())
callback->Run();
}
@@ -80,8 +94,7 @@
// values for the current evaluation. The second is a deadline for the
// evaluation which required a monotonic source of time.
evaluation_start_ = clock_->GetWallclockTime();
- evaluation_monotonic_deadline_ =
- clock_->GetMonotonicTime() + evaluation_timeout_;
+ evaluation_monotonic_deadline_ = MonotonicDeadline(evaluation_timeout_);
reevaluation_time_ = Time::Max();
// Remove the cached values of non-const variables
@@ -95,54 +108,66 @@
}
bool EvaluationContext::RunOnValueChangeOrTimeout(Closure callback) {
- TimeDelta reeval_timeout;
- bool reeval_timeout_set = false;
- bool waiting_for_value_change = false;
-
- // Check if a reevaluation should be triggered due to a IsTimeGreaterThan()
- // call.
- if (reevaluation_time_ != Time::Max()) {
- reeval_timeout = reevaluation_time_ - evaluation_start_;
- reeval_timeout_set = true;
- }
-
- if (value_changed_callback_.get() != NULL) {
+ // Check that the method was not called more than once.
+ if (callback_.get() != nullptr) {
LOG(ERROR) << "RunOnValueChangeOrTimeout called more than once.";
return false;
}
+ // Check that the context did not yet expire.
+ if (is_expired()) {
+ LOG(ERROR) << "RunOnValueChangeOrTimeout called on an expired context.";
+ return false;
+ }
+
+ TimeDelta timeout(TimeDelta::Max());
+ bool waiting_for_value_change = false;
+
+ // Handle reevaluation due to a IsTimeGreaterThan() call.
+ if (!reevaluation_time_.is_max())
+ timeout = reevaluation_time_ - evaluation_start_;
+
+ // Handle reevaluation due to async or poll variables.
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);
+ waiting_for_value_change = true;
break;
case kVariableModePoll:
- if (!reeval_timeout_set || reeval_timeout > it.first->GetPollInterval())
- reeval_timeout = it.first->GetPollInterval();
- reeval_timeout_set = true;
+ timeout = std::min(timeout, it.first->GetPollInterval());
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)
+ if (!waiting_for_value_change && timeout.is_max())
return false;
- if (reeval_timeout_set) {
- DLOG(INFO)
- << "Waiting for poll timeout in "
- << chromeos_update_engine::utils::FormatTimeDelta(reeval_timeout);
- poll_timeout_event_ = RunFromMainLoopAfterTimeout(
- base::Bind(&EvaluationContext::OnPollTimeout,
+
+ // Ensure that we take into account the expiration timeout.
+ TimeDelta expiration = RemainingTime(expiration_monotonic_deadline_);
+ timeout_marks_expiration_ = expiration < timeout;
+ if (timeout_marks_expiration_)
+ timeout = expiration;
+
+ // Store the reevaluation callback.
+ callback_.reset(new Closure(callback));
+
+ // Schedule a timeout event, if one is set.
+ if (!timeout.is_max()) {
+ DLOG(INFO) << "Waiting for timeout in "
+ << chromeos_update_engine::utils::FormatTimeDelta(timeout);
+ timeout_event_ = RunFromMainLoopAfterTimeout(
+ base::Bind(&EvaluationContext::OnTimeout,
weak_ptr_factory_.GetWeakPtr()),
- reeval_timeout);
+ timeout);
}
- value_changed_callback_.reset(new Closure(callback));
return true;
}
diff --git a/update_manager/evaluation_context.h b/update_manager/evaluation_context.h
index 5e36e8a..c277178 100644
--- a/update_manager/evaluation_context.h
+++ b/update_manager/evaluation_context.h
@@ -32,7 +32,7 @@
//
// Example:
//
-// scoped_refptr<EvaluationContext> ec = new EvaluationContext;
+// scoped_refptr<EvaluationContext> ec = new EvaluationContext(...);
//
// ...
// // The following call to ResetEvaluation() is optional. Use it to reset the
@@ -51,8 +51,12 @@
class EvaluationContext : public base::RefCounted<EvaluationContext>,
private BaseVariable::ObserverInterface {
public:
- explicit EvaluationContext(chromeos_update_engine::ClockInterface* clock,
- base::TimeDelta evaluation_timeout);
+ EvaluationContext(chromeos_update_engine::ClockInterface* clock,
+ base::TimeDelta evaluation_timeout,
+ base::TimeDelta expiration_timeout);
+ EvaluationContext(chromeos_update_engine::ClockInterface* clock,
+ base::TimeDelta evaluation_timeout)
+ : EvaluationContext(clock, evaluation_timeout, base::TimeDelta::Max()) {}
~EvaluationContext();
// Returns a pointer to the value returned by the passed variable |var|. The
@@ -64,11 +68,14 @@
template<typename T>
const T* GetValue(Variable<T>* var);
- // Returns whether the passed |timestamp| is greater than the evaluation
- // time. The |timestamp| value should be in the same scale as the values
- // returned by ClockInterface::GetWallclockTime().
+ // Returns whether the passed |timestamp| is less than the evaluation time.
+ // The |timestamp| value should be in the same scale as the values returned by
+ // ClockInterface::GetWallclockTime().
bool IsTimeGreaterThan(base::Time timestamp);
+ // Returns whether the evaluation context has expired.
+ bool is_expired() const { return is_expired_; }
+
// TODO(deymo): Move the following methods to an interface only visible by the
// UpdateManager class and not the policy implementations.
@@ -78,9 +85,9 @@
void ResetEvaluation();
// 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.
+ // variable changes its value, a polling interval passes, or the context
+ // expiration occurs. 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.
@@ -101,15 +108,21 @@
// BaseVariable::ObserverInterface override.
void ValueChanged(BaseVariable* var);
- // Called from the main loop when the scheduled poll timeout has passed.
- void OnPollTimeout();
+ // Called from the main loop when a scheduled timeout has passed.
+ void OnTimeout();
- // Removes the observers from the used Variables and cancels the poll timeout
- // and executes the scheduled callback, if any.
- void OnValueChangedOrPollTimeout();
+ // Removes the observers from the used Variables and cancels the timeout,
+ // then executes the scheduled callback.
+ void OnValueChangedOrTimeout();
- // The remaining time for the current evaluation.
- base::TimeDelta RemainingTime() const;
+ // If |monotonic_deadline| is not Time::Max(), returns the remaining time
+ // until it is reached, or zero if it has passed. Otherwise, returns
+ // TimeDelta::Max().
+ base::TimeDelta RemainingTime(base::Time monotonic_deadline) const;
+
+ // Returns a monotonic clock timestamp at which |timeout| will have elapsed
+ // since the current time.
+ base::Time MonotonicDeadline(base::TimeDelta timeout);
// A map to hold the cached values for every variable.
typedef std::map<BaseVariable*, BoxedValue> ValueCacheMap;
@@ -117,15 +130,24 @@
// 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_;
+ // A callback used for triggering re-evaluation upon a value change or poll
+ // timeout, or notifying about the evaluation context expiration. It is up to
+ // the caller to determine whether or not expiration occured via is_expired().
+ scoped_ptr<base::Closure> callback_;
// The EventId returned by the event loop identifying the timeout callback.
- // Used to cancel the timeout callback.
- EventId poll_timeout_event_ = kEventIdNull;
+ // Used for canceling the timeout callback.
+ EventId timeout_event_ = kEventIdNull;
+
+ // Whether a timeout event firing marks the expiration of the evaluation
+ // context.
+ bool timeout_marks_expiration_;
+
+ // Whether the evaluation context has indeed expired.
+ bool is_expired_ = false;
// Pointer to the mockable clock interface;
- chromeos_update_engine::ClockInterface* clock_;
+ chromeos_update_engine::ClockInterface* const clock_;
// The timestamp when the evaluation of this EvaluationContext started. This
// value is reset every time ResetEvaluation() is called. The time source
@@ -139,15 +161,17 @@
// GetWallclockTime() at some point of the evaluation.
base::Time reevaluation_time_;
- // The timeout of an evaluation, used to compute the RemainingTime() of an
- // evaluation.
+ // The timeout of an evaluation.
const base::TimeDelta evaluation_timeout_;
// The timestamp in the ClockInterface::GetMonotonicTime() scale at which the
- // current evaluation should finish. This is used to compute the
- // RemainingTime().
+ // current evaluation should finish.
base::Time evaluation_monotonic_deadline_;
+ // The monotonic clock deadline at which expiration occurs. This is set once
+ // during construction.
+ const base::Time expiration_monotonic_deadline_;
+
base::WeakPtrFactory<EvaluationContext> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(EvaluationContext);
diff --git a/update_manager/evaluation_context_unittest.cc b/update_manager/evaluation_context_unittest.cc
index c60bcd6..ab07a12 100644
--- a/update_manager/evaluation_context_unittest.cc
+++ b/update_manager/evaluation_context_unittest.cc
@@ -73,7 +73,8 @@
fake_clock_.SetMonotonicTime(Time::FromInternalValue(12345678L));
// Mar 2, 2006 1:23:45 UTC is 1141262625 since the Unix Epoch.
fake_clock_.SetWallclockTime(Time::FromTimeT(1141262625));
- eval_ctx_ = new EvaluationContext(&fake_clock_, default_timeout_);
+ eval_ctx_ = new EvaluationContext(&fake_clock_, default_timeout_,
+ default_timeout_);
}
virtual void TearDown() {
@@ -181,7 +182,7 @@
}
// Test that we don't schedule an event if there's no variable to wait for.
-TEST_F(UmEvaluationContextTest, RunOnValueChangeOrTimeoutWithoutVariablesTest) {
+TEST_F(UmEvaluationContextTest, RunOnValueChangeOrTimeoutWithoutVariables) {
fake_const_var_.reset(new string("Hello world!"));
EXPECT_EQ(*eval_ctx_->GetValue(&fake_const_var_), "Hello world!");
@@ -189,7 +190,7 @@
}
// Test that reevaluation occurs when an async variable it depends on changes.
-TEST_F(UmEvaluationContextTest, RunOnValueChangeOrTimeoutWithVariablesTest) {
+TEST_F(UmEvaluationContextTest, RunOnValueChangeOrTimeoutWithVariables) {
fake_async_var_.reset(new string("Async value"));
eval_ctx_->GetValue(&fake_async_var_);
@@ -208,7 +209,7 @@
}
// Test that we don't re-schedule the events if we are attending one.
-TEST_F(UmEvaluationContextTest, RunOnValueChangeOrTimeoutCalledTwiceTest) {
+TEST_F(UmEvaluationContextTest, RunOnValueChangeOrTimeoutCalledTwice) {
fake_async_var_.reset(new string("Async value"));
eval_ctx_->GetValue(&fake_async_var_);
@@ -222,24 +223,8 @@
EXPECT_TRUE(value);
}
-// Test that we clear the events when destroying the EvaluationContext.
-TEST_F(UmEvaluationContextTest, 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 reevaluation occurs when a polling timeout fires.
-TEST_F(UmEvaluationContextTest, RunOnValueChangeOrTimeoutRunsFromTimeoutTest) {
+TEST_F(UmEvaluationContextTest, RunOnValueChangeOrTimeoutRunsFromTimeout) {
fake_poll_var_.reset(new string("Polled value"));
eval_ctx_->GetValue(&fake_poll_var_);
@@ -252,6 +237,40 @@
EXPECT_TRUE(value);
}
+// Test that callback is called when evaluation context expires, and that it
+// cannot be used again.
+TEST_F(UmEvaluationContextTest, RunOnValueChangeOrTimeoutExpires) {
+ 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 the timeout occurs.
+ RunGMainLoopMaxIterations(10);
+ EXPECT_FALSE(value);
+ RunGMainLoopUntil(10000, Bind(&GetBoolean, &value));
+ EXPECT_TRUE(value);
+
+ // Ensure that we cannot reschedule an evaluation.
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&DoNothing)));
+}
+
+// Test that we clear the events when destroying the EvaluationContext.
+TEST_F(UmEvaluationContextTest, 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_ = nullptr;
+
+ // 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);
+}
+
// Scheduling two reevaluations from the callback should succeed.
TEST_F(UmEvaluationContextTest,
RunOnValueChangeOrTimeoutReevaluatesRepeatedly) {
@@ -294,13 +313,14 @@
// 1 second to give time to the main loop to trigger the timeout Event (of 0
// seconds). Our callback should not be called because the EvaluationContext
// was removed before the timeout event is attended.
- eval_ctx_ = NULL;
+ eval_ctx_ = nullptr;
RunGMainLoopUntil(1000, Bind(&GetBoolean, &value));
EXPECT_FALSE(value);
}
TEST_F(UmEvaluationContextTest, DefaultTimeout) {
- // Test that the RemainingTime() uses the default timeout on setup.
+ // Test that the evaluation timeout calculation uses the default timeout on
+ // setup.
EXPECT_CALL(mock_var_async_, GetValue(default_timeout_, _))
.WillOnce(Return(nullptr));
UMTEST_EXPECT_NULL(eval_ctx_->GetValue(&mock_var_async_));
diff --git a/update_manager/update_manager-inl.h b/update_manager/update_manager-inl.h
index d78b1b5..7d7049e 100644
--- a/update_manager/update_manager-inl.h
+++ b/update_manager/update_manager-inl.h
@@ -21,24 +21,41 @@
std::string*, R*,
Args...) const,
R* result, Args... args) {
- std::string error;
const std::string policy_name = policy_->PolicyRequestName(policy_method);
+ const bool timed_out = ec->is_expired();
- // First try calling the actual policy.
+ // Reset the evaluation context.
+ ec->ResetEvaluation();
+
LOG(INFO) << "Evaluating " << policy_name << " START";
- EvalStatus status = (policy_.get()->*policy_method)(ec, state_.get(), &error,
- result, args...);
+ // First try calling the actual policy, if the request did not time out.
+ EvalStatus status = EvalStatus::kFailed;
+ if (timed_out) {
+ LOG(WARNING) << "Skipping reevaluation because the request timed out.";
+ } else {
+ std::string error;
+ status = (policy_.get()->*policy_method)(ec, state_.get(), &error, result,
+ args...);
+ LOG_IF(WARNING, status == EvalStatus::kFailed)
+ << "Evaluating policy failed: " << error;
+ }
+
+ // If evaluating the main policy failed, defer to the default policy.
if (status == EvalStatus::kFailed) {
- LOG(WARNING) << "Policy request failed: " << error;
- error.clear();
- status = (default_policy_.*policy_method)(ec, state_.get(), &error,
- result, args...);
+ std::string error;
+ status = (default_policy_.*policy_method)(ec, state_.get(), &error, result,
+ args...);
+ LOG_IF(WARNING, status == EvalStatus::kFailed)
+ << "Evaluating default policy failed: " << error;
- if (status == EvalStatus::kFailed) {
- LOG(WARNING) << "Request to default policy also failed: " << error;
+ if (timed_out && status == EvalStatus::kAskMeAgainLater) {
+ LOG(WARNING) << "Default policy would block but request timed out, "
+ << "forcing failure.";
+ status = EvalStatus::kFailed;
}
}
+
LOG(INFO) << "Evaluating " << policy_name << " END";
// TODO(deymo): Log the actual state used from the EvaluationContext.
@@ -53,7 +70,7 @@
std::string*, R*,
Args...) const,
Args... args) {
- ec->ResetEvaluation();
+ // Evaluate the policy.
R result;
EvalStatus status = EvaluatePolicy(ec, policy_method, &result, args...);
@@ -62,20 +79,22 @@
callback.Run(status, result);
return;
}
- // Re-schedule the policy request based on used variables.
- base::Closure closure = base::Bind(
- &UpdateManager::OnPolicyReadyToEvaluate<R, Args...>,
- base::Unretained(this), ec, callback, policy_method, args...);
- 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;
- }
+ // Re-schedule the policy request based on used variables.
+ base::Closure reeval_callback = base::Bind(
+ &UpdateManager::OnPolicyReadyToEvaluate<R, Args...>,
+ base::Unretained(this), ec, callback,
+ policy_method, args...);
+ if (ec->RunOnValueChangeOrTimeout(reeval_callback))
+ return; // Reevaluation scheduled successfully.
+
+ // Scheduling a reevaluation can fail because policy method didn't use any
+ // non-const variable nor there's any time-based event that will change the
+ // status of evaluation. Alternatively, this may indicate an error in the use
+ // of the scheduling interface.
+ LOG(ERROR) << "Failed to schedule a reevaluation of policy "
+ << policy_->PolicyRequestName(policy_method) << "; this is a bug.";
+ callback.Run(status, result);
}
template<typename R, typename... ActualArgs, typename... ExpectedArgs>
@@ -96,26 +115,27 @@
// Sync policy requests must not block, if they do then this is an error.
DCHECK(EvalStatus::kAskMeAgainLater != ret);
LOG_IF(WARNING, EvalStatus::kAskMeAgainLater == ret)
- << "Sync request used with an async policy";
+ << "Sync request used with an async policy; this is a bug";
return ret;
}
template<typename R, typename... ActualArgs, typename... ExpectedArgs>
void UpdateManager::AsyncPolicyRequest(
base::Callback<void(EvalStatus, const R& result)> callback,
+ base::TimeDelta request_timeout,
EvalStatus (Policy::*policy_method)(EvaluationContext*, State*,
std::string*, R*,
ExpectedArgs...) const,
ActualArgs... args) {
scoped_refptr<EvaluationContext> ec =
- new EvaluationContext(clock_, evaluation_timeout_);
+ new EvaluationContext(clock_, evaluation_timeout_, request_timeout);
// IMPORTANT: To ensure that ActualArgs can be converted to ExpectedArgs, we
// explicitly instantiate UpdateManager::OnPolicyReadyToEvaluate with the
// latter in lieu of the former.
- base::Closure closure = base::Bind(
+ base::Closure eval_callback = base::Bind(
&UpdateManager::OnPolicyReadyToEvaluate<R, ExpectedArgs...>,
base::Unretained(this), ec, callback, policy_method, args...);
- RunFromMainLoop(closure);
+ RunFromMainLoop(eval_callback);
}
} // namespace chromeos_update_manager
diff --git a/update_manager/update_manager.h b/update_manager/update_manager.h
index 131dcf0..0b04c07 100644
--- a/update_manager/update_manager.h
+++ b/update_manager/update_manager.h
@@ -53,6 +53,7 @@
// Evaluates the given |policy_method| policy with the provided |args|
// arguments and calls the |callback| callback with the result when done.
+ // Evaluation is not allowed to exceed |request_timeout|.
//
// If the policy implementation should block, returning a
// EvalStatus::kAskMeAgainLater status the Update Manager will re-evaluate the
@@ -62,6 +63,7 @@
template<typename R, typename... ActualArgs, typename... ExpectedArgs>
void AsyncPolicyRequest(
base::Callback<void(EvalStatus, const R& result)> callback,
+ base::TimeDelta request_timeout,
EvalStatus (Policy::*policy_method)(EvaluationContext*, State*,
std::string*, R*,
ExpectedArgs...) const,
@@ -81,6 +83,8 @@
FRIEND_TEST(UmUpdateManagerTest, PolicyRequestCallsDefaultOnError);
FRIEND_TEST(UmUpdateManagerTest, PolicyRequestDoesntBlockDeathTest);
FRIEND_TEST(UmUpdateManagerTest, AsyncPolicyRequestDelaysEvaluation);
+ FRIEND_TEST(UmUpdateManagerTest, AsyncPolicyRequestDoesNotTimeOut);
+ FRIEND_TEST(UmUpdateManagerTest, AsyncPolicyRequestTimesOut);
// EvaluatePolicy() evaluates the passed |policy_method| method on the current
// policy with the given |args| arguments. If the method fails, the default
diff --git a/update_manager/update_manager_unittest.cc b/update_manager/update_manager_unittest.cc
index 4a15e93..7040a34 100644
--- a/update_manager/update_manager_unittest.cc
+++ b/update_manager/update_manager_unittest.cc
@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include <unistd.h>
+
#include <algorithm>
#include <string>
#include <utility>
@@ -71,12 +73,23 @@
// class extends the DefaultPolicy class to allow extensions of the Policy
// class without extending nor changing this test.
class FailingPolicy : public DefaultPolicy {
+ public:
+ explicit FailingPolicy(int* num_called_p) : num_called_p_(num_called_p) {}
+ FailingPolicy() : FailingPolicy(nullptr) {}
virtual EvalStatus UpdateCheckAllowed(EvaluationContext* ec, State* state,
string* error,
UpdateCheckParams* result) const {
+ if (num_called_p_)
+ (*num_called_p_)++;
*error = "FailingPolicy failed.";
return EvalStatus::kFailed;
}
+
+ protected:
+ virtual std::string PolicyName() const override { return "FailingPolicy"; }
+
+ private:
+ int* num_called_p_;
};
// The LazyPolicy always returns EvalStatus::kAskMeAgainLater.
@@ -86,6 +99,40 @@
UpdateCheckParams* result) const {
return EvalStatus::kAskMeAgainLater;
}
+
+ protected:
+ virtual std::string PolicyName() const override { return "LazyPolicy"; }
+};
+
+// A policy that sleeps and returns EvalStatus::kAskMeAgainlater. Will check
+// that time is greater than a given threshold (if non-zero). Increments a
+// counter every time it is being querie, if a pointer to it is provided.
+class DelayPolicy : public DefaultPolicy {
+ public:
+ DelayPolicy(int sleep_secs, base::Time time_threshold, int* num_called_p)
+ : sleep_secs_(sleep_secs), time_threshold_(time_threshold),
+ num_called_p_(num_called_p) {}
+ virtual EvalStatus UpdateCheckAllowed(EvaluationContext* ec, State* state,
+ string* error,
+ UpdateCheckParams* result) const {
+ if (num_called_p_)
+ (*num_called_p_)++;
+ sleep(sleep_secs_);
+ // We check for a time threshold to ensure that the policy has some
+ // non-constant dependency. The threshold is far enough in the future to
+ // ensure that it does not fire immediately.
+ if (time_threshold_ < base::Time::Max())
+ ec->IsTimeGreaterThan(time_threshold_);
+ return EvalStatus::kAskMeAgainLater;
+ }
+
+ protected:
+ virtual std::string PolicyName() const override { return "DelayPolicy"; }
+
+ private:
+ int sleep_secs_;
+ base::Time time_threshold_;
+ int* num_called_p_;
};
// AccumulateCallsCallback() adds to the passed |acc| accumulator vector pairs
@@ -154,11 +201,68 @@
Callback<void(EvalStatus, const UpdateCheckParams& result)> callback =
Bind(AccumulateCallsCallback<UpdateCheckParams>, &calls);
- umut_->AsyncPolicyRequest(callback, &Policy::UpdateCheckAllowed);
+ umut_->AsyncPolicyRequest(callback, base::TimeDelta::FromSeconds(5),
+ &Policy::UpdateCheckAllowed);
// The callback should wait until we run the main loop for it to be executed.
EXPECT_EQ(0, calls.size());
chromeos_update_engine::RunGMainLoopMaxIterations(100);
EXPECT_EQ(1, calls.size());
}
+TEST_F(UmUpdateManagerTest, AsyncPolicyRequestDoesNotTimeOut) {
+ // Set up an async policy call to return immediately, then wait a little and
+ // ensure that the timeout event does not fire.
+ int num_called = 0;
+ umut_->set_policy(new FailingPolicy(&num_called));
+
+ vector<pair<EvalStatus, UpdateCheckParams>> calls;
+ Callback<void(EvalStatus, const UpdateCheckParams&)> callback =
+ Bind(AccumulateCallsCallback<UpdateCheckParams>, &calls);
+
+ umut_->AsyncPolicyRequest(callback, base::TimeDelta::FromSeconds(1),
+ &Policy::UpdateCheckAllowed);
+ // Run the main loop, ensure that policy was attempted once before deferring
+ // to the default.
+ chromeos_update_engine::RunGMainLoopMaxIterations(100);
+ EXPECT_EQ(1, num_called);
+ ASSERT_EQ(1, calls.size());
+ EXPECT_EQ(EvalStatus::kSucceeded, calls[0].first);
+ // Wait for the timeout to expire, run the main loop again, ensure that
+ // nothing happened.
+ sleep(2);
+ chromeos_update_engine::RunGMainLoopMaxIterations(10);
+ EXPECT_EQ(1, num_called);
+ EXPECT_EQ(1, calls.size());
+}
+
+TEST_F(UmUpdateManagerTest, AsyncPolicyRequestTimesOut) {
+ // Set up an async policy call to exceed its overall execution time, and make
+ // sure that it is aborted. Also ensure that the async call is not reattempted
+ // after the timeout fires by waiting pas the time threshold for reevaluation.
+ int num_called = 0;
+ umut_->set_policy(new DelayPolicy(
+ 2, fake_clock_.GetWallclockTime() + base::TimeDelta::FromSeconds(3),
+ &num_called));
+
+ vector<pair<EvalStatus, UpdateCheckParams>> calls;
+ Callback<void(EvalStatus, const UpdateCheckParams&)> callback =
+ Bind(AccumulateCallsCallback<UpdateCheckParams>, &calls);
+
+ umut_->AsyncPolicyRequest(callback, base::TimeDelta::FromSeconds(1),
+ &Policy::UpdateCheckAllowed);
+ // Run the main loop, ensure that policy was attempted once but the callback
+ // was not invoked.
+ chromeos_update_engine::RunGMainLoopMaxIterations(100);
+ EXPECT_EQ(1, num_called);
+ EXPECT_EQ(0, calls.size());
+ // Wait for the time threshold to be satisfied, run the main loop again,
+ // ensure that reevaluation was not attempted but the callback invoked.
+ sleep(2);
+ chromeos_update_engine::RunGMainLoopMaxIterations(10);
+ EXPECT_EQ(1, num_called);
+ ASSERT_EQ(1, calls.size());
+ EXPECT_EQ(EvalStatus::kSucceeded, calls[0].first);
+ EXPECT_EQ(true, calls[0].second.updates_enabled);
+}
+
} // namespace chromeos_update_manager