From 4fc5de7aa55d53d3cdd72aa9ae7d890ccdfefbe0 Mon Sep 17 00:00:00 2001 From: "Chen.Lihui" Date: Fri, 4 Sep 2020 10:38:02 +0800 Subject: [PATCH 1/4] Fix that not to delete some objects after destroying functions Signed-off-by: Chen.Lihui --- rmw_cyclonedds_cpp/src/rmw_node.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/rmw_cyclonedds_cpp/src/rmw_node.cpp b/rmw_cyclonedds_cpp/src/rmw_node.cpp index 7d57005a..9d74dab6 100644 --- a/rmw_cyclonedds_cpp/src/rmw_node.cpp +++ b/rmw_cyclonedds_cpp/src/rmw_node.cpp @@ -3819,6 +3819,8 @@ static rmw_ret_t rmw_init_cs( fail_subtopic: dds_delete(pubtopic); fail_pubtopic: + delete sub; + delete pub; return RMW_RET_ERROR; } @@ -3827,6 +3829,8 @@ static void rmw_fini_cs(CddsCS * cs) dds_delete(cs->sub->rdcondh); dds_delete(cs->sub->enth); dds_delete(cs->pub->enth); + delete cs->sub; + delete cs->pub; } static rmw_ret_t destroy_client(const rmw_node_t * node, rmw_client_t * client) @@ -3859,6 +3863,7 @@ static rmw_ret_t destroy_client(const rmw_node_t * node, rmw_client_t * client) } rmw_fini_cs(&info->client); + delete info; rmw_free(const_cast(client->service_name)); rmw_client_free(client); return RMW_RET_OK; @@ -3915,6 +3920,7 @@ extern "C" rmw_client_t * rmw_create_client( rmw_client_free(rmw_client); fail_client: rmw_fini_cs(&info->client); + delete info; return nullptr; } @@ -3953,6 +3959,7 @@ static rmw_ret_t destroy_service(const rmw_node_t * node, rmw_service_t * servic } rmw_fini_cs(&info->service); + delete info; rmw_free(const_cast(service->service_name)); rmw_service_free(service); return RMW_RET_OK; @@ -4007,6 +4014,7 @@ extern "C" rmw_service_t * rmw_create_service( rmw_service_free(rmw_service); fail_service: rmw_fini_cs(&info->service); + delete info; return nullptr; } From def772c516d70da7eb3f7a9480097fcce596ff29 Mon Sep 17 00:00:00 2001 From: "Chen.Lihui" Date: Mon, 7 Sep 2020 09:14:01 +0800 Subject: [PATCH 2/4] avoid double free Signed-off-by: Chen.Lihui Co-authored-by: Tomoya.Fujita --- rmw_cyclonedds_cpp/src/rmw_node.cpp | 97 ++++++++++++++++------------- 1 file changed, 55 insertions(+), 42 deletions(-) diff --git a/rmw_cyclonedds_cpp/src/rmw_node.cpp b/rmw_cyclonedds_cpp/src/rmw_node.cpp index 9d74dab6..447cda59 100644 --- a/rmw_cyclonedds_cpp/src/rmw_node.cpp +++ b/rmw_cyclonedds_cpp/src/rmw_node.cpp @@ -3829,8 +3829,14 @@ static void rmw_fini_cs(CddsCS * cs) dds_delete(cs->sub->rdcondh); dds_delete(cs->sub->enth); dds_delete(cs->pub->enth); - delete cs->sub; - delete cs->pub; + if (cs->sub) { + delete cs->sub; + cs->sub = NULL; + } + if (cs->pub) { + delete cs->pub; + cs->pub = NULL; + } } static rmw_ret_t destroy_client(const rmw_node_t * node, rmw_client_t * client) @@ -3839,31 +3845,34 @@ static rmw_ret_t destroy_client(const rmw_node_t * node, rmw_client_t * client) RET_WRONG_IMPLID(node); RET_NULL(client); RET_WRONG_IMPLID(client); - auto info = static_cast(client->data); clean_waitset_caches(); - { - // Update graph - auto common = &node->context->impl->common; - std::lock_guard guard(common->node_update_mutex); - static_cast(common->graph_cache.dissociate_writer( - info->client.pub->gid, common->gid, - node->name, node->namespace_)); - rmw_dds_common::msg::ParticipantEntitiesInfo msg = - common->graph_cache.dissociate_reader( - info->client.sub->gid, common->gid, node->name, - node->namespace_); - if (RMW_RET_OK != rmw_publish( - common->pub, - static_cast(&msg), - nullptr)) + if (client->data) { + auto info = static_cast(client->data); { - RMW_SET_ERROR_MSG("failed to publish ParticipantEntitiesInfo when destroying service"); + // Update graph + auto common = &node->context->impl->common; + std::lock_guard guard(common->node_update_mutex); + static_cast(common->graph_cache.dissociate_writer( + info->client.pub->gid, common->gid, + node->name, node->namespace_)); + rmw_dds_common::msg::ParticipantEntitiesInfo msg = + common->graph_cache.dissociate_reader( + info->client.sub->gid, common->gid, node->name, + node->namespace_); + if (RMW_RET_OK != rmw_publish( + common->pub, + static_cast(&msg), + nullptr)) + { + RMW_SET_ERROR_MSG("failed to publish ParticipantEntitiesInfo when destroying service"); + } } - } - rmw_fini_cs(&info->client); - delete info; + rmw_fini_cs(&info->client); + delete info; + client->data = NULL; + } rmw_free(const_cast(client->service_name)); rmw_client_free(client); return RMW_RET_OK; @@ -3935,31 +3944,35 @@ static rmw_ret_t destroy_service(const rmw_node_t * node, rmw_service_t * servic RET_WRONG_IMPLID(node); RET_NULL(service); RET_WRONG_IMPLID(service); - auto info = static_cast(service->data); clean_waitset_caches(); - { - // Update graph - auto common = &node->context->impl->common; - std::lock_guard guard(common->node_update_mutex); - static_cast(common->graph_cache.dissociate_writer( - info->service.pub->gid, common->gid, - node->name, node->namespace_)); - rmw_dds_common::msg::ParticipantEntitiesInfo msg = - common->graph_cache.dissociate_reader( - info->service.sub->gid, common->gid, node->name, - node->namespace_); - if (RMW_RET_OK != rmw_publish( - common->pub, - static_cast(&msg), - nullptr)) + if (service->data) { + auto info = static_cast(service->data); + { - RMW_SET_ERROR_MSG("failed to publish ParticipantEntitiesInfo when destroying service"); + // Update graph + auto common = &node->context->impl->common; + std::lock_guard guard(common->node_update_mutex); + static_cast(common->graph_cache.dissociate_writer( + info->service.pub->gid, common->gid, + node->name, node->namespace_)); + rmw_dds_common::msg::ParticipantEntitiesInfo msg = + common->graph_cache.dissociate_reader( + info->service.sub->gid, common->gid, node->name, + node->namespace_); + if (RMW_RET_OK != rmw_publish( + common->pub, + static_cast(&msg), + nullptr)) + { + RMW_SET_ERROR_MSG("failed to publish ParticipantEntitiesInfo when destroying service"); + } } - } - rmw_fini_cs(&info->service); - delete info; + rmw_fini_cs(&info->service); + delete info; + service->data = NULL; + } rmw_free(const_cast(service->service_name)); rmw_service_free(service); return RMW_RET_OK; From 6e20e18fc29952fa2f837c062c45019e55cb8e42 Mon Sep 17 00:00:00 2001 From: "Chen.Lihui" Date: Wed, 9 Sep 2020 15:54:04 +0800 Subject: [PATCH 3/4] Use unique_ptr for CddsCS Signed-off-by: Chen.Lihui Co-authored-by: eboasson --- rmw_cyclonedds_cpp/src/rmw_node.cpp | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/rmw_cyclonedds_cpp/src/rmw_node.cpp b/rmw_cyclonedds_cpp/src/rmw_node.cpp index 447cda59..240ac523 100644 --- a/rmw_cyclonedds_cpp/src/rmw_node.cpp +++ b/rmw_cyclonedds_cpp/src/rmw_node.cpp @@ -333,8 +333,8 @@ struct client_service_id_t struct CddsCS { - CddsPublisher * pub; - CddsSubscription * sub; + std::unique_ptr pub; + std::unique_ptr sub; client_service_id_t id; }; @@ -3704,8 +3704,8 @@ static rmw_ret_t rmw_init_cs( const rosidl_service_type_support_t * type_support = get_service_typesupport(type_supports); RET_NULL(type_support); - auto pub = new CddsPublisher(); - auto sub = new CddsSubscription(); + auto pub = std::make_unique(); + auto sub = std::make_unique(); std::string subtopic_name, pubtopic_name; void * pub_type_support, * sub_type_support; @@ -3802,8 +3802,8 @@ static rmw_ret_t rmw_init_cs( dds_delete(subtopic); dds_delete(pubtopic); - cs->pub = pub; - cs->sub = sub; + cs->pub = std::move(pub); + cs->sub = std::move(sub); return RMW_RET_OK; fail_instance_handle: @@ -3819,8 +3819,6 @@ static rmw_ret_t rmw_init_cs( fail_subtopic: dds_delete(pubtopic); fail_pubtopic: - delete sub; - delete pub; return RMW_RET_ERROR; } @@ -3829,14 +3827,6 @@ static void rmw_fini_cs(CddsCS * cs) dds_delete(cs->sub->rdcondh); dds_delete(cs->sub->enth); dds_delete(cs->pub->enth); - if (cs->sub) { - delete cs->sub; - cs->sub = NULL; - } - if (cs->pub) { - delete cs->pub; - cs->pub = NULL; - } } static rmw_ret_t destroy_client(const rmw_node_t * node, rmw_client_t * client) From 04bf5bdd44b41a59e45760661461bcdf8e6f88e5 Mon Sep 17 00:00:00 2001 From: "Chen.Lihui" Date: Thu, 10 Sep 2020 18:11:13 +0800 Subject: [PATCH 4/4] Keep same destroy behavior for data with [client/service]_name Signed-off-by: Chen.Lihui --- rmw_cyclonedds_cpp/src/rmw_node.cpp | 87 +++++++++++++---------------- 1 file changed, 40 insertions(+), 47 deletions(-) diff --git a/rmw_cyclonedds_cpp/src/rmw_node.cpp b/rmw_cyclonedds_cpp/src/rmw_node.cpp index 240ac523..45324893 100644 --- a/rmw_cyclonedds_cpp/src/rmw_node.cpp +++ b/rmw_cyclonedds_cpp/src/rmw_node.cpp @@ -3835,34 +3835,31 @@ static rmw_ret_t destroy_client(const rmw_node_t * node, rmw_client_t * client) RET_WRONG_IMPLID(node); RET_NULL(client); RET_WRONG_IMPLID(client); + auto info = static_cast(client->data); clean_waitset_caches(); - if (client->data) { - auto info = static_cast(client->data); + { + // Update graph + auto common = &node->context->impl->common; + std::lock_guard guard(common->node_update_mutex); + static_cast(common->graph_cache.dissociate_writer( + info->client.pub->gid, common->gid, + node->name, node->namespace_)); + rmw_dds_common::msg::ParticipantEntitiesInfo msg = + common->graph_cache.dissociate_reader( + info->client.sub->gid, common->gid, node->name, + node->namespace_); + if (RMW_RET_OK != rmw_publish( + common->pub, + static_cast(&msg), + nullptr)) { - // Update graph - auto common = &node->context->impl->common; - std::lock_guard guard(common->node_update_mutex); - static_cast(common->graph_cache.dissociate_writer( - info->client.pub->gid, common->gid, - node->name, node->namespace_)); - rmw_dds_common::msg::ParticipantEntitiesInfo msg = - common->graph_cache.dissociate_reader( - info->client.sub->gid, common->gid, node->name, - node->namespace_); - if (RMW_RET_OK != rmw_publish( - common->pub, - static_cast(&msg), - nullptr)) - { - RMW_SET_ERROR_MSG("failed to publish ParticipantEntitiesInfo when destroying service"); - } + RMW_SET_ERROR_MSG("failed to publish ParticipantEntitiesInfo when destroying service"); } - - rmw_fini_cs(&info->client); - delete info; - client->data = NULL; } + + rmw_fini_cs(&info->client); + delete info; rmw_free(const_cast(client->service_name)); rmw_client_free(client); return RMW_RET_OK; @@ -3934,35 +3931,31 @@ static rmw_ret_t destroy_service(const rmw_node_t * node, rmw_service_t * servic RET_WRONG_IMPLID(node); RET_NULL(service); RET_WRONG_IMPLID(service); + auto info = static_cast(service->data); clean_waitset_caches(); - if (service->data) { - auto info = static_cast(service->data); - + { + // Update graph + auto common = &node->context->impl->common; + std::lock_guard guard(common->node_update_mutex); + static_cast(common->graph_cache.dissociate_writer( + info->service.pub->gid, common->gid, + node->name, node->namespace_)); + rmw_dds_common::msg::ParticipantEntitiesInfo msg = + common->graph_cache.dissociate_reader( + info->service.sub->gid, common->gid, node->name, + node->namespace_); + if (RMW_RET_OK != rmw_publish( + common->pub, + static_cast(&msg), + nullptr)) { - // Update graph - auto common = &node->context->impl->common; - std::lock_guard guard(common->node_update_mutex); - static_cast(common->graph_cache.dissociate_writer( - info->service.pub->gid, common->gid, - node->name, node->namespace_)); - rmw_dds_common::msg::ParticipantEntitiesInfo msg = - common->graph_cache.dissociate_reader( - info->service.sub->gid, common->gid, node->name, - node->namespace_); - if (RMW_RET_OK != rmw_publish( - common->pub, - static_cast(&msg), - nullptr)) - { - RMW_SET_ERROR_MSG("failed to publish ParticipantEntitiesInfo when destroying service"); - } + RMW_SET_ERROR_MSG("failed to publish ParticipantEntitiesInfo when destroying service"); } - - rmw_fini_cs(&info->service); - delete info; - service->data = NULL; } + + rmw_fini_cs(&info->service); + delete info; rmw_free(const_cast(service->service_name)); rmw_service_free(service); return RMW_RET_OK;