-
Notifications
You must be signed in to change notification settings - Fork 417
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
Conversation
@@ -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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
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()
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
closing this in favor of #468 |
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Connects to #461 (comment)
Running valgrind with the proposed fix cleans up the memory leaked in the referenced
test_node
.