handle node names which are null

[dirk-thomas]

Necessary since each item can potentially contain a nullptr.

Connect to ros2/ros2#438.

[dhood]

Pure DDS participants will show up in this list as null. The current approach returns a list that contains these empty names. At first I didn't see a big issue with this, but reviewing the recently-added tests in ros2/system_tests#260, I was surprised by the behaviour of it outputting blank lines: https://ci.ros2.org/job/ci_linux/4161/testReport/junit/test_rclcpp.build.test_rclcpp/test_node_name__rmw_opensplice_cpp__rmw_connext_cpp_/test_node_name/

From a design perspective, I would think that it is the role of the client library to filter out the implementation detail that rmw implementations may detect these "nameless" participants, since the user is requesting a list of ROS nodes not DDS participants. Otherwise every caller of get_node_names needs to filter it themselves, which seems strange since empty ROS node names aren't actually permitted.

I understand that the test is behaving as expected with this approach, so it may be that my opinion is off for some reason or another (efficiency is the counterargument I suspect). Could I get confirmation that we see it as the user's responsibility to check for empty node names themselves?

[dirk-thomas]

I describes the current behavior as well as the potential alternatives in this comment: ros2/rmw_connext#276 (comment)

[dhood]

The comment that you linked to refers to allowing rmw_get_node_names to return null names, which doesn't preclude filtering it at another level. Though if we filter it anywhere it should probably be done at rmw/rcl (instead of rclcpp like I was suggesting), to save reimplementing it in the separate client libraries.

I understand that it would be frustrating to have these comments come up after these PRs have been merged, but tests also double as usage examples, so when you added a test yesterday it prompted me to re-evaluate this approach because of the user experience.

I don't think the "big" change of these PRs (storing node name in participant data) is what's under question, it's more if we should spin off a followup enhancement that adds filtering at a lower level, which doesn't have to be done immediately. Could be a "help wanted" ticket, if others agree.

[dirk-thomas]

if we should spin off a followup enhancement that adds filtering at a lower level

That is totally fine. I just stated the same kind of question in that ticket and didn't receive any comment. So my perception was nobody cared to do either or and was fine with the status quo. I would be be in favor of implementing option 2 and never return nullptr entries from each rmw impl.

[dhood]

I'll wait for other input and create a spinoff otherwise.

To clarify, I will re-list the options (quoting from ros2/rmw_connext#276 (comment)):
A. As is.
B. 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.
C. 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.

You are in favor of what is labeled as B in this list, correct?
I think that design-wise rmw_get_node_names should only return actual ROS node names (non-null), also B.

[dirk-thomas]

You are in favor of what is labeled as B in this list, correct?

I am in favor of option C assuming that both options B and C will not return nullptr entries in the array anymore.

[dhood]

I'm summarising the tasks in a spinoff ticket.

I think there's another option that doesn't require two-pass and doesn't require changes to the rcutils_string_array_t to have a capacity member (which would require usage updates elsewhere in the codebase).

Inspired by rcutils_split, you could just iterate over the unused parts of the array and deallocate that memory and decrease the size, as is done in the string array fini, yeah?

[dirk-thomas]

you could just iterate over the unused parts of the array and deallocate that memory and decrease the size, as is done in the string array fini, yeah?

As far as I understand your proposal that wouldn't work. If you have an array with two entries and you set the first one to NULL you can't just reduce the size of the array to 1.

The referenced rcutils_string_array_fini function doesn't do that either. It is deallocating "everything" - not only parts of the array.

[dhood]

The array would be overallocated to start with, then populated such that all used elements appear at the front (as I imagine it was in the proposal involving the capacity member). The unused parts of the array that I was referring to would be contiguous and all at the end of the array. So it would be like the string array fini but with the loop starting at index N instead of 0.

edit: linked to string array fini

[dirk-thomas]

Wouldn't you need a function which deallocates the "second unused part" of the array? I don't think that exist atm.

[dhood]

It doesn't exist as a function, no, rcutils_split just takes care of it itself manually. So yeah we could add a function in rcutils' string_array to do it generically (then rmw implementations wouldn't need to "know" how to do that).

The benefit, I see, is that there's no need to change the struct to contain an additional field that only applies to some situations (e.g. for cases where some values in the array are allowed/expected to be null, this additional field isn't useful but still has to be maintained).

[dirk-thomas]

Yes, over-allocating and then calling array_downsize (just to clarify the semantic, not a symbol name proposal) would work.

[dhood]

Spinoff ticket in ros2/ros2#489