commit 802864c7826ea79f8cc29c52801e08bcfc15db76
author Tom Cherry <tomcherry@google.com> Thu Mar 12 14:29:25 2020 -0700
committer Tom Cherry <tomcherry@google.com> Thu Mar 12 17:15:07 2020 -0700
tree 568d89f9584df78d22dc81d24ee46959314d7db5
parent a964a96ae73cd332084d1e80c5ccec6176ee17fa

init: handle property messages asynchronously #2

A previous change moved property_service into its own thread, since
there was otherwise a deadlock whenever a process called by init would
try to set a property.  This new thread, however, would send a message
via a blocking socket to init for each property that it received,
since init may need to take action depending on which property it is.
Unfortunately, this means that the deadlock is still possible, the
only difference is the socket's buffer must be filled before init deadlocks.

This change, therefore, adds the following:
1) A lock for instructing init to reboot
2) A lock for waiting on properties
3) A lock for queueing new properties

A previous version of this change was reverted and added locks around
all service operations and allowed the property thread to spawn
services directly.  This was complex due to the fact that this code
was not designed to be multi-threaded.  It was reverted due to
apparent issues during reboot.  This change keeps a queue of processes
pending control messages, which it will then handle in the future.  It
is less flexible but safer.

Bug: 146877356
Bug: 148236233
Bug: 150863651
Bug: 151251827
Test: multiple reboot tests, safely restarting hwservicemanager
Change-Id: Ice773436e85d3bf636bb0a892f3f6002bdf996b6

init/action_manager.cpp

diff --git a/init/action_manager.cpp b/init/action_manager.cpp
index ebca762..b45f5cd 100644
--- a/init/action_manager.cpp
+++ b/init/action_manager.cpp
@@ -41,10 +41,12 @@
 }
 
 void ActionManager::QueueEventTrigger(const std::string& trigger) {
+    auto lock = std::lock_guard{event_queue_lock_};
     event_queue_.emplace(trigger);
 }
 
 void ActionManager::QueuePropertyChange(const std::string& name, const std::string& value) {
+    auto lock = std::lock_guard{event_queue_lock_};
     event_queue_.emplace(std::make_pair(name, value));
 }
 
@@ -53,6 +55,7 @@
 }
 
 void ActionManager::QueueBuiltinAction(BuiltinFunction func, const std::string& name) {
+    auto lock = std::lock_guard{event_queue_lock_};
     auto action = std::make_unique<Action>(true, nullptr, "<Builtin Action>", 0, name,
                                            std::map<std::string, std::string>{});
     action->AddCommand(std::move(func), {name}, 0);
@@ -62,15 +65,18 @@
 }
 
 void ActionManager::ExecuteOneCommand() {
-    // Loop through the event queue until we have an action to execute
-    while (current_executing_actions_.empty() && !event_queue_.empty()) {
-        for (const auto& action : actions_) {
-            if (std::visit([&action](const auto& event) { return action->CheckEvent(event); },
-                           event_queue_.front())) {
-                current_executing_actions_.emplace(action.get());
+    {
+        auto lock = std::lock_guard{event_queue_lock_};
+        // Loop through the event queue until we have an action to execute
+        while (current_executing_actions_.empty() && !event_queue_.empty()) {
+            for (const auto& action : actions_) {
+                if (std::visit([&action](const auto& event) { return action->CheckEvent(event); },
+                               event_queue_.front())) {
+                    current_executing_actions_.emplace(action.get());
+                }
             }
+            event_queue_.pop();
         }
-        event_queue_.pop();
     }
 
     if (current_executing_actions_.empty()) {
@@ -103,6 +109,7 @@
 }
 
 bool ActionManager::HasMoreCommands() const {
+    auto lock = std::lock_guard{event_queue_lock_};
     return !current_executing_actions_.empty() || !event_queue_.empty();
 }
 
@@ -113,6 +120,7 @@
 }
 
 void ActionManager::ClearQueue() {
+    auto lock = std::lock_guard{event_queue_lock_};
     // We are shutting down so don't claim the oneshot builtin actions back
     current_executing_actions_ = {};
     event_queue_ = {};