Skip to content

Possible infinite loop in retry node #395

@jinwoo-choiter

Description

@jinwoo-choiter

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 :)

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions