[dvadovsz]

Closes #1228.

Adds a Finally control node with two children, main and cleanup. It ticks main, then ticks cleanup.

• After main returns SUCCESS or FAILURE, cleanup runs and the node returns main's status. If cleanup fails, the node returns FAILURE.
• If main returns SKIPPED on its first tick it did nothing, so cleanup does not run and the node returns SKIPPED. If main already returned RUNNING and is then skipped, for example by _while, cleanup runs and the node returns SKIPPED.
• If main throws, the node halts main, runs cleanup, and then rethrows the exception, the same as finally in C++/Java/Python.
• If the node is halted while main or cleanup is RUNNING, it halts main and gives cleanup one tick, like TryCatch with catch_on_halt="true".
• Setting the halt_timeout_msec port above 0 (default 0) lets a multi-step cleanup finish. halt() then keeps ticking cleanup every 10 ms until it finishes, fails or throws, or the timeout runs out and cleanup is halted unfinished. This loop runs only when halt() is called from the thread that last ticked the node. A halt from another thread, such as the Timeout decorator's timer or a Tree::haltTree() from a service callback, gives cleanup the single tick and prints a note to stderr. A Finally nested inside such a halt also gets a single tick.
• An exception thrown by cleanup resets the node and propagates, replacing main's result, as an exception from a finally block does.
• halt() never throws, because ~Tree() calls haltTree(). It prints exceptions from cleanup or from halting a child, and a cleanup FAILURE, to stderr.
• The XML loader rejects a Finally without exactly 2 children, as it does for TryCatch.

This PR includes the commit from #1273, which fixes ControlNode::haltChild leaving a child RUNNING when its halt() threw. Without it, the test binary aborts in std::terminate on a Finally whose main throws from halt(). Once #1273 merges, I will rebase this PR onto master to drop that commit.

Why not an option on TryCatch? The semantics differ in three places. Finally runs cleanup on SUCCESS, returns main's status instead of always FAILURE, and lets main's exception through after cleanup. Packing that into TryCatch would make one node's return value depend on a flag.

Two details worth checking in review:

• executeTick() does not store SKIPPED as the node's status, and it doesn't update the status when tick() throws. In both cases Finally would stay RUNNING, and a later halt would run cleanup a second time. So the node calls resetStatus() before returning SKIPPED or rethrowing.
• This registers a new built-in ID, Finally. An application that already registers its own Finally node will get an "already registered" error, so this targets 4.11.

API/ABI: one new header and one registered node. No signatures change.

Testing: tests/gtest_finally.cpp has 44 cases. They cover sync and async main and cleanup, a halt in each phase, the single-tick default, the opt-in loop finishing and timing out, halts from another thread and from Timeout (including a nested Finally), exceptions from main, from cleanup, and from main's own halt(), non-std exceptions, SKIPPED main before and after RUNNING, a restart after a throw, SubTree and ReactiveSequence parents, and child-count validation. I checked that the key tests fail without their fix. The full suite passes in Release (644 tests), the Finally tests pass 50 times in a row under parallel load, ThreadSanitizer reports no races on them, and ASan/UBSan pass per test. pre-commit passes. clang-tidy-21 isn't installed on my machine, so run_clang_tidy.sh will first run in CI.

[facontidavide]

Thanks for this, it's a carefully done PR. The exception semantics are well thought out, halt() never throwing is the right call, and the tests are thorough. I ran them 30 times, plus 10 more times under 5× parallel load, and they never failed. The full suite passes too.

Before this can go in, there is one design problem to solve, plus a few points to settle.

1. halt() ticks cleanup on whatever thread calls it

TimeoutNode halts its child from its TimerQueue callback (src/decorators/timeout_node.cpp:54), while holding timeout_mutex_. Inside a Timeout, Finally::halt() therefore ticks the cleanup child on the timer thread, and the tick thread is blocked on that mutex until cleanup finishes. I checked it with this test:

TEST(FinallyProbe, HaltedByTimeout_CleanupThread)
{
  BT::BehaviorTreeFactory factory;
  const auto tick_thread = std::this_thread::get_id();
  std::thread::id cleanup_thread;
  int cleanup_ticks = 0;
  factory.registerSimpleCondition("Forever",
                                  [](BT::TreeNode&) { return BT::NodeStatus::RUNNING; });
  factory.registerSimpleCondition("SlowCleanup", [&](BT::TreeNode&) {
    cleanup_thread = std::this_thread::get_id();
    return ++cleanup_ticks < 30 ? BT::NodeStatus::RUNNING : BT::NodeStatus::SUCCESS;
  });
  auto tree = factory.createTreeFromText(R"(<root BTCPP_format="4"><BehaviorTree>
      <Timeout msec="50"><Finally><Forever/><SlowCleanup/></Finally></Timeout>
      </BehaviorTree></root>)");
  tree.tickWhileRunning();
  EXPECT_EQ(cleanup_thread, tick_thread);  // fails: all 30 cleanup ticks ran on the timer thread
}

The doc comment says "Call halt() from the thread that ticks the tree", but a built-in decorator doesn't do that. Neither does a common application pattern: calling Tree::haltTree() from another thread, such as a ROS service callback. Cleanup nodes like action clients, or anything else that isn't thread-safe, would then run on a thread they don't expect.

2. A blocking halt() with a 10 s default

When a ReactiveSequence, ReactiveFallback or Parallel halts a branch that contains a Finally, the whole tree stops for up to halt_timeout_msec. During that time no other node is ticked, including conditions that the reactive parent is supposed to keep re-checking. You describe the trade-off clearly. Still, it changes what halt() means in the library, and the 10 s default makes it the normal case rather than an opt-in.

Some possible directions, and I'm open to others:

• on halt, give cleanup a single tick, as TryCatch catch_on_halt="true" does, and document that a multi-step cleanup will be cut short;
• keep the multi-tick loop but make it opt-in, e.g. halt_timeout_msec defaulting to 0 meaning "one tick";
• keep the loop, but don't run it when halt() is called from a thread other than the one that ticks the tree.

Which one would still cover your collision-checking use case?

3. Smaller points

