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

Fix memory leak in node_base #467

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion rclcpp/src/rclcpp/node_interfaces/node_base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,11 @@ NodeBase::NodeBase(
if (ret != RCL_RET_OK) {
// Finalize the interrupt guard condition.
finalize_notify_guard_condition();

// Finalize previously allocated node arguments
if (!rcl_arguments_fini(&options.arguments) == RCL_RET_OK) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh oops. Maybe rcl_node_init()should always finalize the passed in rcl_node_options_t even when an error occurs? At the very least it should document when it takes ownership of the options and when it doesn.t

Copy link
Contributor Author

@Karsten1987 Karsten1987 Apr 30, 2018

Choose a reason for hiding this comment

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

i actually think the rcl_node_fini functions also lacks the call for rcl_arguments_fini.
I wasn't sure whether the node options arguments are a real copy or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem looks larger than this leak. I think adding rcl_arguments_t broke some assumptions about rcl_node_options_t.

https://github.com/ros2/rcl/blob/fdd534e19e3ee5c5551077288749053c32ddc968/rcl/src/rcl/node.c#L226-L227

rcl_arguments_t isn't trivially copyable since it's using PIMPL, so adding it to rcl_node_options_t made that not trivially copyable as well. Maybe rcl_arguments_t should become a separate parameter to rcl_node_init().

Copy link
Member

Choose a reason for hiding this comment

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

Or you can make a rcl_node_options_copy(src, dst) function and then make sure to use it everywhere.

Also, I'd express a slight preference to have who ever called rcl_arguments_init() (or the equivalent) also be the one to call rcl_arguments_fini(). That is to say, having rcl_node_init/fini clean up the arguments is a bit undesirable because it's hard to look at this code and see that arguments are cleaned up properly. The trade-off might be that the node has to copy the given arguments structure or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other side, one could argue that when rcl_node_fini is called the complete struct including its nested types ought to be cleaned up.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's on the "other side", if the clean up is done in this method, then the node would need to make a copy to store in itself, and then the node would clean up that copy in the fini function as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I made pull request ros2/rcl#231 to make rcl_node_init() deep copy the options. If that's accepted then this PR could be changed to always free the arguments.

I think making an rcl_arguments_t argument to rcl_node_init() is less error prone. Anywhere it's not passed is visible as a compiler error, while anywhere a node options structure is shallow copied is not. I didn't change that though since it would have made for a much larger PR.

Copy link
Member

Choose a reason for hiding this comment

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

That's a fair point, I'd also be ok with that, but the idea behind the node options is that it prevents parameter bloat in the rcl_node_init() function.

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, the default node options will never have a valid arguments field, so maybe it doesn't belong in the options struct.

Copy link
Member

Choose a reason for hiding this comment

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

Even if the arguments are a separate argument to the init function, I'd say it should be copied, however.

throw_from_rcl_error(RCL_RET_ERROR, "failed to deallocate node arguments");
}

if (ret == RCL_RET_NODE_INVALID_NAME) {
rcl_reset_error(); // discard rcl_node_init error
int validation_result;
Expand Down