update_engine: Pass Action ownership to ActionProcessor
Currently, an object that uses an ActionProcessor for processing one or more
actions has to own the Actions. This is problematic, because if we want to
create an action on the fly and use an ActionProcessor to perform it, we have to
own the Action until it is finished. Furthermore, if someone forget to own the
action, there will be memory leaks because ActionProcessor does not delete the
Action.
This patch passes the ownership of the Actions to the ActionProcessor through
unique pointers. If an object wants to have access to the Action, it can get it
when ActionComplete() is called.
BUG=chromium:807976
TEST=unittests
TEST=cros flash
TEST=precq
Change-Id: I28f7e9fd3425f17cc51b4db4a4abc130a7d6ef8f
Reviewed-on: https://chromium-review.googlesource.com/1065113
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Xiaochu Liu <xiaochu@chromium.org>
diff --git a/payload_consumer/download_action_unittest.cc b/payload_consumer/download_action_unittest.cc
index a3ad5b9..6d2a9f0 100644
--- a/payload_consumer/download_action_unittest.cc
+++ b/payload_consumer/download_action_unittest.cc
@@ -65,9 +65,8 @@
class DownloadActionTestProcessorDelegate : public ActionProcessorDelegate {
public:
- explicit DownloadActionTestProcessorDelegate(ErrorCode expected_code)
- : processing_done_called_(false),
- expected_code_(expected_code) {}
+ DownloadActionTestProcessorDelegate()
+ : processing_done_called_(false), expected_code_(ErrorCode::kSuccess) {}
~DownloadActionTestProcessorDelegate() override {
EXPECT_TRUE(processing_done_called_);
}
@@ -91,6 +90,7 @@
const string type = action->Type();
if (type == DownloadAction::StaticType()) {
EXPECT_EQ(expected_code_, code);
+ p2p_file_id_ = static_cast<DownloadAction*>(action)->p2p_file_id();
} else {
EXPECT_EQ(ErrorCode::kSuccess, code);
}
@@ -100,6 +100,7 @@
brillo::Blob expected_data_;
bool processing_done_called_;
ErrorCode expected_code_;
+ string p2p_file_id_;
};
class TestDirectFileWriter : public DirectFileWriter {
@@ -155,25 +156,26 @@
install_plan.source_slot, true);
fake_system_state.fake_boot_control()->SetSlotBootable(
install_plan.target_slot, true);
- ObjectFeederAction<InstallPlan> feeder_action;
- feeder_action.set_obj(install_plan);
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+ feeder_action->set_obj(install_plan);
MockPrefs prefs;
MockHttpFetcher* http_fetcher = new MockHttpFetcher(data.data(),
data.size(),
nullptr);
// takes ownership of passed in HttpFetcher
- DownloadAction download_action(&prefs,
- fake_system_state.boot_control(),
- fake_system_state.hardware(),
- &fake_system_state,
- http_fetcher,
- false /* interactive */);
- download_action.SetTestFileWriter(&writer);
- BondActions(&feeder_action, &download_action);
+ auto download_action =
+ std::make_unique<DownloadAction>(&prefs,
+ fake_system_state.boot_control(),
+ fake_system_state.hardware(),
+ &fake_system_state,
+ http_fetcher,
+ false /* interactive */);
+ download_action->SetTestFileWriter(&writer);
+ BondActions(feeder_action.get(), download_action.get());
MockDownloadActionDelegate download_delegate;
if (use_download_delegate) {
InSequence s;
- download_action.set_delegate(&download_delegate);
+ download_action->set_delegate(&download_delegate);
if (data.size() > kMockHttpFetcherChunkSize)
EXPECT_CALL(download_delegate,
BytesReceived(_, kMockHttpFetcherChunkSize, _));
@@ -181,16 +183,15 @@
EXPECT_CALL(download_delegate, DownloadComplete())
.Times(fail_write == 0 ? 1 : 0);
}
- ErrorCode expected_code = ErrorCode::kSuccess;
- if (fail_write > 0)
- expected_code = ErrorCode::kDownloadWriteError;
- DownloadActionTestProcessorDelegate delegate(expected_code);
+ DownloadActionTestProcessorDelegate delegate;
+ delegate.expected_code_ =
+ (fail_write > 0) ? ErrorCode::kDownloadWriteError : ErrorCode::kSuccess;
delegate.expected_data_ = brillo::Blob(data.begin() + 1, data.end());
delegate.path_ = output_temp_file.path();
ActionProcessor processor;
processor.set_delegate(&delegate);
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(&download_action);
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(download_action));
loop.PostTask(FROM_HERE,
base::Bind(&StartProcessorInRunLoop, &processor, http_fetcher));
@@ -272,24 +273,25 @@
{.size = size, .type = InstallPayloadType::kFull});
total_expected_download_size += size;
}
- ObjectFeederAction<InstallPlan> feeder_action;
- feeder_action.set_obj(install_plan);
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+ feeder_action->set_obj(install_plan);
MockPrefs prefs;
MockHttpFetcher* http_fetcher = new MockHttpFetcher(
payload_datas[0].data(), payload_datas[0].size(), nullptr);
// takes ownership of passed in HttpFetcher
- DownloadAction download_action(&prefs,
- fake_system_state.boot_control(),
- fake_system_state.hardware(),
- &fake_system_state,
- http_fetcher,
- false /* interactive */);
- download_action.SetTestFileWriter(&mock_file_writer);
- BondActions(&feeder_action, &download_action);
+ auto download_action =
+ std::make_unique<DownloadAction>(&prefs,
+ fake_system_state.boot_control(),
+ fake_system_state.hardware(),
+ &fake_system_state,
+ http_fetcher,
+ false /* interactive */);
+ download_action->SetTestFileWriter(&mock_file_writer);
+ BondActions(feeder_action.get(), download_action.get());
MockDownloadActionDelegate download_delegate;
{
InSequence s;
- download_action.set_delegate(&download_delegate);
+ download_action->set_delegate(&download_delegate);
// these are hand-computed based on the payloads specified above
EXPECT_CALL(download_delegate,
BytesReceived(kMockHttpFetcherChunkSize,
@@ -321,8 +323,8 @@
total_expected_download_size));
}
ActionProcessor processor;
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(&download_action);
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(download_action));
loop.PostTask(
FROM_HERE,
@@ -361,31 +363,31 @@
EXPECT_EQ(0, writer.Open(temp_file.path().c_str(), O_WRONLY | O_CREAT, 0));
// takes ownership of passed in HttpFetcher
- ObjectFeederAction<InstallPlan> feeder_action;
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
InstallPlan install_plan;
install_plan.payloads.resize(1);
- feeder_action.set_obj(install_plan);
+ feeder_action->set_obj(install_plan);
FakeSystemState fake_system_state_;
MockPrefs prefs;
- DownloadAction download_action(
+ auto download_action = std::make_unique<DownloadAction>(
&prefs,
fake_system_state_.boot_control(),
fake_system_state_.hardware(),
&fake_system_state_,
new MockHttpFetcher(data.data(), data.size(), nullptr),
false /* interactive */);
- download_action.SetTestFileWriter(&writer);
+ download_action->SetTestFileWriter(&writer);
MockDownloadActionDelegate download_delegate;
if (use_download_delegate) {
- download_action.set_delegate(&download_delegate);
+ download_action->set_delegate(&download_delegate);
EXPECT_CALL(download_delegate, BytesReceived(_, _, _)).Times(0);
}
TerminateEarlyTestProcessorDelegate delegate;
ActionProcessor processor;
processor.set_delegate(&delegate);
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(&download_action);
- BondActions(&feeder_action, &download_action);
+ BondActions(feeder_action.get(), download_action.get());
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(download_action));
loop.PostTask(FROM_HERE,
base::Bind(&TerminateEarlyTestStarter, &processor));
@@ -423,22 +425,21 @@
// This is a simple Action class for testing.
class DownloadActionTestAction : public Action<DownloadActionTestAction> {
public:
- DownloadActionTestAction() : did_run_(false) {}
+ DownloadActionTestAction() = default;
typedef InstallPlan InputObjectType;
typedef InstallPlan OutputObjectType;
ActionPipe<InstallPlan>* in_pipe() { return in_pipe_.get(); }
ActionPipe<InstallPlan>* out_pipe() { return out_pipe_.get(); }
ActionProcessor* processor() { return processor_; }
void PerformAction() {
- did_run_ = true;
ASSERT_TRUE(HasInputObject());
EXPECT_TRUE(expected_input_object_ == GetInputObject());
ASSERT_TRUE(processor());
processor()->ActionComplete(this, ErrorCode::kSuccess);
}
- string Type() const { return "DownloadActionTestAction"; }
+ static std::string StaticType() { return "DownloadActionTestAction"; }
+ string Type() const { return StaticType(); }
InstallPlan expected_input_object_;
- bool did_run_;
};
namespace {
@@ -447,9 +448,19 @@
// only by the test PassObjectOutTest.
class PassObjectOutTestProcessorDelegate : public ActionProcessorDelegate {
public:
- void ProcessingDone(const ActionProcessor* processor, ErrorCode code) {
+ void ProcessingDone(const ActionProcessor* processor,
+ ErrorCode code) override {
brillo::MessageLoop::current()->BreakLoop();
}
+ void ActionCompleted(ActionProcessor* processor,
+ AbstractAction* action,
+ ErrorCode code) override {
+ if (action->Type() == DownloadActionTestAction::StaticType()) {
+ did_test_action_run_ = true;
+ }
+ }
+
+ bool did_test_action_run_ = false;
};
} // namespace
@@ -466,29 +477,30 @@
install_plan.payloads.push_back({.size = 1});
EXPECT_TRUE(
HashCalculator::RawHashOfData({'x'}, &install_plan.payloads[0].hash));
- ObjectFeederAction<InstallPlan> feeder_action;
- feeder_action.set_obj(install_plan);
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+ feeder_action->set_obj(install_plan);
MockPrefs prefs;
FakeSystemState fake_system_state_;
- DownloadAction download_action(&prefs,
- fake_system_state_.boot_control(),
- fake_system_state_.hardware(),
- &fake_system_state_,
- new MockHttpFetcher("x", 1, nullptr),
- false /* interactive */);
- download_action.SetTestFileWriter(&writer);
+ auto download_action =
+ std::make_unique<DownloadAction>(&prefs,
+ fake_system_state_.boot_control(),
+ fake_system_state_.hardware(),
+ &fake_system_state_,
+ new MockHttpFetcher("x", 1, nullptr),
+ false /* interactive */);
+ download_action->SetTestFileWriter(&writer);
- DownloadActionTestAction test_action;
- test_action.expected_input_object_ = install_plan;
- BondActions(&feeder_action, &download_action);
- BondActions(&download_action, &test_action);
+ auto test_action = std::make_unique<DownloadActionTestAction>();
+ test_action->expected_input_object_ = install_plan;
+ BondActions(feeder_action.get(), download_action.get());
+ BondActions(download_action.get(), test_action.get());
ActionProcessor processor;
PassObjectOutTestProcessorDelegate delegate;
processor.set_delegate(&delegate);
- processor.EnqueueAction(&feeder_action);
- processor.EnqueueAction(&download_action);
- processor.EnqueueAction(&test_action);
+ processor.EnqueueAction(std::move(feeder_action));
+ processor.EnqueueAction(std::move(download_action));
+ processor.EnqueueAction(std::move(test_action));
loop.PostTask(
FROM_HERE,
@@ -498,7 +510,7 @@
loop.Run();
EXPECT_FALSE(loop.PendingTasks());
- EXPECT_EQ(true, test_action.did_run_);
+ EXPECT_EQ(true, delegate.did_test_action_run_);
}
// Test fixture for P2P tests.
@@ -553,43 +565,44 @@
install_plan.payloads.push_back(
{.size = data_.length(),
.hash = {'1', '2', '3', '4', 'h', 'a', 's', 'h'}});
- ObjectFeederAction<InstallPlan> feeder_action;
- feeder_action.set_obj(install_plan);
+ auto feeder_action = std::make_unique<ObjectFeederAction<InstallPlan>>();
+ feeder_action->set_obj(install_plan);
MockPrefs prefs;
- http_fetcher_ = new MockHttpFetcher(data_.c_str(),
- data_.length(),
- nullptr);
// Note that DownloadAction takes ownership of the passed in HttpFetcher.
- download_action_.reset(new DownloadAction(&prefs,
- fake_system_state_.boot_control(),
- fake_system_state_.hardware(),
- &fake_system_state_,
- http_fetcher_,
- false /* interactive */));
- download_action_->SetTestFileWriter(&writer);
- BondActions(&feeder_action, download_action_.get());
- DownloadActionTestProcessorDelegate delegate(ErrorCode::kSuccess);
- delegate.expected_data_ = brillo::Blob(data_.begin() + start_at_offset_,
- data_.end());
- delegate.path_ = output_temp_file.path();
- processor_.set_delegate(&delegate);
- processor_.EnqueueAction(&feeder_action);
- processor_.EnqueueAction(download_action_.get());
+ auto download_action = std::make_unique<DownloadAction>(
+ &prefs,
+ fake_system_state_.boot_control(),
+ fake_system_state_.hardware(),
+ &fake_system_state_,
+ new MockHttpFetcher(data_.c_str(), data_.length(), nullptr),
+ false /* interactive */);
+ auto http_fetcher = download_action->http_fetcher();
+ download_action->SetTestFileWriter(&writer);
+ BondActions(feeder_action.get(), download_action.get());
+ delegate_.expected_data_ =
+ brillo::Blob(data_.begin() + start_at_offset_, data_.end());
+ delegate_.path_ = output_temp_file.path();
+ processor_.set_delegate(&delegate_);
+ processor_.EnqueueAction(std::move(feeder_action));
+ processor_.EnqueueAction(std::move(download_action));
- loop_.PostTask(FROM_HERE, base::Bind(
- &P2PDownloadActionTest::StartProcessorInRunLoopForP2P,
- base::Unretained(this)));
+ loop_.PostTask(
+ FROM_HERE,
+ base::Bind(
+ [](P2PDownloadActionTest* action_test, HttpFetcher* http_fetcher) {
+ action_test->processor_.StartProcessing();
+ http_fetcher->SetOffset(action_test->start_at_offset_);
+ },
+ base::Unretained(this),
+ base::Unretained(http_fetcher)));
loop_.Run();
}
// Mainloop used to make StartDownload() synchronous.
brillo::FakeMessageLoop loop_{nullptr};
- // The DownloadAction instance under test.
- unique_ptr<DownloadAction> download_action_;
-
- // The HttpFetcher used in the test.
- MockHttpFetcher* http_fetcher_;
+ // Delegate that is passed to the ActionProcessor.
+ DownloadActionTestProcessorDelegate delegate_;
// The P2PManager used in the test.
unique_ptr<P2PManager> p2p_manager_;
@@ -604,12 +617,6 @@
string data_;
private:
- // Callback used in StartDownload() method.
- void StartProcessorInRunLoopForP2P() {
- processor_.StartProcessing();
- download_action_->http_fetcher()->SetOffset(start_at_offset_);
- }
-
// The requested starting offset passed to SetupDownload().
off_t start_at_offset_;
@@ -627,7 +634,7 @@
StartDownload(true); // use_p2p_to_share
// Check the p2p file and its content matches what was sent.
- string file_id = download_action_->p2p_file_id();
+ string file_id = delegate_.p2p_file_id_;
EXPECT_NE("", file_id);
EXPECT_EQ(static_cast<int>(data_.length()),
p2p_manager_->FileGetSize(file_id));
@@ -651,7 +658,7 @@
// DownloadAction should convey that the file is not being shared.
// and that we don't have any p2p files.
- EXPECT_EQ(download_action_->p2p_file_id(), "");
+ EXPECT_EQ(delegate_.p2p_file_id_, "");
EXPECT_EQ(p2p_manager_->CountSharedFiles(), 0);
}
@@ -679,7 +686,7 @@
// DownloadAction should convey the same file_id and the file should
// have the expected size.
- EXPECT_EQ(download_action_->p2p_file_id(), file_id);
+ EXPECT_EQ(delegate_.p2p_file_id_, file_id);
EXPECT_EQ(static_cast<ssize_t>(data_.length()),
p2p_manager_->FileGetSize(file_id));
EXPECT_EQ(static_cast<ssize_t>(data_.length()),