Fix logic that moves goal handles when one expires

[sloretz]

This fixes a bug in the action server that can result in a crash when a goal is expired. The logic to move goals backwards when one expires can leave invalid pointers and lose track of valid goal handles.

To reproduce the crash

1. Shorten the expire time to make the crash happen sooner

   diff --git a/rcl_action/src/rcl_action/action_server.c b/rcl_action/src/rcl_action/action_server.c
   index ac8b927..916e2ca 100644
   --- a/rcl_action/src/rcl_action/action_server.c
   +++ b/rcl_action/src/rcl_action/action_server.c
   @@ -286,7 +286,7 @@ rcl_action_server_get_default_options(void)
      default_options.feedback_topic_qos = rmw_qos_profile_default;
      default_options.status_topic_qos = rcl_action_qos_profile_status_default;
      default_options.allocator = rcl_get_default_allocator();
   -  default_options.result_timeout.nanoseconds = RCUTILS_S_TO_NS(15 * 60);  // 15 minutes
   +  default_options.result_timeout.nanoseconds = RCUTILS_S_TO_NS(15);  //yyy * 60);  // 15 minutes
      return default_options;
    }
    
   

2. Build

3. Start an action server

   ros2 run examples_rclcpp_minimal_action_server action_server_not_composable
   

4. Run an action client a bunch of times (hapens within 10 times usually with 15 second expiration timeout)

   for i in {1..100} ; do echo $i ; ros2 run examples_rclcpp_minimal_action_client action_client_not_composable_with_cancel ; done
   

When the goal time expires, sometimes the broken logic will leave an invalid pointer that causes an exception later when something tries to access it.

terminate called after throwing an instance of 'rclcpp::exceptions::RCLError'
  what():  goal handle implementation is invalid, at /home/developer/workspaces/ros2-dev1/src/ros2/rcl/rcl_action/src/rcl_action/goal_handle.c:157

[wjwwood]

lgtm, after staring at it for a long time :)

[hidmic]

LGTM too!

[jacobperron]

Would the logic also be fixed if this deallocate was for index i and moved one line up?
For sure the --i; is needed.

allocator.deallocate(action_server->impl->goal_handles[i], allocator.state);
action_server->impl->goal_handles[i] = action_server->impl->goal_handles[num_goal_handles];
action_server->impl->goal_handles[num_goal_handles] = NULL;
// decrement i to check the same index again now that it has a new goal handle
--i;

Just want to make sure I understand the bug :)

[sloretz]

That would be an improvement, though it would still have a bug. All goal handles need to be moved to shrink the array, but this shrinking logic is doing it one at a time in a block that only executes if the goal handle at i has expired. An active goal at the end of the array would be lost when the array shrinks because this bit wouldn't be reached. If there were two goals that expired at once then goal handles after the second expired one would only be moved back one place when they need to be moved back two places.

This PR adds a for-loop to make sure all goals after the expired one are moved.

[jacobperron]

It makes sense to me, thanks for the explanation!

[sloretz]

CI (testing packages above rcl_action)

• Linux
• Linux-aarch64
• macOS
• Windows