Fix Memory Leak in Actions Using EventsExecutor

[mauropasse]

When using ROS2 Actions with the EventsExecutor, completed goals are not automatically cleaned up because execute_check_expired_goals() is not called.

As a result, goal results remain indefinitely stored, leading to increased memory usage with each completed goal.

In contrast, the SingleThreadedExecutor manages goal expiration using the server "goal expire" timeout on wait_for_work(<timeout>).

To address this issue, I'm creating a one-shot timer on the server (only for EventsExecutor) that triggers after a goal expires, to handle cleanup.

While other solutions may exist, this PR is primarily to discuss the issue and explore options.

[mauropasse]

My fix is a workaround for something that might not be needed:

In ROS2 actions, the user first sends a goal request and gets an accepted/rejected response. Then they must send another request to get the result with action_client->async_get_result(handle)

• First client/service pair: goal request and accepted response.
• Second client/service pair: result request and result.

To support this, the server must store the result until expiration timeout, giving time for the client to request the result. The result is not cleaned when the client requests the result, it is cleaned using a timer (default expiration is 10 seconds).

We at iRobot think it may be better for the server to send the result directly to the client as soon as it's ready, avoiding the need for storage and cleanup.

This could work by replacing the second client/service pair with a simple publish-subscribe model, where the server publishes the result as soon as it’s available:

• Action Server Publishing: The server publishes the result immediately when it’s ready.
• Action Client Subscription: The client subscribes to the result topic and, upon receiving the result, fulfills a promise.
• Maintained API Consistency: On the client side, this setup allows the use of result_future.get() and spin_until_future_complete() as usual, so the change in the underlying mechanism doesn’t affect the client’s overall API.

What do you think?

@fujitatomoya: can we keep the timer object and recalculate the timeout based on the current caching results?

Yes, that's a good improvement for my fix implem.

[mjcarroll]

Perhaps something worth bringing up in Client Library working group. CC: @alsora and @jmachowinski

[jmachowinski]

When using ROS2 Actions with the EventsExecutor, completed goals are not automatically cleaned up because execute_check_expired_goals() is not called.

Why does this not happen ?
This would mean, that the events system never emits 'EntityType::Expired'.

[jmachowinski]

To answer my own question, I just had a look at the code, and found the hidden timer in rcl, that we are missing in the events part.
I wonder if we should refactor _recalculate_expire_timer in rcl and expose it and reuse it here. This would increase the chance, that the behaviors of both implementations do not diverge...

[jmachowinski]

There is a slight behavior difference to the rcl implementation here. RCL uses the clock of the node.
Perhaps use create_timer with the node clock ?

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/next-client-library-wg-meeting-friday-8th-november-2024/40457/1

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/next-client-library-wg-meeting-friday-8th-november-2024/40457/2

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/next-client-library-wg-meeting-friday-22nd-november-2024/40703/1