Node name in user data

[dirk-thomas]

Connect to ros2/ros2#438.

[mikaelarguedas]

lgtm except for one comment about hardcoded values

[mikaelarguedas]

We would reduce the number of hardcoded values if we defined this prefix above and use it's length everywhere below (rather than hardcoding 5s and 6s)

[dirk-thomas]

Done in 221769e.

[mikaelarguedas]

Nit: enums and litterals on left side of operators

[dirk-thomas]

Done in 1164cea.

[mikaelarguedas]

This generates a warning on Windows

[dirk-thomas]

Fixed.

[dhood]

Are you testing this with opensplice 6.7 or a different version? When I tested the behaviour with ros2 node list cross-vendor (to verify ros2/ros2cli#72 is resolved), it didn't work. A connext daemon will detect an opensplice talker node now, but still has issues with whatever other participants opensplice spawns.

[dhood]

The participant name might be null (that seems to be the case for whatever additional participants opensplice is spawning). If we just store the participant name without checking, this causes a segfault at the if (name.empty()) check on the std::string below

[dirk-thomas]

That makes sense. While the participant name is a std::string in other implementations it is a char * in Connext. I checked for NULL in ae30090 before assigning it to a std::string.

[dhood]

I don't think continueing here is a good idea, higher layers are probably assuming that the node_names map will return valid strings (nothing in the rcl_get_node_names documentation suggests otherwise). FWICT rclpy, for example, is assuming null-terminated strings are stored at each index: https://github.com/ros2/rclpy/blob/fbd80a27342d56b32047ecb7f2ad2e44e9d027b3/rclpy/src/rclpy/_rclpy.c#L2323

[dirk-thomas]

The line you refenced should be fine if NULL is passed in as far as I understand the API docs:

If u is NULL, this function behaves like PyUnicode_FromUnicode() with the buffer set to NULL. This usage is deprecated in favor of PyUnicode_New().

[dhood]

Looks like you're referencing the documentation for PyUnicode_FromStringAndSize, PyUnicode_FromString says:

Create a Unicode object from a UTF-8 encoded null-terminated char buffer u.

The error (when it doesn't segfault) is: UnicodeDecodeError: 'utf-8' codec can't decode byte 0xf0 in position 0: invalid continuation byte

[dirk-thomas]

You are right, I stopped at the first match and didn't check the exact function name 😞

[dirk-thomas]

Are you testing this with opensplice 6.7 or a different version?

Yes, with 6.7.

When I tested the behaviour with ros2 node list cross-vendor (to verify ros2/ros2cli#72 is resolved), it didn't work. A connext daemon will detect an opensplice talker node now, but still has issues with whatever other participants opensplice spawns.

Can you describe what exact issues you are seeing. Maybe with a sequence of commands you are running in order to reproduce it.

[dhood]

The specific issue is the segfault that I mentioned in the inline comments.

The sequence is the same as in the initial issue:

RMW_IMPLEMENTATION=rmw_opensplice_cpp ros2 run demo_nodes_cpp talker

$ RMW_IMPLEMENTATION=rmw_connext_cpp ros2 node list
RTI Data Distribution Service EVAL License issued to OSRF dhood@osrfoundation.org For non-production use only.
Expires on 05-Mar-2018 See www.rti.com for more information.
Segmentation fault (core dumped)

[dhood]

What output are you seeing/how are you testing the cross-vendor behaviour? Does ros2 node list have blank lines still? Maybe it's using an older version of the daemon instead of a "fresh" one?

[dirk-thomas]

I was able to reproduce the problem with the OpenSplice talker and the Connext node list. I created PRs to document the behavior of the rcl API as well as fix the usage in rclcpp and rclpy.

Two considered alternatives:

• Each rmw implementation could implement rmw_get_node_names() with a two-pass: the first iteration only checks how many non-null names exist, then it allocates the array with the exact size, in the second iteration it populates the array with all non-null names.
• If the rcutils_string_array_t struct would have a capacity member storing the number of array items which actually contain data (beside the existing allocated size) null node names could be skipped in each rmw implementation already in a single pass without the need for reallocating the array.