Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ServerGoalHandle should be destroyed before removing. #1113

Merged

Conversation

fujitatomoya
Copy link
Collaborator

replace #1070.

This PR addresses the following two issues.

  1. memory leak. It should call destroy() before removing ServerGoalHandle. (original issue https://answers.ros.org/question/411394/rclpy-action-server-seems-slowing-down-and-consuming-memory-in-long-term/)
  2. ActionServer should own the result_future, but ServerGoalHandle. (see Call destroy() before removing ServerGoalHandle #1070 (comment))

@otamachan i borrowed your code from #1070, so added you as Co-Author.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Co-authored-by: Tamaki Nishino <otamachan@gmail.com>
@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Apr 14, 2023

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This honestly seems like a good cleanup to me overall. I think it is more logical to track the futures in the ActionServer than in the unrelated ServerGoalHandle.

@clalancette clalancette merged commit 79ec706 into ros2:rolling Apr 14, 2023
3 checks passed
@apockill
Copy link

Woohoo! Is there any chance this might be backported to humble?

@fujitatomoya
Copy link
Collaborator Author

We should probably wait for a while to see if this does not have any regression with rolling, but i think we can do that.

CC: @clalancette @sloretz

@aditya2592
Copy link

aditya2592 commented Apr 26, 2023

Hi! thanks for the fix, Is it possible to create a backport PR for humble so we could try it out?

@fujitatomoya
Copy link
Collaborator Author

@Mergifyio backport humble

@mergify
Copy link

mergify bot commented Apr 26, 2023

backport humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Apr 26, 2023
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Co-authored-by: Tamaki Nishino <otamachan@gmail.com>
(cherry picked from commit 79ec706)
fujitatomoya added a commit that referenced this pull request May 17, 2023
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Co-authored-by: Tamaki Nishino <otamachan@gmail.com>
(cherry picked from commit 79ec706)

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@apockill
Copy link

apockill commented Jun 5, 2023

Hey folks! Any idea when the next rclpy release might be cut?

@fujitatomoya
Copy link
Collaborator Author

I guess we do not have concrete date for that so far, last patch was https://discourse.ros.org/t/preparing-for-humble-sync-and-patch-release-2023-05-02/31105.

CC: @audrow

@gavanderhoorn
Copy link

@fujitatomoya: friendly ping?

@fujitatomoya
Copy link
Collaborator Author

@audrow do you happen to know the next patch schedule for humble?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants