Possible infinite loop in retry node

[aufheben1]

Retry node with while loop implementation can cause infinite loop in specific cases;

• repeat_count is -1 (which means infinite)
• The child of the retry node is condition node (never returns running)
• The child returns FAILURE in first tick.
• The child can return SUCCESS only when the state of the module is changed by information gathered by other module with ros2 spin.

Possible fix can be modifying the retry node logic;

• Rather than trying another loop on child failure, return RUNNING and wait for next tick.

-    while (try_index_ < max_attempts_ || max_attempts_ == -1)
+    NodeStatus child_state = child_node_->executeTick();
+    switch (child_state)
     {
-        NodeStatus child_state = child_node_->executeTick();
-        switch (child_state)
-        {
-            case NodeStatus::SUCCESS:
-            {
-                try_index_ = 0;
-                haltChild();
-                return (NodeStatus::SUCCESS);
-            }
+        case NodeStatus::SUCCESS: {
+            try_index_ = 0;
+            haltChild();
+            return (NodeStatus::SUCCESS);
+        }
 
-            case NodeStatus::FAILURE:
+        case NodeStatus::FAILURE: {
+            try_index_++;
+            haltChild();
+            if (try_index_ >= max_attempts_ && max_attempts_ != -1)
             {
-                try_index_++;
-                haltChild();
+                try_index_ = 0;
+                return (NodeStatus::FAILURE);
             }
-            break;
+        }
 
-            case NodeStatus::RUNNING:
-            {
-                return NodeStatus::RUNNING;
-            }
+        case NodeStatus::RUNNING: {
+            return (NodeStatus::RUNNING);
+        }
 
-            default:
-            {
-                throw LogicError("A child node must never return IDLE");
-            }
+        default: {
+            throw LogicError("A child node must never return IDLE");
         }
     }
-
-    try_index_ = 0;
-    return NodeStatus::FAILURE;
 }

But I'm worried it might affect some nodes that runs asynchronously so I need confirmation before updating the code. Thanks :)

[asasine]

We've encountered this same issue before.

Since we use ROS-enabled BTs, we spin the ROS nodes between each BT tick. Spinning the ROS node is likely necessary to make SomeAsynchronousAction return SUCCESS or RUNNING the next time its ticked, such as through completing an action goal. Since RetryUntilSuccessful loops infinitely if SomeAsynchronousAction returns FAILURE, this tree is insufficient:

<RetryUntilSuccessful>
  <SomeAsynchronousAction />
</RetryUntilSuccessful>

We solved it by double-inverting a KeepRunningUntilFailure:

<Inverter>
  <KeepRunningUntilFailure>
    <Inverter>
      <SomeAsynchronousAction />
    </Inverter>
  </KeepRunningUntilFailure>
</Inverter>

KeepRunningUntilFailure returns RUNNING if its child returns SUCCESS, which is what we expected RetryUntilSuccessful to do when its child returns FAILURE.

[facontidavide]

Let me think about this.... there might be a better way to avoid locking the tree

[facontidavide]

I moved the discussion here https://discourse.behaviortree.dev/t/change-looping-in-controlnodes-and-decorators-to-make-them-interruptable/53

[cmraaron]

Could let the user decide? <Sequence policy="OneAfterTheOtherInATightLoop"> vs <Sequence policy="StepByStepReturningRunningAtEachTickWhileChildrenAreSuccess">

Some room for improvement on those policy names Or perhaps different node names but that could grow rather quickly

[asasine]

@facontidavide have you had a chance to think about this?

Side note: The https://discourse.behaviortree.dev website isn't working for me, seems like it might be down. What do you think of using GitHub Disussions as opposed to a discourse website for the community? It might foster more engagement since I see several discussion- or question-like pop up here in BT.CPP Issues.

[facontidavide]

I know exactly how to solve this from a technical point of view, but this change slightly change the behavior of the node and affect some users. Instead of fixing this node, it is safer to introduce a new one, that will become default in 4,0

[facontidavide]

I know exactly how to solve this from a technical point of view, but this change slightly change the behavior of the node and affect some users. Instead of fixing this node, it is safer to introduce a new one, that will become default in 4,0

On Fri, Aug 19, 2022, 7:34 AM Adam Sasine @.***> wrote:

@facontidavide https://github.com/facontidavide have you had a chance to think about this?

Side note: The https://discourse.behaviortree.dev website isn't working for me, seems like it might be down. What do you think of using GitHub Disussions https://resources.github.com/devops/process/planning/discussions/ as opposed to a discourse website for the community? It might foster more engagement since I see several discussion- or question-like pop up here in BT.CPP Issues.

— Reply to this email directly, view it on GitHub https://github.com/BehaviorTree/BehaviorTree.CPP/issues/395#issuecomment-1220690434, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVRF2HJPZGU2C52E3OK47DVZ6EOTANCNFSM5YS7C6WA . You are receiving this because you were mentioned.Message ID: @.***>

[facontidavide]

Fixed in version 4.0. See comments here: https://www.behaviortree.dev/migration/#preemptable-nodes-and-treetickroot