PM: Blocking policy requests cannot be called synchronously.
Policy requests that may return EvalStatus::kAskMeAgainLater are
considered blocking and should only be called asynchronously. It is
therefore an error to call a policy returning this value via
UpdateManager::PolicyRequest(), and so we kill the program with an
assertion (DCHECK) if this happens; for release builds, a warning log is
emitted.
Note: the associated death test builds and runs iff DCHECK is enabled.
BUG=None
TEST=Unit tests.
Change-Id: I75e2f5e4498f85857aff41778df0300d7c8898e7
Reviewed-on: https://chromium-review.googlesource.com/200760
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
diff --git a/update_manager/update_manager-inl.h b/update_manager/update_manager-inl.h
index 1b1dc49..544a371 100644
--- a/update_manager/update_manager-inl.h
+++ b/update_manager/update_manager-inl.h
@@ -28,13 +28,13 @@
result, args...);
if (status == EvalStatus::kFailed) {
- LOG(WARNING) << "PolicyRequest() failed with error: " << error;
+ LOG(WARNING) << "Policy request failed: " << error;
error.clear();
status = (default_policy_.*policy_method)(ec, state_.get(), &error,
result, args...);
if (status == EvalStatus::kFailed) {
- LOG(WARNING) << "Request to DefaultPolicy also failed, passing error.";
+ LOG(WARNING) << "Request to default policy also failed: " << error;
}
}
// TODO(deymo): Log the actual state used from the EvaluationContext.
@@ -86,7 +86,13 @@
// IMPORTANT: To ensure that ActualArgs can be converted to ExpectedArgs, we
// explicitly instantiate EvaluatePolicy with the latter in lieu of the
// former.
- return EvaluatePolicy<R, ExpectedArgs...>(ec, policy_method, result, args...);
+ EvalStatus ret = EvaluatePolicy<R, ExpectedArgs...>(ec, policy_method, result,
+ args...);
+ // 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";
+ return ret;
}
template<typename R, typename... ActualArgs, typename... ExpectedArgs>
diff --git a/update_manager/update_manager.h b/update_manager/update_manager.h
index b3dfc7f..8c96d7b 100644
--- a/update_manager/update_manager.h
+++ b/update_manager/update_manager.h
@@ -34,11 +34,10 @@
// |policy_method|.
//
// When the policy request succeeds, the |result| is set and the method
- // returns EvalStatus::kSucceeded, otherwise, the |result| may not be set.
- // Also, if the policy implementation should block, this method returns
- // immediately with EvalStatus::kAskMeAgainLater. In case of failure
- // EvalStatus::kFailed is returned and the |error| message is set, which must
- // not be NULL.
+ // returns EvalStatus::kSucceeded, otherwise, the |result| may not be set. A
+ // policy called with this method should not block (i.e. return
+ // EvalStatus::kAskMeAgainLater), which is considered a programming error. On
+ // failure, EvalStatus::kFailed is returned.
//
// An example call to this method is:
// um.PolicyRequest(&Policy::SomePolicyMethod, &bool_result, arg1, arg2);
@@ -77,7 +76,7 @@
private:
FRIEND_TEST(UmUpdateManagerTest, PolicyRequestCallsPolicy);
FRIEND_TEST(UmUpdateManagerTest, PolicyRequestCallsDefaultOnError);
- FRIEND_TEST(UmUpdateManagerTest, PolicyRequestDoesntBlock);
+ FRIEND_TEST(UmUpdateManagerTest, PolicyRequestDoesntBlockDeathTest);
FRIEND_TEST(UmUpdateManagerTest, AsyncPolicyRequestDelaysEvaluation);
// EvaluatePolicy() evaluates the passed |policy_method| method on the current
diff --git a/update_manager/update_manager_unittest.cc b/update_manager/update_manager_unittest.cc
index 39427b3..95fd3e0 100644
--- a/update_manager/update_manager_unittest.cc
+++ b/update_manager/update_manager_unittest.cc
@@ -128,14 +128,16 @@
EXPECT_TRUE(result.updates_enabled);
}
-TEST_F(UmUpdateManagerTest, PolicyRequestDoesntBlock) {
+// This test only applies to debug builds where DCHECK is enabled.
+#if DCHECK_IS_ON
+TEST_F(UmUpdateManagerTest, PolicyRequestDoesntBlockDeathTest) {
+ // The update manager should die (DCHECK) if a policy called synchronously
+ // returns a kAskMeAgainLater value.
UpdateCheckParams result;
umut_->set_policy(new LazyPolicy());
-
- EvalStatus status = umut_->PolicyRequest(
- &Policy::UpdateCheckAllowed, &result);
- EXPECT_EQ(EvalStatus::kAskMeAgainLater, status);
+ EXPECT_DEATH(umut_->PolicyRequest(&Policy::UpdateCheckAllowed, &result), "");
}
+#endif // DCHECK_IS_ON
TEST_F(UmUpdateManagerTest, AsyncPolicyRequestDelaysEvaluation) {
// To avoid differences in code execution order between an AsyncPolicyRequest