fixed a critical problem of thread safety in AsynActionNode

BehaviorTree · Jan 16, 2019 · e76ef47 · e76ef47
1 parent 0c68e3f
commit e76ef47
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 18 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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