From 178cc38062f7631d31672a31e4012f9c82ff19bf Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 27 Nov 2023 18:02:24 +0000 Subject: [PATCH] Remove Entity::node_info. Because it returned a reference, it is difficult to tell the lifetime of it. It would also be hard to add in locking in the future. Since we are reaching through it in most cases, just return some data directly. Signed-off-by: Chris Lalancette --- rmw_zenoh_cpp/src/detail/graph_cache.cpp | 34 +++++++++---------- rmw_zenoh_cpp/src/detail/liveliness_utils.cpp | 20 ++++++++--- rmw_zenoh_cpp/src/detail/liveliness_utils.hpp | 7 ++-- 3 files changed, 37 insertions(+), 24 deletions(-) diff --git a/rmw_zenoh_cpp/src/detail/graph_cache.cpp b/rmw_zenoh_cpp/src/detail/graph_cache.cpp index b4e82dd9..40293ad8 100644 --- a/rmw_zenoh_cpp/src/detail/graph_cache.cpp +++ b/rmw_zenoh_cpp/src/detail/graph_cache.cpp @@ -143,9 +143,9 @@ void GraphCache::parse_put(const std::string & keyexpr) [&](const Entity & entity) -> std::shared_ptr { auto graph_node = std::make_shared(); - graph_node->ns_ = entity.node_info().ns_; - graph_node->name_ = entity.node_info().name_; - graph_node->enclave_ = entity.node_info().enclave_; + graph_node->ns_ = entity.node_namespace(); + graph_node->name_ = entity.node_name(); + graph_node->enclave_ = entity.node_enclave(); if (!entity.topic_info().has_value()) { // Token was for a node. @@ -161,26 +161,26 @@ void GraphCache::parse_put(const std::string & keyexpr) std::lock_guard lock(graph_mutex_); // If the namespace did not exist, create it and add the node to the graph and return. - auto ns_it = graph_.find(entity.node_info().ns_); + auto ns_it = graph_.find(entity.node_namespace()); if (ns_it == graph_.end()) { std::unordered_map node_map = { - {entity.node_info().name_, make_graph_node(entity)}}; - graph_.insert(std::make_pair(entity.node_info().ns_, std::move(node_map))); + {entity.node_name(), make_graph_node(entity)}}; + graph_.insert(std::make_pair(entity.node_namespace(), std::move(node_map))); RCUTILS_LOG_WARN_NAMED( "rmw_zenoh_cpp", "Added node /%s to a new namespace %s in the graph.", - entity.node_info().name_.c_str(), - entity.node_info().ns_.c_str()); + entity.node_name().c_str(), + entity.node_namespace().c_str()); return; } // Add the node to the namespace if it did not exist and return. - auto node_it = ns_it->second.find(entity.node_info().name_); + auto node_it = ns_it->second.find(entity.node_name()); if (node_it == ns_it->second.end()) { - ns_it->second.insert(std::make_pair(entity.node_info().name_, make_graph_node(entity))); + ns_it->second.insert(std::make_pair(entity.node_name(), make_graph_node(entity))); RCUTILS_LOG_WARN_NAMED( "rmw_zenoh_cpp", "Added node /%s to an existing namespace %s in the graph.", - entity.node_info().name_.c_str(), - entity.node_info().ns_.c_str()); + entity.node_name().c_str(), + entity.node_namespace().c_str()); return; } @@ -304,13 +304,13 @@ void GraphCache::parse_del(const std::string & keyexpr) std::lock_guard lock(graph_mutex_); // If namespace does not exist, ignore the request. - auto ns_it = graph_.find(entity.node_info().ns_); + auto ns_it = graph_.find(entity.node_namespace()); if (ns_it == graph_.end()) { return; } // If the node does not exist, ignore the request. - auto node_it = ns_it->second.find(entity.node_info().name_); + auto node_it = ns_it->second.find(entity.node_name()); if (node_it == ns_it->second.end()) { return; } @@ -326,16 +326,16 @@ void GraphCache::parse_del(const std::string & keyexpr) "rmw_zenoh_cpp", "Received liveliness token to remove node /%s from the graph before all pub/subs for this " "node have been removed. Report this issue.", - entity.node_info().name_.c_str() + entity.node_name().c_str() ); // TODO(Yadunund): Iterate through the nodes pubs_ and subs_ and decrement topic count in // graph_topics_. } - ns_it->second.erase(entity.node_info().name_); + ns_it->second.erase(entity.node_name()); RCUTILS_LOG_WARN_NAMED( "rmw_zenoh_cpp", "Removed node /%s from the graph.", - entity.node_info().name_.c_str() + entity.node_name().c_str() ); return; } diff --git a/rmw_zenoh_cpp/src/detail/liveliness_utils.cpp b/rmw_zenoh_cpp/src/detail/liveliness_utils.cpp index 589335df..0e3a108c 100644 --- a/rmw_zenoh_cpp/src/detail/liveliness_utils.cpp +++ b/rmw_zenoh_cpp/src/detail/liveliness_utils.cpp @@ -119,7 +119,7 @@ Entity::Entity( * /////// */ std::stringstream token_ss; - const auto & ns = node_info_.ns_; + const std::string & ns = node_info_.ns_; token_ss << ADMIN_SPACE << "/" << node_info_.domain_id_ << "/" << entity_to_str.at(type_) << ns; // An empty namespace from rcl will contain "/" but zenoh does not allow keys with "//". // Hence we add an "_" to denote an empty namespace such that splitting the key @@ -230,7 +230,8 @@ std::optional Entity::make(const std::string & keyexpr) // Get the entity, ie NN, MP, MS, SS, SC. std::string & entity_str = parts[2]; - const auto entity_it = str_to_entity.find(entity_str); + std::unordered_map::const_iterator entity_it = + str_to_entity.find(entity_str); if (entity_it == str_to_entity.end()) { RCUTILS_LOG_ERROR_NAMED( "rmw_zenoh_cpp", @@ -270,10 +271,19 @@ const EntityType & Entity::type() const return this->type_; } -///============================================================================= -const NodeInfo & Entity::node_info() const +std::string Entity::node_namespace() const +{ + return this->node_info_.ns_; +} + +std::string Entity::node_name() const +{ + return this->node_info_.name_; +} + +std::string Entity::node_enclave() const { - return this->node_info_; + return this->node_info_.enclave_; } ///============================================================================= diff --git a/rmw_zenoh_cpp/src/detail/liveliness_utils.hpp b/rmw_zenoh_cpp/src/detail/liveliness_utils.hpp index 5f07ea4b..529ef092 100644 --- a/rmw_zenoh_cpp/src/detail/liveliness_utils.hpp +++ b/rmw_zenoh_cpp/src/detail/liveliness_utils.hpp @@ -89,8 +89,11 @@ class Entity /// Get the entity type. const EntityType & type() const; - /// Get the node_info. - const NodeInfo & node_info() const; + std::string node_namespace() const; + + std::string node_name() const; + + std::string node_enclave() const; /// Get the topic_info. const std::optional & topic_info() const;