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";