From e76ef470ce9a07a6b36ea8fe685f56ddc8eb6b5a Mon Sep 17 00:00:00 2001
From: Davide Faconti <davide.faconti@eurecat.org>
Date: Tue, 8 Jan 2019 10:58:03 +0100
Subject: [PATCH] fixed a critical problem of thread safety in AsynActionNode

---
 gtest/include/action_test_node.h       |  7 ++++---
 include/behaviortree_cpp/action_node.h |  4 ++--
 src/action_node.cpp                    | 14 ++++++++------
 src/tree_node.cpp                      | 14 +++++++-------
 4 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/gtest/include/action_test_node.h b/gtest/include/action_test_node.h
index 92f8f3453..2f698c1ae 100644
--- a/gtest/include/action_test_node.h
+++ b/gtest/include/action_test_node.h
@@ -57,9 +57,10 @@ class AsyncActionTest : public AsyncActionNode
     }
 
   private:
-    int time_;
-    bool boolean_value_;
-    int tick_count_;
+    // using atomic because these variables might be accessed from different threads
+    std::atomic<int> time_;
+    std::atomic_bool boolean_value_;
+    std::atomic<int> tick_count_;
     std::atomic_bool stop_loop_;
 };
 }
diff --git a/include/behaviortree_cpp/action_node.h b/include/behaviortree_cpp/action_node.h
index cf6150853..d8e4141ca 100644
--- a/include/behaviortree_cpp/action_node.h
+++ b/include/behaviortree_cpp/action_node.h
@@ -131,9 +131,9 @@ class AsyncActionNode : public ActionNodeBase
 
     std::thread thread_;
 
-    std::mutex mutex_;
+    std::mutex start_mutex_;
 
-    std::condition_variable condition_variable_;
+    std::condition_variable start_signal_;
 
     std::exception_ptr exptr_;
 };
diff --git a/src/action_node.cpp b/src/action_node.cpp
index 7146d0499..0349f3871 100644
--- a/src/action_node.cpp
+++ b/src/action_node.cpp
@@ -63,9 +63,11 @@ NodeStatus SimpleActionNode::tick()
 //-------------------------------------------------------
 
 AsyncActionNode::AsyncActionNode(const std::string& name, const NodeConfiguration& config)
-  : ActionNodeBase(name, config), keep_thread_alive_(true), start_action_(false),
-  thread_(&AsyncActionNode::asyncThreadLoop, this)
+  : ActionNodeBase(name, config),
+  keep_thread_alive_(true),
+  start_action_(false)
 {
+    thread_ = std::thread(&AsyncActionNode::asyncThreadLoop, this);
 }
 
 AsyncActionNode::~AsyncActionNode()
@@ -78,19 +80,19 @@ AsyncActionNode::~AsyncActionNode()
 
 void AsyncActionNode::waitStart()
 {
-    std::unique_lock<std::mutex> UniqueLock(mutex_);
+    std::unique_lock<std::mutex> lock(start_mutex_);
     while (!start_action_)
     {
-        condition_variable_.wait(UniqueLock);
+        start_signal_.wait(lock);
     }
     start_action_ = false;
 }
 
 void AsyncActionNode::notifyStart()
 {
-    std::lock_guard<std::mutex> LockGuard(mutex_);
+    std::unique_lock<std::mutex> lock(start_mutex_);
     start_action_ = true;
-    condition_variable_.notify_all();
+    start_signal_.notify_all();
 }
 
 void AsyncActionNode::asyncThreadLoop()
diff --git a/src/tree_node.cpp b/src/tree_node.cpp
index 03c175c08..9a5398bc1 100644
--- a/src/tree_node.cpp
+++ b/src/tree_node.cpp
@@ -55,18 +55,18 @@ void TreeNode::setStatus(NodeStatus new_status)
 
 NodeStatus TreeNode::status() const
 {
-    std::lock_guard<std::mutex> LockGuard(state_mutex_);
+    std::lock_guard<std::mutex> lock(state_mutex_);
     return status_;
 }
 
 NodeStatus TreeNode::waitValidStatus()
 {
-    std::unique_lock<std::mutex> lk(state_mutex_);
+    std::unique_lock<std::mutex> lock(state_mutex_);
 
-    state_condition_variable_.wait(lk, [&]() {
-        return (status_ == NodeStatus::RUNNING || status_ == NodeStatus::SUCCESS ||
-                status_ == NodeStatus::FAILURE);
-    });
+    while( isHalted() )
+    {
+        state_condition_variable_.wait(lock);
+    }
     return status_;
 }
 
@@ -77,7 +77,7 @@ const std::string& TreeNode::name() const
 
 bool TreeNode::isHalted() const
 {
-    return status() == NodeStatus::IDLE;
+    return status_ == NodeStatus::IDLE;
 }
 
 TreeNode::StatusChangeSubscriber