Fix rmw_fastrtps dead code

[gaoethan]

Dead code in API rmw_create_subscription()
becasue the condition "rmw_subscription" cannot be true in the
"fail" clauses, the execution cannot reach this statement:

rmw_subscription_free(rmw_subscription);

finally, the "rmw_subscribtion" is free by
rmw_destroy_subscription() when it's not a nullptr

Dead code in API rmw_create_publisher()
because the condition "rmw_publisher" cannot be true in the
"fail" clauses, the execution cannot reach this statement:

rmw_publisher_free(rmw_publisher);

finally, the "rmw_publisher" is free by
rmw_destroy_publisher() when it's not a nullptr

Signed-off-by: Ethan Gao ethan.gao@linux.intel.com

[clalancette]

Usually I'm all for deleting dead code, but in this particular case, I'm slightly concerned that this removal could lead to memory leaks in the future. In particular, if someone inserts some code after a successful rmw_subscription_allocate, then that person might have to add this rmw_subscription_free back in. If they forget to do it, the resulting memory leak would be hard to discover via testing. However, I could go either way on this. @ros2/team, other opinions?

[dirk-thomas]

The current code fails to check if the returned value from rmw_allocate is valid. Once that has been added the code which is removed in this PR is necessary.

[mikaelarguedas]

@gaoethan based on #163 (review), could you modify this PR to check the return value of rmw_allocate and restore the removed code (that won't be dead anymore)?
Thanks!

[gaoethan]

@dirk-thomas @mikaelarguedas If the return value from rmw_allocate is valid, the value is finally returned by the following code and it still won't reach the fail clauses, or I get confused about what you're pointing out?

return rmw_publisher;

[mikaelarguedas]

@gaoethan I think @dirk-thomas was referring to the fact that the return value of the allocation for the topic_name should be checked. and if publisher->topic_name is null it should goto fail otherwise the publisher would be leaked. Same thing for the subscriber.

You can look at other rmw_implementations for the equivalent code:
https://github.com/ros2/rmw_connext/blob/5c3e2fa9158e5d93e53fc50e2112306e0aaff340/rmw_connext_cpp/src/rmw_publisher.cpp#L217-L221

[gaoethan]

@mikaelarguedas thanks, okay, Iet me add that checking

[gaoethan]

@dirk-thomas the check for the return from rmw_allocate is enabled and the previous dead code is alive now :)

[dirk-thomas]

Shoulder be "publisher topic name" instead of "pubiser node name".

Similar below.

[gaoethan]

done. thanks.

[dirk-thomas]

client? Is this supposed to be rmw_client?

Did you build the code locally? If not, please do so in the future as well as run the tests for the changed code locally.

[gaoethan]

please ignore the modification from rmw_client.cpp and it's added here exceptionally, because it has nothing to do with this topic and it should be in another PR to avoid misleading. thanks.

[clalancette]

I just pushed a small fix to the string to fix publiser -> publisher. After that, this patch looks OK to me. I'll run a CI job on it to make sure there is no problem, then I'll merge.

CI:

• Linux
• Linux-aarch64
• macOS
• Windows