Sort alphabetically fill_names_and_types

[ahcorde]

ros2node test are failing because the names are not sorted alphabetically, this should fix the issue

Related with this issue #523

[Yadunund]

hmm we actually introduced ordered_map in #250 to preserve the order in which entities were introduced into the system. That was required to fix some other tests.
Could you double check that sorting alphabetically does not regress those other tests? Also it might be worthwhile to look into how the topics are filled in DDS-based middlewares which utilize rmw_dds_common::GraphCache.

[ahcorde]

hmm we actually introduced ordered_map in #250 to preserve the order in which entities were introduced into the system. That was required to fix some other tests. Could you double check that sorting alphabetically does not regress those other tests? Also it might be worthwhile to look into how the topics are filled in DDS-based middlewares which utilize rmw_dds_common::GraphCache.

@Yadunund According with the documentation in the code, we use a ordered map but remember by insertion https://github.com/ros2/rmw_zenoh/blob/rolling/rmw_zenoh_cpp/src/detail/graph_cache.hpp#L85-L88

I reruned all the stack I can't see any new error

[Yadunund]

hmm we actually introduced ordered_map in #250 to preserve the order in which entities were introduced into the system. That was required to fix some other tests. Could you double check that sorting alphabetically does not regress those other tests? Also it might be worthwhile to look into how the topics are filled in DDS-based middlewares which utilize rmw_dds_common::GraphCache.

@Yadunund According with the documentation in the code, we use a ordered map but remember by insertion https://github.com/ros2/rmw_zenoh/blob/rolling/rmw_zenoh_cpp/src/detail/graph_cache.hpp#L85-L88

I reruned all the stack I can't see any new error

My point is that we explicitly switched to the custom ordered_map which preserves insertion order as some parts of the ROS 2 stack required this when calling get_names_and _types.
So I'd like to be careful about this change and first ascertain what the original need was to switch to ordered_map.
If not required (unlikely) we might as well declare TopicMap as a map that stores keys alphabetically. Creating a temp map and inserting topics into that as you have it now adds unnecessary overhead at runtime. Hence, it would also be good to look at how the DDS graph cache handles both insertion ordering and alphabetical ordering requirements.

[YuanYuYuan]

I browsed the other DDS-based implementations. They use the GraphCache::get_names_and_types to return the topics, which create a NamesAndTypes topics (basically a std::map).

To align with them, we can preserve the std::ordered_map structure for some requirement by client libraries and create an additional sorted map for rmw_get_topic_names_and_types call. This is exactly what @ahcorde does in this PR.

[YuanYuYuan]

A small suggestion: For better efficiency, we can create and sort only the keys. I also tried wrapping the ordered_map key with std::reference_wrapper, but it doesn't seem to be supported. So, we need to clone the std::string with minimal overhead.

diff --git a/rmw_zenoh_cpp/src/detail/graph_cache.cpp b/rmw_zenoh_cpp/src/detail/graph_cache.cpp
index e24a53e..e8c1d8d 100644
--- a/rmw_zenoh_cpp/src/detail/graph_cache.cpp
+++ b/rmw_zenoh_cpp/src/detail/graph_cache.cpp
@@ -770,15 +770,24 @@ rmw_ret_t fill_names_and_types(
     });
   // Fill topic names and types.
   std::size_t index = 0;
-  for (const std::pair<std::string, GraphNode::TopicTypeMap> & item : entity_map) {
-    names_and_types->names.data[index] = rcutils_strdup(item.first.c_str(), *allocator);
+
+  // Sort the topics of entity_map alphabetically
+  std::vector<std::string> sorted_topics;
+  for (const auto & item : entity_map) {
+    sorted_topics.push_back(item.first);
+  }
+  std::sort(sorted_topics.begin(), sorted_topics.end());
+
+  for (const auto & topic : sorted_topics) {
+    names_and_types->names.data[index] = rcutils_strdup(topic.c_str(), *allocator);
     if (!names_and_types->names.data[index]) {
       return RMW_RET_BAD_ALLOC;
     }
 
+    auto topic_map = entity_map.at(topic);
     rcutils_ret_t rcutils_ret = rcutils_string_array_init(
       &names_and_types->types[index],
-      item.second.size(),
+      topic_map.size(),
       allocator);
     if (RCUTILS_RET_OK != rcutils_ret) {
       RMW_SET_ERROR_MSG(rcutils_get_error_string().str);
@@ -786,7 +795,7 @@ rmw_ret_t fill_names_and_types(
     }
 
     size_t type_index = 0;
-    for (const std::pair<const std::string, GraphNode::TopicQoSMap> & type : item.second) {
+    for (const std::pair<const std::string, GraphNode::TopicQoSMap> & type : topic_map) {
       char * type_name = rcutils_strdup(_demangle_if_ros_type(type.first).c_str(), *allocator);
       if (!type_name) {
         RMW_SET_ERROR_MSG("failed to allocate memory for type name");

[ahcorde]

If I use a std::map o std::unordered_map instead of the tsl::ordered_map. This was the PR that this was introduced c12ff3e

I can see some error in rcl:

	 97 - test_get_node_names__rmw_zenoh_cpp (Failed)

or rclcpp

	 44 - test_node_interfaces__node_graph (Failed)

[YuanYuYuan]

If I use a std::map o std::unordered_map instead of the tsl::ordered_map. This was the PR that this was introduced c12ff3e

I can see some error in rcl:

	 97 - test_get_node_names__rmw_zenoh_cpp (Failed)

or rclcpp

	 44 - test_node_interfaces__node_graph (Failed)

Hi @ahcorde, I tested this branch against rcl, rclcpp, and system_tests. I didn't see any failures.
refactor-ordered-map-results.log

[ahcorde]

@YuanYuYuan That's true, I had a hanging process which made these tests to fail

Do you min to try this one #546?

[YuanYuYuan]

@YuanYuYuan That's true, I had a hanging process which made these tests to fail

Do you min to try this one #546?

Sure. Will do.

[YuanYuYuan]

1. use std::map in topicMap type #546 works fine. I believe we can use a std::map now.
2. I can reproduce the failure on rcl/test_get_node_names__rmw_zenoh_cpp and rclcpp/test_node_interfaces__node_graph if I have some hang processes caused by the failing ros2topic.

[Yadunund]

If I use a std::map o std::unordered_map instead of the tsl::ordered_map. This was the PR that this was introduced c12ff3e

I can see some error in rcl:

	 97 - test_get_node_names__rmw_zenoh_cpp (Failed)

or rclcpp

	 44 - test_node_interfaces__node_graph (Failed)

So here's where i'm confused.

rcl/test_get_node_names calls rcl_get_node_names_with_enclaves which in turn calls rmw_get_node_names_with_enclaves. The implementation of rmw_get_node_names_with_enclaves in rmw_zennoh calls GraphCache::get_node_names which simply iterates over an std::unordered_map to get the list of node names. So it should not require any ordering.

My original question still stands, which is the test that actually requires topic names in a node to be ordered? Because the ordered_map is only used here. I'm not comfortable with switching to std::map without ascertaining this.