haltTree() from another thread now stops tickWhileRunning() (fixes #686) (#1201)

tickRoot() kept ticking after a concurrent haltTree(), restarting the tree
that had just been halted. haltTree() resets the root to IDLE, so a RUNNING
tick result with an IDLE root identifies the halt without adding any state
to Tree (whose inline defaulted move constructor makes new members an ABI
break). haltTree() also emits the wake-up signal to interrupt the sleep.

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

Author: facontidavide

src/bt_factory.cpp

@@ -679,6 +679,12 @@ void Tree::haltTree()
   BT::applyRecursiveVisitor(rootNode(), visitor);
 
   rootNode()->resetStatus();
+
+  // interrupt the sleep of tickWhileRunning(), if called from another thread
+  if(wake_up_)
+  {
+    wake_up_->emitSignal();
+  }
 }
 
 TreeNode* Tree::rootNode() const
@@ -767,14 +773,21 @@ NodeStatus Tree::tickRoot(TickOption opt, std::chrono::milliseconds sleep_time)
     throw RuntimeError("Empty Tree");
   }
 
+  // haltTree() resets the root to IDLE. If that happens (from another thread)
+  // while the last tick returned RUNNING, we must not tick again, because that
+  // would restart the tree (issue #686).
+  const auto halted = [&]() {
+    return status == NodeStatus::RUNNING && rootNode()->status() == NodeStatus::IDLE;
+  };
+
   while(status == NodeStatus::IDLE ||
         (opt == TickOption::WHILE_RUNNING && status == NodeStatus::RUNNING))
   {
     status = rootNode()->executeTick();
 
     // Inner loop. The previous tick might have triggered the wake-up
     // in this case, unless TickOption::EXACTLY_ONCE, we tick again
-    while(opt != TickOption::EXACTLY_ONCE && status == NodeStatus::RUNNING &&
+    while(opt != TickOption::EXACTLY_ONCE && !halted() && status == NodeStatus::RUNNING &&
           wake_up_->waitFor(std::chrono::milliseconds(0)))
     {
       status = rootNode()->executeTick();
@@ -788,6 +801,10 @@ NodeStatus Tree::tickRoot(TickOption opt, std::chrono::milliseconds sleep_time)
     {
       sleep(std::chrono::milliseconds(sleep_time));
     }
+    if(halted())
+    {
+      return NodeStatus::IDLE;
+    }
   }
 
   return status;

tests/gtest_wakeup.cpp

@@ -1,5 +1,9 @@
 #include "behaviortree_cpp/bt_factory.h"
 
+#include <atomic>
+#include <future>
+#include <thread>
+
 #include <gtest/gtest.h>
 
 using namespace BT;
@@ -54,3 +58,67 @@ TEST(WakeUp, BasicTest)
   // the wake-up should fire well under that threshold.
   ASSERT_LT(dT, 100);
 }
+
+// Issue #686: haltTree() called from another thread must stop
+// tickWhileRunning(), instead of letting it tick (and restart) the tree again.
+TEST(WakeUp, HaltTreeStopsTickWhileRunning_Issue686)
+{
+  static std::atomic_int starts;
+  static std::atomic_bool give_up;
+  starts = 0;
+  give_up = false;
+
+  class RunForever : public StatefulActionNode
+  {
+  public:
+    RunForever(const std::string& name, const NodeConfig& config)
+      : StatefulActionNode(name, config)
+    {}
+    static PortsList providedPorts()
+    {
+      return {};
+    }
+    NodeStatus onStart() override
+    {
+      starts++;
+      return NodeStatus::RUNNING;
+    }
+    NodeStatus onRunning() override
+    {
+      // escape hatch, so that a failing test does not hang forever
+      return give_up ? NodeStatus::SUCCESS : NodeStatus::RUNNING;
+    }
+    void onHalted() override
+    {}
+  };
+
+  BehaviorTreeFactory factory;
+  factory.registerNodeType<RunForever>("RunForever");
+
+  auto tree = factory.createTreeFromText(R"(
+    <root BTCPP_format="4">
+      <BehaviorTree ID="MainTree">
+        <RunForever/>
+      </BehaviorTree>
+    </root>)");
+
+  auto result = std::async(std::launch::async, [&] {
+    return tree.tickWhileRunning(std::chrono::milliseconds(10000));
+  });
+
+  while(starts == 0)
+  {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  }
+  tree.haltTree();
+
+  // haltTree must also interrupt the 10 seconds sleep
+  const bool returned =
+      result.wait_for(std::chrono::seconds(2)) == std::future_status::ready;
+  give_up = true;
+  tree.emitWakeUpSignal();
+
+  ASSERT_TRUE(returned);
+  ASSERT_EQ(result.get(), NodeStatus::IDLE);
+  ASSERT_EQ(starts, 1);
+}