Allow to Suspend/Resume the ActionProcessor.
This patch implements the core functionality of suspend/resume actions
from the ActionProcessor. No actions support suspend/resume yet.
Bug: 27047026
TEST=Added unittets, tested on edison-eng.
Change-Id: Ib9600098dbccf05fc30f10f0add4a5bc87892b66
diff --git a/common/action.h b/common/action.h
index d8049ac..6c88216 100644
--- a/common/action.h
+++ b/common/action.h
@@ -124,6 +124,15 @@
// Only the ActionProcessor should call this.
virtual void TerminateProcessing() {}
+ // Called on asynchronous actions if the processing is suspended and resumed,
+ // respectively. These methods are called by the ActionProcessor and should
+ // not be explicitly called.
+ // The action may still call ActionCompleted() once the action is completed
+ // while the processing is suspended, for example if suspend/resume is not
+ // implemented for the given action.
+ virtual void SuspendAction() {}
+ virtual void ResumeAction() {}
+
// These methods are useful for debugging. TODO(adlr): consider using
// std::type_info for this?
// Type() returns a string of the Action type. I.e., for DownloadAction,
diff --git a/common/action_processor.cc b/common/action_processor.cc
index 7ccdfbd..2618e4e 100644
--- a/common/action_processor.cc
+++ b/common/action_processor.cc
@@ -21,14 +21,12 @@
#include <base/logging.h>
#include "update_engine/common/action.h"
+#include "update_engine/common/error_code_utils.h"
using std::string;
namespace chromeos_update_engine {
-ActionProcessor::ActionProcessor()
- : current_action_(nullptr), delegate_(nullptr) {}
-
ActionProcessor::~ActionProcessor() {
if (IsRunning())
StopProcessing();
@@ -45,8 +43,7 @@
CHECK(!IsRunning());
if (!actions_.empty()) {
current_action_ = actions_.front();
- LOG(INFO) << "ActionProcessor::StartProcessing: "
- << current_action_->Type();
+ LOG(INFO) << "ActionProcessor: starting " << current_action_->Type();
actions_.pop_front();
current_action_->PerformAction();
}
@@ -54,17 +51,55 @@
void ActionProcessor::StopProcessing() {
CHECK(IsRunning());
- CHECK(current_action_);
- current_action_->TerminateProcessing();
- CHECK(current_action_);
- current_action_->SetProcessor(nullptr);
- LOG(INFO) << "ActionProcessor::StopProcessing: aborted "
- << current_action_->Type();
+ if (current_action_) {
+ current_action_->TerminateProcessing();
+ current_action_->SetProcessor(nullptr);
+ }
+ LOG(INFO) << "ActionProcessor: aborted "
+ << (current_action_ ? current_action_->Type() : "")
+ << (suspended_ ? " while suspended" : "");
current_action_ = nullptr;
+ suspended_ = false;
if (delegate_)
delegate_->ProcessingStopped(this);
}
+void ActionProcessor::SuspendProcessing() {
+ // No current_action_ when not suspended means that the action processor was
+ // never started or already finished.
+ if (suspended_ || !current_action_) {
+ LOG(WARNING) << "Called SuspendProcessing while not processing.";
+ return;
+ }
+ suspended_ = true;
+
+ // If there's a current action we should notify it that it should suspend, but
+ // the action can ignore that and terminate at any point.
+ LOG(INFO) << "ActionProcessor: suspending " << current_action_->Type();
+ current_action_->SuspendAction();
+}
+
+void ActionProcessor::ResumeProcessing() {
+ if (!suspended_) {
+ LOG(WARNING) << "Called ResumeProcessing while not suspended.";
+ return;
+ }
+ suspended_ = false;
+ if (current_action_) {
+ // The current_action_ did not call ActionComplete while suspended, so we
+ // should notify it of the resume operation.
+ LOG(INFO) << "ActionProcessor: resuming " << current_action_->Type();
+ current_action_->ResumeAction();
+ } else {
+ // The last action called ActionComplete while suspended, so there is
+ // already a log message with the type of the finished action. We simply
+ // state that we are resuming processing and the next function will log the
+ // start of the next action or processing completion.
+ LOG(INFO) << "ActionProcessor: resuming processing";
+ StartNextActionOrFinish(suspended_error_code_);
+ }
+}
+
void ActionProcessor::ActionComplete(AbstractAction* actionptr,
ErrorCode code) {
CHECK_EQ(actionptr, current_action_);
@@ -74,17 +109,26 @@
current_action_->ActionCompleted(code);
current_action_->SetProcessor(nullptr);
current_action_ = nullptr;
- if (actions_.empty()) {
- LOG(INFO) << "ActionProcessor::ActionComplete: finished last action of"
- " type " << old_type;
- } else if (code != ErrorCode::kSuccess) {
- LOG(INFO) << "ActionProcessor::ActionComplete: " << old_type
- << " action failed. Aborting processing.";
+ LOG(INFO) << "ActionProcessor: finished "
+ << (actions_.empty() ? "last action " : "") << old_type
+ << (suspended_ ? " while suspended" : "")
+ << " with code " << utils::ErrorCodeToString(code);
+ if (!actions_.empty() && code != ErrorCode::kSuccess) {
+ LOG(INFO) << "ActionProcessor: Aborting processing due to failure.";
actions_.clear();
}
+ if (suspended_) {
+ // If an action finished while suspended we don't start the next action (or
+ // terminate the processing) until the processor is resumed. This condition
+ // will be flagged by a nullptr current_action_ while suspended_ is true.
+ suspended_error_code_ = code;
+ return;
+ }
+ StartNextActionOrFinish(code);
+}
+
+void ActionProcessor::StartNextActionOrFinish(ErrorCode code) {
if (actions_.empty()) {
- LOG(INFO) << "ActionProcessor::ActionComplete: finished last action of"
- " type " << old_type;
if (delegate_) {
delegate_->ProcessingDone(this, code);
}
@@ -92,8 +136,7 @@
}
current_action_ = actions_.front();
actions_.pop_front();
- LOG(INFO) << "ActionProcessor::ActionComplete: finished " << old_type
- << ", starting " << current_action_->Type();
+ LOG(INFO) << "ActionProcessor: starting " << current_action_->Type();
current_action_->PerformAction();
}
diff --git a/common/action_processor.h b/common/action_processor.h
index d61e12d..050eee9 100644
--- a/common/action_processor.h
+++ b/common/action_processor.h
@@ -20,6 +20,7 @@
#include <deque>
#include <base/macros.h>
+#include <brillo/errors/error.h>
#include "update_engine/common/error_code.h"
@@ -40,7 +41,7 @@
class ActionProcessor {
public:
- ActionProcessor();
+ ActionProcessor() = default;
virtual ~ActionProcessor();
@@ -54,8 +55,20 @@
// will be lost and must be re-enqueued if this Processor is to use it.
void StopProcessing();
- // Returns true iff an Action is currently processing.
- bool IsRunning() const { return nullptr != current_action_; }
+ // Suspend the processing. If an Action is running, it will have the
+ // SuspendProcessing() called on it, and it should suspend operations until
+ // ResumeProcessing() is called on this class to continue. While suspended,
+ // no new actions will be started. Calling SuspendProcessing while the
+ // processing is suspended or not running this method performs no action.
+ void SuspendProcessing();
+
+ // Resume the suspended processing. If the ActionProcessor is not suspended
+ // or not running on the first place this method performs no action.
+ void ResumeProcessing();
+
+ // Returns true iff the processing was started but not yet completed nor
+ // stopped.
+ bool IsRunning() const { return current_action_ != nullptr || suspended_; }
// Adds another Action to the end of the queue.
virtual void EnqueueAction(AbstractAction* action);
@@ -75,15 +88,29 @@
void ActionComplete(AbstractAction* actionptr, ErrorCode code);
private:
+ // Continue processing actions (if any) after the last action terminated with
+ // the passed error code. If there are no more actions to process, the
+ // processing will terminate.
+ void StartNextActionOrFinish(ErrorCode code);
+
// Actions that have not yet begun processing, in the order in which
// they'll be processed.
std::deque<AbstractAction*> actions_;
// A pointer to the currently processing Action, if any.
- AbstractAction* current_action_;
+ AbstractAction* current_action_{nullptr};
+
+ // The ErrorCode reported by an action that was suspended but finished while
+ // being suspended. This error code is stored here to be reported back to the
+ // delegate once the processor is resumed.
+ ErrorCode suspended_error_code_{ErrorCode::kSuccess};
+
+ // Whether the action processor is or should be suspended.
+ bool suspended_{false};
// A pointer to the delegate, or null if none.
- ActionProcessorDelegate *delegate_;
+ ActionProcessorDelegate* delegate_{nullptr};
+
DISALLOW_COPY_AND_ASSIGN(ActionProcessor);
};
diff --git a/common/action_processor_unittest.cc b/common/action_processor_unittest.cc
index 8285470..631e42d 100644
--- a/common/action_processor_unittest.cc
+++ b/common/action_processor_unittest.cc
@@ -16,9 +16,12 @@
#include "update_engine/common/action_processor.h"
-#include <gtest/gtest.h>
#include <string>
+
+#include <gtest/gtest.h>
+
#include "update_engine/common/action.h"
+#include "update_engine/common/mock_action.h"
using std::string;
@@ -51,26 +54,6 @@
string Type() const { return "ActionProcessorTestAction"; }
};
-class ActionProcessorTest : public ::testing::Test { };
-
-// This test creates two simple Actions and sends a message via an ActionPipe
-// from one to the other.
-TEST(ActionProcessorTest, SimpleTest) {
- ActionProcessorTestAction action;
- ActionProcessor action_processor;
- EXPECT_FALSE(action_processor.IsRunning());
- action_processor.EnqueueAction(&action);
- EXPECT_FALSE(action_processor.IsRunning());
- EXPECT_FALSE(action.IsRunning());
- action_processor.StartProcessing();
- EXPECT_TRUE(action_processor.IsRunning());
- EXPECT_TRUE(action.IsRunning());
- EXPECT_EQ(action_processor.current_action(), &action);
- action.CompleteAction();
- EXPECT_FALSE(action_processor.IsRunning());
- EXPECT_FALSE(action.IsRunning());
-}
-
namespace {
class MyActionProcessorDelegate : public ActionProcessorDelegate {
public:
@@ -109,53 +92,79 @@
};
} // namespace
-TEST(ActionProcessorTest, DelegateTest) {
- ActionProcessorTestAction action;
- ActionProcessor action_processor;
- MyActionProcessorDelegate delegate(&action_processor);
- action_processor.set_delegate(&delegate);
+class ActionProcessorTest : public ::testing::Test {
+ void SetUp() override {
+ action_processor_.set_delegate(&delegate_);
+ // Silence Type() calls used for logging.
+ EXPECT_CALL(mock_action_, Type()).Times(testing::AnyNumber());
+ }
- action_processor.EnqueueAction(&action);
- action_processor.StartProcessing();
- action.CompleteAction();
- action_processor.set_delegate(nullptr);
- EXPECT_TRUE(delegate.processing_done_called_);
- EXPECT_TRUE(delegate.action_completed_called_);
+ void TearDown() override {
+ action_processor_.set_delegate(nullptr);
+ }
+
+ protected:
+ // The ActionProcessor under test.
+ ActionProcessor action_processor_;
+
+ MyActionProcessorDelegate delegate_{&action_processor_};
+
+ // Common actions used during most tests.
+ testing::StrictMock<MockAction> mock_action_;
+ ActionProcessorTestAction action_;
+};
+
+TEST_F(ActionProcessorTest, SimpleTest) {
+ EXPECT_FALSE(action_processor_.IsRunning());
+ action_processor_.EnqueueAction(&action_);
+ EXPECT_FALSE(action_processor_.IsRunning());
+ EXPECT_FALSE(action_.IsRunning());
+ action_processor_.StartProcessing();
+ EXPECT_TRUE(action_processor_.IsRunning());
+ EXPECT_TRUE(action_.IsRunning());
+ EXPECT_EQ(action_processor_.current_action(), &action_);
+ action_.CompleteAction();
+ EXPECT_FALSE(action_processor_.IsRunning());
+ EXPECT_FALSE(action_.IsRunning());
}
-TEST(ActionProcessorTest, StopProcessingTest) {
- ActionProcessorTestAction action;
- ActionProcessor action_processor;
- MyActionProcessorDelegate delegate(&action_processor);
- action_processor.set_delegate(&delegate);
-
- action_processor.EnqueueAction(&action);
- action_processor.StartProcessing();
- action_processor.StopProcessing();
- action_processor.set_delegate(nullptr);
- EXPECT_TRUE(delegate.processing_stopped_called_);
- EXPECT_FALSE(delegate.action_completed_called_);
- EXPECT_FALSE(action_processor.IsRunning());
- EXPECT_EQ(nullptr, action_processor.current_action());
+TEST_F(ActionProcessorTest, DelegateTest) {
+ action_processor_.EnqueueAction(&action_);
+ action_processor_.StartProcessing();
+ action_.CompleteAction();
+ EXPECT_TRUE(delegate_.processing_done_called_);
+ EXPECT_TRUE(delegate_.action_completed_called_);
}
-TEST(ActionProcessorTest, ChainActionsTest) {
+TEST_F(ActionProcessorTest, StopProcessingTest) {
+ action_processor_.EnqueueAction(&action_);
+ action_processor_.StartProcessing();
+ action_processor_.StopProcessing();
+ EXPECT_TRUE(delegate_.processing_stopped_called_);
+ EXPECT_FALSE(delegate_.action_completed_called_);
+ EXPECT_FALSE(action_processor_.IsRunning());
+ EXPECT_EQ(nullptr, action_processor_.current_action());
+}
+
+TEST_F(ActionProcessorTest, ChainActionsTest) {
+ // This test doesn't use a delegate since it terminates several actions.
+ action_processor_.set_delegate(nullptr);
+
ActionProcessorTestAction action1, action2;
- ActionProcessor action_processor;
- action_processor.EnqueueAction(&action1);
- action_processor.EnqueueAction(&action2);
- action_processor.StartProcessing();
- EXPECT_EQ(&action1, action_processor.current_action());
- EXPECT_TRUE(action_processor.IsRunning());
+ action_processor_.EnqueueAction(&action1);
+ action_processor_.EnqueueAction(&action2);
+ action_processor_.StartProcessing();
+ EXPECT_EQ(&action1, action_processor_.current_action());
+ EXPECT_TRUE(action_processor_.IsRunning());
action1.CompleteAction();
- EXPECT_EQ(&action2, action_processor.current_action());
- EXPECT_TRUE(action_processor.IsRunning());
+ EXPECT_EQ(&action2, action_processor_.current_action());
+ EXPECT_TRUE(action_processor_.IsRunning());
action2.CompleteAction();
- EXPECT_EQ(nullptr, action_processor.current_action());
- EXPECT_FALSE(action_processor.IsRunning());
+ EXPECT_EQ(nullptr, action_processor_.current_action());
+ EXPECT_FALSE(action_processor_.IsRunning());
}
-TEST(ActionProcessorTest, DtorTest) {
+TEST_F(ActionProcessorTest, DtorTest) {
ActionProcessorTestAction action1, action2;
{
ActionProcessor action_processor;
@@ -169,22 +178,87 @@
EXPECT_FALSE(action2.IsRunning());
}
-TEST(ActionProcessorTest, DefaultDelegateTest) {
+TEST_F(ActionProcessorTest, DefaultDelegateTest) {
// Just make sure it doesn't crash
- ActionProcessorTestAction action;
- ActionProcessor action_processor;
- ActionProcessorDelegate delegate;
- action_processor.set_delegate(&delegate);
+ action_processor_.EnqueueAction(&action_);
+ action_processor_.StartProcessing();
+ action_.CompleteAction();
- action_processor.EnqueueAction(&action);
- action_processor.StartProcessing();
- action.CompleteAction();
+ action_processor_.EnqueueAction(&action_);
+ action_processor_.StartProcessing();
+ action_processor_.StopProcessing();
+}
- action_processor.EnqueueAction(&action);
- action_processor.StartProcessing();
- action_processor.StopProcessing();
+// This test suspends and resume the action processor while running one action_.
+TEST_F(ActionProcessorTest, SuspendResumeTest) {
+ action_processor_.EnqueueAction(&mock_action_);
- action_processor.set_delegate(nullptr);
+ testing::InSequence s;
+ EXPECT_CALL(mock_action_, PerformAction());
+ action_processor_.StartProcessing();
+
+ EXPECT_CALL(mock_action_, SuspendAction());
+ action_processor_.SuspendProcessing();
+ // Suspending the processor twice should not suspend the action twice.
+ action_processor_.SuspendProcessing();
+
+ // IsRunning should return whether there's is an action doing some work, even
+ // if it is suspended.
+ EXPECT_TRUE(action_processor_.IsRunning());
+ EXPECT_EQ(&mock_action_, action_processor_.current_action());
+
+ EXPECT_CALL(mock_action_, ResumeAction());
+ action_processor_.ResumeProcessing();
+
+ // Calling ResumeProcessing twice should not affect the action_.
+ action_processor_.ResumeProcessing();
+
+ action_processor_.ActionComplete(&mock_action_, ErrorCode::kSuccess);
+}
+
+// This test suspends an action that presumably doesn't support suspend/resume
+// and it finished before being resumed.
+TEST_F(ActionProcessorTest, ActionCompletedWhileSuspendedTest) {
+ action_processor_.EnqueueAction(&mock_action_);
+
+ testing::InSequence s;
+ EXPECT_CALL(mock_action_, PerformAction());
+ action_processor_.StartProcessing();
+
+ EXPECT_CALL(mock_action_, SuspendAction());
+ action_processor_.SuspendProcessing();
+
+ // Simulate the action completion while suspended. No other call to
+ // |mock_action_| is expected at this point.
+ action_processor_.ActionComplete(&mock_action_, ErrorCode::kSuccess);
+
+ // The processing should not be done since the ActionProcessor is suspended
+ // and the processing is considered to be still running until resumed.
+ EXPECT_FALSE(delegate_.processing_done_called_);
+ EXPECT_TRUE(action_processor_.IsRunning());
+
+ action_processor_.ResumeProcessing();
+ EXPECT_TRUE(delegate_.processing_done_called_);
+ EXPECT_FALSE(delegate_.processing_stopped_called_);
+}
+
+TEST_F(ActionProcessorTest, StoppedWhileSuspendedTest) {
+ action_processor_.EnqueueAction(&mock_action_);
+
+ testing::InSequence s;
+ EXPECT_CALL(mock_action_, PerformAction());
+ action_processor_.StartProcessing();
+ EXPECT_CALL(mock_action_, SuspendAction());
+ action_processor_.SuspendProcessing();
+
+ EXPECT_CALL(mock_action_, TerminateProcessing());
+ action_processor_.StopProcessing();
+ // Stopping the processing should abort the current execution no matter what.
+ EXPECT_TRUE(delegate_.processing_stopped_called_);
+ EXPECT_FALSE(delegate_.processing_done_called_);
+ EXPECT_FALSE(delegate_.action_completed_called_);
+ EXPECT_FALSE(action_processor_.IsRunning());
+ EXPECT_EQ(nullptr, action_processor_.current_action());
}
} // namespace chromeos_update_engine
diff --git a/mock_action.h b/common/mock_action.h
similarity index 74%
rename from mock_action.h
rename to common/mock_action.h
index 0ba796d..06acad1 100644
--- a/mock_action.h
+++ b/common/mock_action.h
@@ -14,8 +14,8 @@
// limitations under the License.
//
-#ifndef UPDATE_ENGINE_MOCK_ACTION_H_
-#define UPDATE_ENGINE_MOCK_ACTION_H_
+#ifndef UPDATE_ENGINE_COMMON_MOCK_ACTION_H_
+#define UPDATE_ENGINE_COMMON_MOCK_ACTION_H_
#include <string>
@@ -27,7 +27,7 @@
class MockAction;
-template<>
+template <>
class ActionTraits<MockAction> {
public:
typedef NoneType OutputObjectType;
@@ -36,10 +36,17 @@
class MockAction : public Action<MockAction> {
public:
+ MockAction() {
+ ON_CALL(*this, Type()).WillByDefault(testing::Return("MockAction"));
+ }
+
MOCK_METHOD0(PerformAction, void());
+ MOCK_METHOD0(TerminateProcessing, void());
+ MOCK_METHOD0(SuspendAction, void());
+ MOCK_METHOD0(ResumeAction, void());
MOCK_CONST_METHOD0(Type, std::string());
};
} // namespace chromeos_update_engine
-#endif // UPDATE_ENGINE_MOCK_ACTION_H_
+#endif // UPDATE_ENGINE_COMMON_MOCK_ACTION_H_
diff --git a/mock_action_processor.h b/common/mock_action_processor.h
similarity index 84%
rename from mock_action_processor.h
rename to common/mock_action_processor.h
index c98ff3c..04275c1 100644
--- a/mock_action_processor.h
+++ b/common/mock_action_processor.h
@@ -14,8 +14,8 @@
// limitations under the License.
//
-#ifndef UPDATE_ENGINE_MOCK_ACTION_PROCESSOR_H_
-#define UPDATE_ENGINE_MOCK_ACTION_PROCESSOR_H_
+#ifndef UPDATE_ENGINE_COMMON_MOCK_ACTION_PROCESSOR_H_
+#define UPDATE_ENGINE_COMMON_MOCK_ACTION_PROCESSOR_H_
#include <gmock/gmock.h>
@@ -31,4 +31,4 @@
} // namespace chromeos_update_engine
-#endif // UPDATE_ENGINE_MOCK_ACTION_PROCESSOR_H_
+#endif // UPDATE_ENGINE_COMMON_MOCK_ACTION_PROCESSOR_H_
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index a0977b1..35bd206 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -38,6 +38,8 @@
#include "libcros/dbus-proxy-mocks.h"
#include "update_engine/common/fake_clock.h"
#include "update_engine/common/fake_prefs.h"
+#include "update_engine/common/mock_action.h"
+#include "update_engine/common/mock_action_processor.h"
#include "update_engine/common/mock_http_fetcher.h"
#include "update_engine/common/mock_prefs.h"
#include "update_engine/common/platform_constants.h"
@@ -45,8 +47,6 @@
#include "update_engine/common/test_utils.h"
#include "update_engine/common/utils.h"
#include "update_engine/fake_system_state.h"
-#include "update_engine/mock_action.h"
-#include "update_engine/mock_action_processor.h"
#include "update_engine/mock_p2p_manager.h"
#include "update_engine/mock_payload_state.h"
#include "update_engine/payload_consumer/filesystem_verifier_action.h"