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

issue-919 Fixed "memory leak" in action clients #920

Merged
merged 1 commit into from
Nov 19, 2019

Conversation

astere-cpr
Copy link
Contributor

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.

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>
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!
@jacobperron for a second check

@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label Nov 18, 2019
@ivanpauno
Copy link
Member

Fixes #919

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Change looks okay to me. Thanks for the fix!

@ivanpauno
Copy link
Member

CI:

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

@ivanpauno
Copy link
Member

Ok, going in. Thanks @astere-cpr for the PR!

@ivanpauno ivanpauno merged commit ecc39ca into ros2:master Nov 19, 2019
@ivanpauno ivanpauno added this to Needs Backport in Dashing Patch Release 5 Dec 2, 2019
ivanpauno pushed a commit that referenced this pull request Dec 5, 2019
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>
ivanpauno pushed a commit that referenced this pull request Dec 5, 2019
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>
sloretz pushed a commit that referenced this pull request Dec 5, 2019
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>
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
* Add C++ bag rewriter

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants