Additional guard needed on Python single goal action server example?

mrjogo commented 5 months ago

I'm converting a ROS1 SimpleActionServer to ROS2, and I believe as written goal_handle.succeed() could be called on a cancelled goal (if a new goal is accepted just previously):

https://github.com/ros2/examples/blob/1d97c4fc7445554f6f85f63305d424fc017212a0/rclpy/actions/minimal_action_server/examples_rclpy_minimal_action_server/server_single_goal.py#L101-L109

Should this instead be:

with self._goal_lock:
  if not goal_handle.is_active:
    self.get_logger().info('Goal aborted')
    return Fibonacci.Result()

  goal_handle.succeed()

  # Populate result message
  result = Fibonacci.Result()
  result.sequence = feedback_msg.sequence

  self.get_logger().info('Returning result: {0}'.format(result.sequence))

  return result

If so, I can make a PR

fujitatomoya commented 5 months ago

i think so, there is a racy condition, and goal_handle could be invalid.

audrow commented 5 months ago

Sounds good, it'd be great if you make a PR for this!

fujitatomoya commented 2 months ago

@mrjogo really appreciate the PR, i posted a comment on that!

ros2 / examples

Additional guard needed on Python single goal action server example? #371