buffet: Delay removal of completed command
User will need check status of the command.
Processed which handle commands execution needs to check
state of a command to avoid execution of commands in terminal
states.
TEST=unittest
BUG=brillo:431
Change-Id: I72dc65f824bee200ee81a1970330b6e8f2d08fc2
Reviewed-on: https://chromium-review.googlesource.com/263498
Tested-by: Vitaly Buka <vitalybuka@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Vitaly Buka <vitalybuka@chromium.org>
diff --git a/buffet/commands/command_instance.cc b/buffet/commands/command_instance.cc
index 926dcf5..c12f1ed 100644
--- a/buffet/commands/command_instance.cc
+++ b/buffet/commands/command_instance.cc
@@ -215,12 +215,8 @@
}
void CommandInstance::RemoveFromQueue() {
- if (queue_) {
- // Store this instance in unique_ptr until the end of this method,
- // otherwise it will be destroyed as soon as CommandQueue::Remove() returns.
- std::unique_ptr<CommandInstance> this_instance = queue_->Remove(GetID());
- // The command instance will survive till the end of this scope.
- }
+ if (queue_)
+ queue_->DelayedRemove(GetID());
}
diff --git a/buffet/commands/command_queue.cc b/buffet/commands/command_queue.cc
index d660a1d..62d8c20 100644
--- a/buffet/commands/command_queue.cc
+++ b/buffet/commands/command_queue.cc
@@ -4,10 +4,16 @@
#include "buffet/commands/command_queue.h"
+#include <base/time/time.h>
+
#include "buffet/commands/command_dispatch_interface.h"
namespace buffet {
+namespace {
+const int kRemoveCommandDelayMin = 5;
+}
+
void CommandQueue::Add(std::unique_ptr<CommandInstance> instance) {
std::string id = instance->GetID();
LOG_IF(FATAL, id.empty()) << "Command has no ID";
@@ -17,20 +23,44 @@
<< "' is already in the queue";
if (dispatch_interface_)
dispatch_interface_->OnCommandAdded(pair.first->second.get());
+ Cleanup();
}
-std::unique_ptr<CommandInstance> CommandQueue::Remove(
- const std::string& id) {
- std::unique_ptr<CommandInstance> instance;
+void CommandQueue::DelayedRemove(const std::string& id) {
auto p = map_.find(id);
- if (p != map_.end()) {
- instance = std::move(p->second);
- instance->SetCommandQueue(nullptr);
- map_.erase(p);
- if (dispatch_interface_)
- dispatch_interface_->OnCommandRemoved(instance.get());
+ if (p == map_.end())
+ return;
+ remove_queue_.push(std::make_pair(
+ base::Time::Now() + base::TimeDelta::FromMinutes(kRemoveCommandDelayMin),
+ id));
+ Cleanup();
+}
+
+bool CommandQueue::Remove(const std::string& id) {
+ auto p = map_.find(id);
+ if (p == map_.end())
+ return false;
+ std::unique_ptr<CommandInstance> instance{std::move(p->second)};
+ instance->SetCommandQueue(nullptr);
+ map_.erase(p);
+ if (dispatch_interface_)
+ dispatch_interface_->OnCommandRemoved(instance.get());
+ return true;
+}
+
+void CommandQueue::Cleanup() {
+ while (!remove_queue_.empty() && remove_queue_.front().first < Now()) {
+ Remove(remove_queue_.front().second);
+ remove_queue_.pop();
}
- return instance;
+}
+
+void CommandQueue::SetNowForTest(base::Time now) {
+ test_now_ = now;
+}
+
+base::Time CommandQueue::Now() const {
+ return test_now_.is_null() ? base::Time::Now() : test_now_;
}
CommandInstance* CommandQueue::Find(const std::string& id) const {
diff --git a/buffet/commands/command_queue.h b/buffet/commands/command_queue.h
index 6d6547d..09339d9 100644
--- a/buffet/commands/command_queue.h
+++ b/buffet/commands/command_queue.h
@@ -7,7 +7,9 @@
#include <map>
#include <memory>
+#include <queue>
#include <string>
+#include <utility>
#include <base/macros.h>
@@ -39,10 +41,9 @@
// One shouldn't attempt to add a command with the same ID.
void Add(std::unique_ptr<CommandInstance> instance);
- // Removes a command identified by |id| from the queue. Returns a unique
- // pointer to the command instance if removed successfully, or an empty
- // unique_ptr if the command with the given ID doesn't exist in the queue.
- std::unique_ptr<CommandInstance> Remove(const std::string& id);
+ // Selects command identified by |id| ready for removal. Command will actually
+ // be removed after some time.
+ void DelayedRemove(const std::string& id);
// Finds a command instance in the queue by the instance |id|. Returns
// nullptr if the command with the given |id| is not found. The returned
@@ -50,8 +51,30 @@
CommandInstance* Find(const std::string& id) const;
private:
+ friend class CommandQueueTest;
+ friend class DBusCommandDispacherTest;
+
+ // Removes a command identified by |id| from the queue.
+ bool Remove(const std::string& id);
+
+ // Removes old commands selected with DelayedRemove.
+ void Cleanup();
+
+ // Overrides CommandQueue::Now() for tests.
+ void SetNowForTest(base::Time now);
+
+ // Returns current time.
+ base::Time Now() const;
+
+ // Overrided value to be returned from Now().
+ base::Time test_now_;
+
// ID-to-CommandInstance map.
std::map<std::string, std::unique_ptr<CommandInstance>> map_;
+
+ // Queue of commands to be removed.
+ std::queue<std::pair<base::Time, std::string>> remove_queue_;
+
// Callback interface for command dispatch, if provided.
CommandDispachInterface* dispatch_interface_ = nullptr;
diff --git a/buffet/commands/command_queue_unittest.cc b/buffet/commands/command_queue_unittest.cc
index 9587797..3e4b6ab 100644
--- a/buffet/commands/command_queue_unittest.cc
+++ b/buffet/commands/command_queue_unittest.cc
@@ -15,36 +15,47 @@
#include "buffet/commands/command_dispatch_interface.h"
#include "buffet/commands/object_schema.h"
-namespace {
+namespace buffet {
class CommandQueueTest : public testing::Test {
public:
- std::unique_ptr<buffet::CommandInstance> CreateDummyCommandInstance(
- const std::string& name, const std::string& id) {
- std::unique_ptr<buffet::CommandInstance> cmd{
- new buffet::CommandInstance{name, &command_definition_, {}}};
+ std::unique_ptr<CommandInstance> CreateDummyCommandInstance(
+ const std::string& name,
+ const std::string& id) {
+ std::unique_ptr<CommandInstance> cmd{
+ new CommandInstance{name, &command_definition_, {}}};
cmd->SetID(id);
return cmd;
}
+ bool Remove(const std::string& id) { return queue_.Remove(id); }
+
+ void Cleanup(const base::TimeDelta& interval) {
+ queue_.SetNowForTest(base::Time::Now() + interval);
+ return queue_.Cleanup();
+ }
+
+ CommandQueue queue_;
+
private:
- buffet::CommandDefinition command_definition_{
- "powerd", buffet::ObjectSchema::Create(), buffet::ObjectSchema::Create()};
+ CommandDefinition command_definition_{"powerd",
+ ObjectSchema::Create(),
+ ObjectSchema::Create()};
};
// Fake implementation of CommandDispachInterface.
-// Just keeps track of commands being added to and removed from the queue.
+// Just keeps track of commands being added to and removed from the queue_.
// Aborts if duplicate commands are added or non-existent commands are removed.
-class FakeDispatchInterface : public buffet::CommandDispachInterface {
+class FakeDispatchInterface : public CommandDispachInterface {
public:
- void OnCommandAdded(buffet::CommandInstance* command_instance) override {
+ void OnCommandAdded(CommandInstance* command_instance) override {
CHECK(ids_.insert(command_instance->GetID()).second)
<< "Command ID already exists: " << command_instance->GetID();
CHECK(commands_.insert(command_instance).second)
<< "Command instance already exists";
}
- void OnCommandRemoved(buffet::CommandInstance* command_instance) override {
+ void OnCommandRemoved(CommandInstance* command_instance) override {
CHECK_EQ(1, ids_.erase(command_instance->GetID()))
<< "Command ID not found: " << command_instance->GetID();
CHECK_EQ(1, commands_.erase(command_instance))
@@ -52,7 +63,7 @@
}
// Get the comma-separated list of command IDs currently accumulated in the
- // command queue.
+ // command queue_.
std::string GetIDs() const {
using chromeos::string_utils::Join;
return Join(",", std::vector<std::string>(ids_.begin(), ids_.end()));
@@ -60,77 +71,88 @@
private:
std::set<std::string> ids_;
- std::set<buffet::CommandInstance*> commands_;
+ std::set<CommandInstance*> commands_;
};
-} // anonymous namespace
-
TEST_F(CommandQueueTest, Empty) {
- buffet::CommandQueue queue;
- EXPECT_TRUE(queue.IsEmpty());
- EXPECT_EQ(0, queue.GetCount());
+ EXPECT_TRUE(queue_.IsEmpty());
+ EXPECT_EQ(0, queue_.GetCount());
}
TEST_F(CommandQueueTest, Add) {
- buffet::CommandQueue queue;
- queue.Add(CreateDummyCommandInstance("base.reboot", "id1"));
- queue.Add(CreateDummyCommandInstance("base.reboot", "id2"));
- queue.Add(CreateDummyCommandInstance("base.reboot", "id3"));
- EXPECT_EQ(3, queue.GetCount());
- EXPECT_FALSE(queue.IsEmpty());
+ queue_.Add(CreateDummyCommandInstance("base.reboot", "id1"));
+ queue_.Add(CreateDummyCommandInstance("base.reboot", "id2"));
+ queue_.Add(CreateDummyCommandInstance("base.reboot", "id3"));
+ EXPECT_EQ(3, queue_.GetCount());
+ EXPECT_FALSE(queue_.IsEmpty());
}
TEST_F(CommandQueueTest, Remove) {
- buffet::CommandQueue queue;
const std::string id1 = "id1";
const std::string id2 = "id2";
- queue.Add(CreateDummyCommandInstance("base.reboot", id1));
- queue.Add(CreateDummyCommandInstance("base.reboot", id2));
- EXPECT_FALSE(queue.IsEmpty());
- EXPECT_EQ(nullptr, queue.Remove("dummy").get());
- EXPECT_EQ(2, queue.GetCount());
- EXPECT_NE(nullptr, queue.Remove(id1).get());
- EXPECT_EQ(1, queue.GetCount());
- EXPECT_EQ(nullptr, queue.Remove(id1).get());
- EXPECT_EQ(1, queue.GetCount());
- EXPECT_NE(nullptr, queue.Remove(id2).get());
- EXPECT_EQ(0, queue.GetCount());
- EXPECT_EQ(nullptr, queue.Remove(id2).get());
- EXPECT_EQ(0, queue.GetCount());
- EXPECT_TRUE(queue.IsEmpty());
+ queue_.Add(CreateDummyCommandInstance("base.reboot", id1));
+ queue_.Add(CreateDummyCommandInstance("base.reboot", id2));
+ EXPECT_FALSE(queue_.IsEmpty());
+ EXPECT_FALSE(Remove("dummy"));
+ EXPECT_EQ(2, queue_.GetCount());
+ EXPECT_TRUE(Remove(id1));
+ EXPECT_EQ(1, queue_.GetCount());
+ EXPECT_FALSE(Remove(id1));
+ EXPECT_EQ(1, queue_.GetCount());
+ EXPECT_TRUE(Remove(id2));
+ EXPECT_EQ(0, queue_.GetCount());
+ EXPECT_FALSE(Remove(id2));
+ EXPECT_EQ(0, queue_.GetCount());
+ EXPECT_TRUE(queue_.IsEmpty());
+}
+
+TEST_F(CommandQueueTest, DelayedRemove) {
+ const std::string id1 = "id1";
+ queue_.Add(CreateDummyCommandInstance("base.reboot", id1));
+ EXPECT_EQ(1, queue_.GetCount());
+
+ queue_.DelayedRemove(id1);
+ EXPECT_EQ(1, queue_.GetCount());
+
+ Cleanup(base::TimeDelta::FromMinutes(1));
+ EXPECT_EQ(1, queue_.GetCount());
+
+ Cleanup(base::TimeDelta::FromMinutes(15));
+ EXPECT_EQ(0, queue_.GetCount());
}
TEST_F(CommandQueueTest, Dispatch) {
FakeDispatchInterface dispatch;
- buffet::CommandQueue queue;
- queue.SetCommandDispachInterface(&dispatch);
+ queue_.SetCommandDispachInterface(&dispatch);
const std::string id1 = "id1";
const std::string id2 = "id2";
- queue.Add(CreateDummyCommandInstance("base.reboot", id1));
- queue.Add(CreateDummyCommandInstance("base.reboot", id2));
+ queue_.Add(CreateDummyCommandInstance("base.reboot", id1));
+ queue_.Add(CreateDummyCommandInstance("base.reboot", id2));
std::set<std::string> ids{id1, id2}; // Make sure they are sorted properly.
std::string expected_set = chromeos::string_utils::Join(
",", std::vector<std::string>(ids.begin(), ids.end()));
EXPECT_EQ(expected_set, dispatch.GetIDs());
- queue.Remove(id1);
+ Remove(id1);
EXPECT_EQ(id2, dispatch.GetIDs());
- queue.Remove(id2);
+ Remove(id2);
EXPECT_EQ("", dispatch.GetIDs());
+ queue_.SetCommandDispachInterface(nullptr);
}
TEST_F(CommandQueueTest, Find) {
- buffet::CommandQueue queue;
const std::string id1 = "id1";
const std::string id2 = "id2";
- queue.Add(CreateDummyCommandInstance("base.reboot", id1));
- queue.Add(CreateDummyCommandInstance("base.shutdown", id2));
- EXPECT_EQ(nullptr, queue.Find("dummy"));
- auto cmd1 = queue.Find(id1);
+ queue_.Add(CreateDummyCommandInstance("base.reboot", id1));
+ queue_.Add(CreateDummyCommandInstance("base.shutdown", id2));
+ EXPECT_EQ(nullptr, queue_.Find("dummy"));
+ auto cmd1 = queue_.Find(id1);
EXPECT_NE(nullptr, cmd1);
EXPECT_EQ("base.reboot", cmd1->GetName());
EXPECT_EQ(id1, cmd1->GetID());
- auto cmd2 = queue.Find(id2);
+ auto cmd2 = queue_.Find(id2);
EXPECT_NE(nullptr, cmd2);
EXPECT_EQ("base.shutdown", cmd2->GetName());
EXPECT_EQ(id2, cmd2->GetID());
}
+
+} // namespace buffet
diff --git a/buffet/commands/dbus_command_dispatcher_unittest.cc b/buffet/commands/dbus_command_dispatcher_unittest.cc
index ec3c658..1e27aac 100644
--- a/buffet/commands/dbus_command_dispatcher_unittest.cc
+++ b/buffet/commands/dbus_command_dispatcher_unittest.cc
@@ -30,7 +30,6 @@
namespace buffet {
namespace {
-
const char kCommandCategory[] = "test_category";
} // anonymous namespace
@@ -38,6 +37,8 @@
class DBusCommandDispacherTest : public testing::Test {
public:
void SetUp() override {
+ command_queue_.SetNowForTest(base::Time::Max());
+
const dbus::ObjectPath kExportedObjectManagerPath("/test/om_path");
std::string cmd_path = dbus_constants::kCommandServicePathPrefix;
cmd_path += "1";
