Call destroy() before removing ServerGoalHandle

[otamachan]

Signed-off-by: Tamaki Nishino otamachan@gmail.com

As reported by @longjie0723 here https://answers.ros.org/question/411394/rclpy-action-server-seems-slowing-down-and-consuming-memory-in-long-term/ ,Python ActionServer could keep Future objects while it's running, which causes memory consumption increase and the performance issue.
It seems that it's because ServerGoalHandler registers Future when it is constructed and unregisters when destroy() is called, but destroy() is not called when the object is removed here .

This PR calls destroy() before ServerGoalHandler is deleted.

I confirmed the the original issue is resolved by this PR.

[fujitatomoya]

@otamachan thanks for the contribution.

[fujitatomoya]

I see this works and addresses memory leak.

how about adding dtor in ServerGoalHandle to call destroy method internally? so that once this object is deleted, it automatically issues dtor to destroy the goal_handle?

[fujitatomoya]

CC: @sloretz

[otamachan]

Thanks for the comment. I thought so too at the first time, but I fount only a few classes have __del__ method, so I assumed that the most of the classes in rclpy are expected to be called destroy. I'm ok to change to call destroy in __del__ but I'd like to ask maintainers' opinion as well.

$ git grep __del__
rclpy/rclpy/executors.py:    def __del__(self):
rclpy/rclpy/signals.py:    def __del__(self):
rclpy/rclpy/task.py:    def __del__(self):

[fujitatomoya]

this is not hard requirement, so that we can do the refactor later, but lets wait and see other opinion.

CC: @sloretz @clalancette @ivanpauno

[sloretz]

so I assumed that the most of the classes in rclpy are expected to be called destroy

Most classes in rclpy support either being cleaned up by the garbage collector, or having their resources cleaned up before then by destroy(). The purpose of destroy() is to allow stopping external communication, like ceasing to subscribe to topics. It cleans up resources, but it's purpose is more like Socket.close() or File.close(). It just so happens it "closes" those resources by freeing the rcl entities being used.

Ideally here del self._goal_handles[goal_uuid] below would be sufficient as Python's garbage collector would clean up the goal handle. Theirs something larger wrong here with the addition and removal of the future from the Waitable being done in ServerGoalHandle, and maybe the ownership of the future itself. The result future isn't even used by the ServerGoalHandle class.

I would recommend fixing by making the result_future for each goal handle be owned by the ActionServer class, and then here the action server can remove the future from itself directly instead of indirectly via ServerGoalHandle destroy.

[otamachan]

I see. Thank you for your suggestion. I'll try that!

[apockill]

I'm running into this memory leak in production- but the concern is less in the memory aspect but the fact that action call rate slows down rather dramatically after a few hundred calls (from 300 calls per second to 50 to 10).

I've put together a small reproducible example that shows that ActionServer._futures is indeed growing at a high rate, but unfortunately for some reason, _execute_expire_goals is never being called by the ActionServer, and this fix is therefor not working.

Could someone help me understand what triggers the calling of _execute_expire_goals, so I can better help debug this (and my) issue? Thanks!

Edit: ignore me! I understand now- there's a timeout wherein execute_expire_goals gets called. It looks like the fix proposed in this PR also solves the problem on my end 🎉

[fujitatomoya]

@otamachan are you working on #1070 (comment)?

[dcconner]

I am also interested in the status of this fix

[apockill]

Hey folks! This issue seems to be pretty key to any action running in ROS- especially if you rely on ROS actions being relatively fast to call, and if you plan on running a robot for longer periods of time. I understand that #1070 (comment) might not be the ideal fix, but wouldn't it be worth merging it in the interim until the proposed refactor can be complete?

[fujitatomoya]

@apockill Just FYI, i created #1113.

[clalancette]

Closing this one in favor of #1113