diff --git a/src/actions/sleep_node.cpp b/src/actions/sleep_node.cpp index 108518010..be52f7070 100644 --- a/src/actions/sleep_node.cpp +++ b/src/actions/sleep_node.cpp @@ -25,12 +25,16 @@ NodeStatus SleepNode::onStart() timer_waiting_ = true; timer_id_ = timer_.add(std::chrono::milliseconds(msec), [this](bool aborted) { - const std::unique_lock lk(delay_mutex_); - if(!aborted) + // An aborted handler runs later on the timer thread, possibly after + // onStart() armed a new timer: it must not clear timer_waiting_, which + // onHalted() already did. + if(aborted) { - emitWakeUpSignal(); + return; } + const std::unique_lock lk(delay_mutex_); timer_waiting_ = false; + emitWakeUpSignal(); }); return NodeStatus::RUNNING; diff --git a/tests/gtest_decorator.cpp b/tests/gtest_decorator.cpp index 8a93026cd..d434c8069 100644 --- a/tests/gtest_decorator.cpp +++ b/tests/gtest_decorator.cpp @@ -566,6 +566,35 @@ TEST(Decorator, TimeoutHaltWhileTimerIsHaltingTheChild) ASSERT_EQ(counters.total.load(), 1); } +// Regression test: halting a Sleep cancels its timer, and the canceled handler +// runs later on the TimerQueue worker thread. When the Sleep was restarted in +// the meantime, that stale handler used to clear the flag of the new timer, so +// the restarted Sleep returned SUCCESS long before its duration. +TEST(Decorator, SleepRestartedAfterHaltWaitsFullDuration) +{ + BT::BehaviorTreeFactory factory; + + const std::string xml_text = R"( + + + + + )"; + + auto tree = factory.createTreeFromText(xml_text); + + for(int i = 0; i < 20; i++) + { + ASSERT_EQ(tree.tickOnce(), NodeStatus::RUNNING); + tree.haltTree(); + // restart before the timer thread runs the canceled handler + ASSERT_EQ(tree.tickOnce(), NodeStatus::RUNNING); + std::this_thread::sleep_for(milliseconds(20)); + ASSERT_EQ(tree.tickOnce(), NodeStatus::RUNNING) << "iteration " << i; + tree.haltTree(); + } +} + // Test KeepRunningUntilFailure decorator TEST(Decorator, KeepRunningUntilFailure) {