ClientGoalHandle can throw in the destructor

[haudren-woven]

Bug report

Required Info:

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • Binary
• Version or commit hash:
  • Foxy ( 2.4.1-1focal.20220209.143946 )
• DDS implementation:
  • FastRTPS
• Client library (if applicable):
  • rclcpp

Steps to reproduce issue

1. Create an action server within a node
2. Spin the node using rclcpp::spin(node)
3. When receiving a new goal, store the goal handle inside the node
4. Shutdown the node while the goal is being executed

class MyActionServer : public rclcpp::Node {
public:
// Initialize action server here

void handle_accepted(const std::shared_ptr<MyGoalHandle> goal_handle)
{
    goal_handle_ = goal_handle;
    // Spin up a worker thread here probably
}

private:
    std::shared_ptr<MyGoalHandle> goal_handle_;
    std::shared_ptr<MyAction> server_;
}

int main()
{
    auto node = std::make_shared<MyActionServer>();
    rclcpp::spin(node):
    rclcpp::shutdown();
}

(Note : I can write a complete example code, but it's a lot of boilerplate to set up an action server...)

Expected behavior

The node shutdowns cleanly

Actual behavior

An exception is thrown in the destructor:

• The destructor will first call on_terminal_state
• on_terminal_state calls notify_goal_terminal_state
• Since rclcpp has already been shutdown, notify_goal_terminal_state will throw here

Additional information

Since I think it's undesirable to throw in the destructor, I would recommend to catch exceptions and log since not being able to notify the terminal state is a non-issue when destroying goal handles.

If storing GoalHandle is an undesired behavior, I wonder if there is any way to prevent the user from doing so?

Finally, the bug only appears if we destroy the goal handle before the action server. So this issue can be mitigated by managing manually the destruction ordering.

[llapx]

@haudren-woven

It works well on ros:rolling with latest rclcpp.

[alsora]

I agree that this is a bug, however I don't agree with your recommendation

Since I think it's undesirable to throw in the destructor, I would recommend to catch exceptions and log since not being able to notify the terminal state is a non-issue when destroying goal handles.

Logging an error and continuing with the shut down will leave the action client waiting forever for a goal result. The action server should be notified when "shutdown" is occurring and, before that, it should be allowed to have time to abort the goal.

[haudren-woven]

Logging an error and continuing with the shut down will leave the action client waiting forever for a goal result. The action server should be notified when "shutdown" is occurring and, before that, it should be allowed to have time to abort the goal.

Yes I think that's the best way to do it, and that's pretty much what I ended up doing by manually registering shutdown callbacks to destroy the ActionServer before the shutdown is complete.

However, in the current situation, I don't see how it would be an issue to ignore the exception: we already can't terminate the goal handle correctly, so the client will hang forever anyways. We might as well catch the exception and prevent abnormal termination.