diff --git a/include/behaviortree_cpp/decorators/timeout_node.h b/include/behaviortree_cpp/decorators/timeout_node.h index 1d8518885..3b0f88d82 100644 --- a/include/behaviortree_cpp/decorators/timeout_node.h +++ b/include/behaviortree_cpp/decorators/timeout_node.h @@ -58,6 +58,10 @@ class TimeoutNode : public DecoratorNode ~TimeoutNode() override { + { + const std::unique_lock lk(timeout_mutex_); + timeout_started_ = false; + } timer_.cancelAll(); } diff --git a/src/decorators/timeout_node.cpp b/src/decorators/timeout_node.cpp index 5fc254954..f96cc115c 100644 --- a/src/decorators/timeout_node.cpp +++ b/src/decorators/timeout_node.cpp @@ -42,6 +42,12 @@ NodeStatus TimeoutNode::tick() return; } const std::unique_lock lk(timeout_mutex_); + // halt() may have run between the timer firing and this lock: the + // child was already halted and may not exist anymore. + if(!timeout_started_) + { + return; + } if(child()->status() == NodeStatus::RUNNING) { child_halted_ = true; @@ -73,8 +79,14 @@ NodeStatus TimeoutNode::tick() void TimeoutNode::halt() { - timeout_started_ = false; - timer_.cancelAll(); + { + // The timer handler halts the child on the worker thread while holding + // this mutex: wait for it, and make a handler that fires later return + // before it touches the child. + const std::unique_lock lk(timeout_mutex_); + timeout_started_ = false; + timer_.cancelAll(); + } DecoratorNode::halt(); } diff --git a/tests/gtest_decorator.cpp b/tests/gtest_decorator.cpp index 2cec4fbc4..8a93026cd 100644 --- a/tests/gtest_decorator.cpp +++ b/tests/gtest_decorator.cpp @@ -15,6 +15,9 @@ #include "behaviortree_cpp/bt_factory.h" +#include +#include + #include using BT::NodeStatus; @@ -473,6 +476,96 @@ TEST(Decorator, DestroyTreeWhileTimerIsPending) } } +// Regression test: the Timeout timer fires on the TimerQueue worker thread +// and halts the child from there. halt() on the tick thread (a parent halting +// the Timeout, or Tree::haltTree) used to run concurrently with that handler, +// so the child was halted from two threads at the same time, and when the +// tree was destroyed right after, the handler kept using a child that no +// longer existed. +TEST(Decorator, TimeoutHaltWhileTimerIsHaltingTheChild) +{ + struct HaltCounters + { + std::atomic active{ 0 }; + std::atomic max_active{ 0 }; + std::atomic total{ 0 }; + }; + + // An action whose onHalted() takes a while, like one that must cancel a + // remote goal before it can return. + class SlowHaltAction : public BT::StatefulActionNode + { + public: + SlowHaltAction(const std::string& name, const BT::NodeConfig& config, + HaltCounters* counters) + : StatefulActionNode(name, config), counters_(counters) + {} + + static BT::PortsList providedPorts() + { + return {}; + } + + NodeStatus onStart() override + { + return NodeStatus::RUNNING; + } + + NodeStatus onRunning() override + { + return NodeStatus::RUNNING; + } + + void onHalted() override + { + const int active = ++counters_->active; + int max_active = counters_->max_active.load(); + while(active > max_active && + !counters_->max_active.compare_exchange_weak(max_active, active)) + { + } + // the first cancellation does the slow work; a cancellation that + // arrives while it is in progress has nothing left to do + if(active == 1) + { + std::this_thread::sleep_for(milliseconds(200)); + } + ++counters_->total; + --counters_->active; + } + + private: + HaltCounters* counters_; + }; + + HaltCounters counters; + BT::BehaviorTreeFactory factory; + factory.registerNodeType("SlowHalt", &counters); + + const std::string xml_text = R"( + + + + + + + )"; + + { + auto tree = factory.createTreeFromText(xml_text); + ASSERT_EQ(tree.tickOnce(), NodeStatus::RUNNING); + // let the timeout expire: its handler is now halting the child on the + // timer thread + std::this_thread::sleep_for(milliseconds(60)); + tree.haltTree(); + // the tree, child included, is destroyed here + } + + // the child must be halted once, never from two threads at the same time + ASSERT_EQ(counters.max_active.load(), 1); + ASSERT_EQ(counters.total.load(), 1); +} + // Test KeepRunningUntilFailure decorator TEST(Decorator, KeepRunningUntilFailure) {