Fix memory leak in node_base

[Karsten1987]

Connects to #461 (comment)

Running valgrind with the proposed fix cleans up the memory leaked in the referenced test_node.

[sloretz]

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

[Karsten1987]

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.

[sloretz]

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().

[wjwwood]

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.

[Karsten1987]

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.

[wjwwood]

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.

[sloretz]

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.

[wjwwood]

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.

[wjwwood]

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.

[wjwwood]

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

[Karsten1987]

closing this in favor of #468