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.

Sign up for GitHub

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

Jump to bottom

[dashing backport] Fixed "memory leak" in action clients (#920) #934

Merged
merged 1 commit into from
Dec 5, 2019
Merged

[dashing backport] Fixed "memory leak" in action clients (#920) #934

merged 1 commit into from
Dec 5, 2019

Conversation

ivanpauno
Copy link
Member

Backport of #920.

@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label Dec 5, 2019
@ivanpauno ivanpauno self-assigned this Dec 5, 2019
@sloretz
Copy link
Contributor

sloretz commented Dec 5, 2019

@ivanpauno mind signing off the cherry-picked commit? I think git commit --amend -s will do it.

@sloretz sloretz self-requested a review December 5, 2019 17:50
@astere-cpr @ivanpauno
issue-919 Fixed "memory leak" in action clients (#920)
8332e15 
Whenever a call is made to `rclcpp_action::Client::wait_for_action_server`
a weak pointer to an Event object gets added to the graph_event_ vector
of the NodeGraph interface. This vector will be cleared on a node graph
change event, but if no such event occurs the weak pointer will be stuck
in the vector.  Furthermore, if client code issues repeated calls to
`wait_for_action_server` the vector will keep growing.

The fix moves the Event object creation right after the early return from
`wait_for_action_server` so that the Event object is not created in the
case that the server is known to be present and therefore there is no
need to wait for a node graph change event to occur.

Signed-off-by: Adrian Stere <astere@clearpath.ai>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/dashing-backport-#920 branch from 29234b3 to 8332e15 Compare December 5, 2019 18:29
@sloretz
Copy link
Contributor

sloretz commented Dec 5, 2019

@ros-pull-request-builder retest this please

1 similar comment
@sloretz
Copy link
Contributor

sloretz commented Dec 5, 2019

@ros-pull-request-builder retest this please

@sloretz
Copy link
Contributor

sloretz commented Dec 5, 2019

@nuclearsandwich are these known flaky tests on the dashing branch? The PR job failures have been inconsistent. The latest job passes though.

  1. http://build.ros2.org/job/Dpr__rclcpp__ubuntu_bionic_amd64/250/
  • rclcpp_components.TestComponentManager.load_components
  1. http://build.ros2.org/job/Dpr__rclcpp__ubuntu_bionic_amd64/251/
  • rclcpp.TestWithDifferentNodeOptions/TestPublisherSubscriptionCount.increasing_and_decreasing_counts/two_subscriptions_intraprocess_comm (two_subscriptions_intraprocess_comm)
  • rclcpp.TestWithDifferentNodeOptions/TestPublisherSubscriptionCount.increasing_and_decreasing_counts/two_subscriptions_one_intraprocess_one_not (two_subscriptions_one_intraprocess_one_not)
  • rclcpp.TestWithDifferentNodeOptions/TestPublisherSubscriptionCount.increasing_and_decreasing_counts/two_subscriptions_in_two_contexts_with_intraprocess_comm (two_subscriptions_in_two_contexts_with_intraprocess_comm)

@sloretz
Copy link
Contributor

sloretz commented Dec 5, 2019 •
edited
Loading

CI (dashing, testing just rclcpp_action)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS aborted, osx executors full
  • Windows Build Status

@dirk-thomas
Copy link
Member

dirk-thomas commented Dec 5, 2019 •
edited
Loading

are these known flaky tests on the dashing branch?

See the history of the dev job: http://build.ros2.org/job/Ddev__rclcpp__ubuntu_bionic_amd64/

It also had the failing tests but a retrigger passed green - so it looks like a flaky test.

@ivanpauno
Copy link
Member Author

ivanpauno commented Dec 5, 2019 •
edited
Loading

  1. http://build.ros2.org/job/Dpr__rclcpp__ubuntu_bionic_amd64/250/

Similar to ros2/build_farmer#184, though it's failing in another part of the test.
Also, this patch is completely unrelated with the failure.

I don't remember those failures, but it seems to be completely unrelated to this patch.

@sloretz
Copy link
Contributor

sloretz commented Dec 5, 2019

CI looks good. Merging so I can make a release.

@sloretz sloretz merged commit 7cb2ece into dashing Dec 5, 2019
@ivanpauno ivanpauno deleted the ivanpauno/dashing-backport-#920 branch February 26, 2020 18:57
@ivanpauno ivanpauno restored the ivanpauno/dashing-backport-#920 branch February 26, 2020 18:57
@ivanpauno ivanpauno deleted the ivanpauno/dashing-backport-#920 branch February 26, 2020 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuclearsandwich nuclearsandwich nuclearsandwich approved these changes

@sloretz sloretz sloretz approved these changes

Assignees

@ivanpauno ivanpauno

Labels
in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ivanpauno @sloretz @dirk-thomas @nuclearsandwich @astere-cpr