• SKIPPED main: cleanup runs and the node returns SKIPPED, so the parent is told that nothing happened even though cleanup did. A skipped main changed nothing, so not running cleanup seems more consistent. What do you think?
• New built-in ID: registering Finally breaks applications that already register their own node with that name (#547 suggests some exist). That's acceptable in a minor release, so this would target 4.11 rather than a patch release.
• The halt loop polls every 10 ms and ignores the tree's wake-up signal, so a cleanup that calls emitWakeUpSignal() doesn't finish any sooner. Minor.

4. Please split out the haltChild fix

Resetting the child before rethrowing in ControlNode::haltChild is a real bug fix, independent of Finally: without it, ~Tree() can halt the same child twice and terminate. Could you open it as a separate small PR with its test? I'd merge that right away, while we settle the design of Finally.

[facontidavide]

I would like to merge this before the end of the week and release 4.11.

Do you think you will have some time to look into this?

[dvadovsz]

I would like to merge this before the end of the week and release 4.11.

Do you think you will have some time to look into this?

Yes, I'll try to look into this over the next few days. Thank you for the in depth review.

[dvadovsz]

Thanks for the careful review and the Timeout probe. I've pushed a revision that takes your second and third options together, and split out the haltChild fix as #1273.

1 and 2. halt_timeout_msec now defaults to 0, so on halt cleanup gets a single tick, like TryCatch catch_on_halt="true". A value above 0 turns the multi-tick loop back on, and the loop only runs when halt() is called from the thread that last ticked the node. A halt from the Timeout timer or from a Tree::haltTree() on another thread gets the single tick and a note on stderr when a timeout was set. That single tick still runs on the halting thread, under timeout_mutex_ in the Timeout case, the same as TryCatch. I didn't see a way to give cleanup any tick on a halt without that. A Finally nested inside such a halt, for example as another Finally's cleanup, also gets one tick. Your probe is in the tests, along with the nested case and a tick on one thread followed by a halt on another.

For the collision-checking case, re-enabling collisions is a call to another process that returns RUNNING on its first tick, so a single tick would cut it short. The halts that matter there come from a ReactiveSequence or SubTree parent on the tick thread, so setting halt_timeout_msec on that Finally still covers it. A halt from a Timeout around it would get the single tick, and I think that's the right trade-off for that case.

3. Agreed. A main that is SKIPPED on its first tick now skips cleanup and the node returns SKIPPED. If main already returned RUNNING and is then skipped, for example by _while turning false, it may have started work, so cleanup still runs and the node returns SKIPPED.

4. Done. The fix and its test are in reset a child's status when its halt() throws in ControlNode::haltChild #1273. This PR is rebased on master and still carries that commit until reset a child's status when its halt() throws in ControlNode::haltChild #1273 merges. The same pattern exists in ControlNode::resetChildren() and DecoratorNode::resetChild(). I left those for a follow-up to keep the fix small, but can fold them in if you prefer.

On the new built-in ID, 4.11 works for me. The 10 ms poll is unchanged, since the node has no access to the tree's wake-up signal, and it now only applies when the loop is opted in.

[facontidavide]

Thanks @dvadovsz for the careful revision. I went through it against the code, and every claim in your Oct 29 comment holds. I also ran FinallyTest.* ×30, the timing tests ×20 under full CPU load, TSan ×5 (Finally plus every *Timeout* test) and ASan+UBSan. All clean.

One design change before merging: please drop halt_timeout_msec and the multi-tick loop, and ship Finally with a single cleanup tick on halt, the same as TryCatch catch_on_halt="true". About 90 of the 254 lines (the thread id, the thread-local, the sleep loop, the deadline, two stderr paths) exist only for the loop, and the loop has two properties I don't want in a built-in node:

• it blocks reactive parents for up to halt_timeout_msec;
• it can't be bounded by an enclosing Timeout: it runs on the tick thread while that Timeout holds timeout_mutex_, so <Timeout msec="100"> … <Finally halt_timeout_msec="5000"> waits 5 s.

If users need multi-tick cleanup on halt, we can add it later as a separate feature.

Small things while you're at it:

1. finally_node.cpp:88: if the main child threw, cleanup is RUNNING and a halt arrives, main_exception_ is cleared silently. Please log it (e.g. printException) before dropping it.
2. Declare halt() noexcept, so the "halt never throws" guarantee is checked by the compiler.
3. In the header and docs, say that the single tick on halt suits synchronous cleanup: an async cleanup child gets onStart() and then onHalted() right away.
4. #1273 is being reworked: the status reset moves into TreeNode::haltNode() so that every halt path gets it, not only haltChild(). Once that lands, please drop the control_node.cpp commit from this PR and merge master.

[facontidavide]

Requested changes addressed in 01d987d.

[facontidavide]

@dvadovsz I take back point 1 of my review from yesterday. halt_timeout_msec stays. Your revision implements exactly the option I offered on Oct 24, and asynchronous cleanup on halt (the EnableCollisions case from #1228) is the reason the node exists. Removing the loop would have defeated it. Sorry for moving the goalposts.

To get this into 4.11 I pushed 01d987d on top of a merge of master (maintainer edits), and it is now merged as 4f672ea. That commit contains only the small changes:

• reset a child's status when its halt() throws in ControlNode::haltChild #1273 is merged (the guard now lives in TreeNode::haltNode()), so src/control_node.cpp and tests/gtest_halt_child.cpp are back to master's version, and this PR no longer carries its own copy.
• Docs: the port description and the header now say that with halt_timeout_msec above 10, halt() blocks the halting parent for up to that time; that an enclosing Timeout is delayed by the same amount and nested Finally nodes add up; that halts from other threads get a single tick; and that with the default 0, an asynchronous cleanup child gets onStart() and then onHalted() right away. Also changed "below 10" to "10 or less".
• Concurrent second halt: if another thread calls haltTree() while the multi-tick loop runs, it no longer ticks cleanup at the same time (atomic guard, with a test).
• A pending main-child exception that a halt drops is now logged instead of discarded silently.
• halt() is noexcept.

Locally: full suite, Finally* ×30, TSan, ASan+UBSan, and the timing tests under CPU load are all clean. Thanks for the careful work and the patience on this one.