-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix memory leak #1900
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
Fix memory leak #1900
Changes from all commits
b56b78f
cc56e2a
08a598d
b990bc9
bfdf4e9
151bc6f
4e888b3
5d23e33
aeae2cc
8942273
0722a30
45775b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ namespace nav2_bt_navigator | |
| class RosTopicLogger : public BT::StatusChangeLogger | ||
| { | ||
| public: | ||
| RosTopicLogger(const rclcpp::Node::SharedPtr & ros_node, const BT::Tree & tree); | ||
| RosTopicLogger(const rclcpp::Node::WeakPtr & ros_node, const BT::Tree & tree); | ||
|
|
||
| void callback( | ||
| BT::Duration timestamp, | ||
|
|
@@ -38,7 +38,8 @@ class RosTopicLogger : public BT::StatusChangeLogger | |
| void flush() override; | ||
|
|
||
| protected: | ||
| rclcpp::Node::SharedPtr ros_node_; | ||
| rclcpp::Clock::SharedPtr clock_; | ||
| rclcpp::Logger logger_{rclcpp::get_logger("bt_navigator")}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it might be worth noting on these that the names won't actually be used (right? cause these get overwritten in the constructor once we have the SharedPtr?). I could see someone getting confused if they expect the logger to have this name but instead it has the name of whatever the node has been named at runtime.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the idea here was to default the logger object to a reasonable name in case the logger object isn't overridden by a user since a lot of the places that use the logger object are plugin interfaces that could be further implemented by an external user.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would agree with @mikeferguson, if we were going to set them, then I think this is the way to do it, but I think we should ensure that they're always set properly from the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a nice safety mechanism for the specific nodes where that mistake may be possible (costmap_2d comes to mind). I'm not sure that is possible in other places like this instance. We "own" BT navigator's node and logger object, there's no plugin coming along to not properly initialize it |
||
| rclcpp::Publisher<nav2_msgs::msg::BehaviorTreeLog>::SharedPtr log_pub_; | ||
| std::vector<nav2_msgs::msg::BehaviorTreeStatusChange> event_log_; | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,7 +58,7 @@ class SimpleGoalChecker : public nav2_core::GoalChecker | |
| SimpleGoalChecker(); | ||
| // Standard GoalChecker Interface | ||
| void initialize( | ||
| const rclcpp_lifecycle::LifecycleNode::SharedPtr & nh, | ||
| const rclcpp_lifecycle::LifecycleNode::WeakPtr & parent, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For all instances: I don't think its generally best practice to pass smart pointers by reference. Weak ptrs might be different, but I don't see any documentation about that. Why not have the weakptr be by value?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From this comment: #1900 (comment) what you say makes sense to me, but the conventions around smart pointers are the opposite. We should discuss this and then resolve one way or another.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Passing shared_ptrs by reference makes sense (to avoid the overhead of incrementing and then decrementing the reference count). For a weak_ptr though, it doesn't have the same overhead - although the multiple copy/initialization argument does seem to make since for this use case. |
||
| const std::string & plugin_name) override; | ||
| void reset() override; | ||
| bool isGoalReached( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.