From 6df0003fe6d36fbcab77b5a663acba738e265890 Mon Sep 17 00:00:00 2001 From: Antonio Cuadros <49162117+Acuadros95@users.noreply.github.com> Date: Thu, 26 Jan 2023 08:10:51 +0100 Subject: [PATCH] Fix memory release on entities creation (#280) * Update CI * Fix memory release on entity creation Signed-off-by: acuadros95 * Update node destroy on error Signed-off-by: acuadros95 * Uncrustify Signed-off-by: acuadros95 Signed-off-by: acuadros95 --- rmw_microxrcedds_c/src/rmw_client.c | 10 +++------- rmw_microxrcedds_c/src/rmw_microxrcedds_topic.c | 15 +++++++-------- rmw_microxrcedds_c/src/rmw_node.c | 17 +++++++++++++++++ rmw_microxrcedds_c/src/rmw_publisher.c | 13 +++++++------ rmw_microxrcedds_c/src/rmw_service.c | 7 +------ rmw_microxrcedds_c/src/rmw_subscription.c | 13 +++++++------ 6 files changed, 42 insertions(+), 33 deletions(-) diff --git a/rmw_microxrcedds_c/src/rmw_client.c b/rmw_microxrcedds_c/src/rmw_client.c index db029901..2f44bb23 100644 --- a/rmw_microxrcedds_c/src/rmw_client.c +++ b/rmw_microxrcedds_c/src/rmw_client.c @@ -33,6 +33,7 @@ rmw_create_client( const char * service_name, const rmw_qos_profile_t * qos_policies) { + rmw_uxrce_client_t * custom_client = NULL; rmw_client_t * rmw_client = NULL; if (!node) { RMW_UROS_TRACE_MESSAGE("node handle is null") @@ -51,10 +52,10 @@ rmw_create_client( RMW_UROS_TRACE_MESSAGE("Not available memory node") return NULL; } - rmw_uxrce_client_t * custom_client = (rmw_uxrce_client_t *)memory_node->data; + custom_client = (rmw_uxrce_client_t *)memory_node->data; rmw_client = &custom_client->rmw_client; - rmw_client->data = NULL; + rmw_client->data = custom_client; rmw_client->implementation_identifier = rmw_get_implementation_identifier(); rmw_client->service_name = custom_client->service_name; if ((strlen(service_name) + 1 ) > sizeof(custom_client->service_name)) { @@ -125,7 +126,6 @@ rmw_create_client( RMW_UXRCE_TYPE_NAME_MAX_LENGTH)) { RMW_UROS_TRACE_MESSAGE("Not enough memory for service type names") - put_memory(&client_memory, &custom_client->mem); goto fail; } @@ -136,7 +136,6 @@ rmw_create_client( RMW_UXRCE_TOPIC_NAME_MAX_LENGTH)) { RMW_UROS_TRACE_MESSAGE("Not enough memory for service topic names") - put_memory(&client_memory, &custom_client->mem); goto fail; } @@ -154,13 +153,10 @@ rmw_create_client( UXR_REPLACE | UXR_REUSE); #endif /* ifdef RMW_UXRCE_USE_XML */ - rmw_client->data = custom_client; - if (!run_xrce_session( custom_node->context, custom_node->context->creation_stream, client_req, custom_node->context->creation_timeout)) { - put_memory(&client_memory, &custom_client->mem); goto fail; } diff --git a/rmw_microxrcedds_c/src/rmw_microxrcedds_topic.c b/rmw_microxrcedds_c/src/rmw_microxrcedds_topic.c index 741d622c..96818d9d 100644 --- a/rmw_microxrcedds_c/src/rmw_microxrcedds_topic.c +++ b/rmw_microxrcedds_c/src/rmw_microxrcedds_topic.c @@ -58,8 +58,6 @@ create_topic( sizeof(rmw_uxrce_entity_naming_buffer))) { RMW_UROS_TRACE_MESSAGE("failed to generate xml request for node creation") - rmw_uxrce_fini_topic_memory(custom_topic); - custom_topic = NULL; goto fail; } @@ -73,15 +71,11 @@ create_topic( if (!generate_topic_name(topic_name, full_topic_name, sizeof(full_topic_name))) { RMW_UROS_TRACE_MESSAGE("Error creating topic name"); - rmw_uxrce_fini_topic_memory(custom_topic); - custom_topic = NULL; goto fail; } if (!generate_type_name(message_type_support_callbacks, type_name, sizeof(type_name))) { RMW_UROS_TRACE_MESSAGE("Error creating type name"); - rmw_uxrce_fini_topic_memory(custom_topic); - custom_topic = NULL; goto fail; } @@ -99,12 +93,17 @@ create_topic( custom_node->context, custom_node->context->creation_stream, topic_req, custom_node->context->creation_timeout)) { - rmw_uxrce_fini_topic_memory(custom_topic); - custom_topic = NULL; goto fail; } + return custom_topic; + fail: + if (custom_topic != NULL) { + rmw_uxrce_fini_topic_memory(custom_topic); + } + + custom_topic = NULL; return custom_topic; } diff --git a/rmw_microxrcedds_c/src/rmw_node.c b/rmw_microxrcedds_c/src/rmw_node.c index e6503628..448ebdce 100644 --- a/rmw_microxrcedds_c/src/rmw_node.c +++ b/rmw_microxrcedds_c/src/rmw_node.c @@ -169,6 +169,11 @@ rmw_ret_t rmw_destroy_node( item = item->next; if (custom_publisher->owner_node == custom_node) { ret = rmw_destroy_publisher(node, &custom_publisher->rmw_publisher); + + // We should not early return on a RMW_RET_TIMEOUT, as it may be an expected output + if (RMW_RET_ERROR == ret) { + return ret; + } } } @@ -178,6 +183,10 @@ rmw_ret_t rmw_destroy_node( item = item->next; if (custom_subscription->owner_node == custom_node) { ret = rmw_destroy_subscription(node, &custom_subscription->rmw_subscription); + + if (RMW_RET_ERROR == ret) { + return ret; + } } } @@ -187,6 +196,10 @@ rmw_ret_t rmw_destroy_node( item = item->next; if (custom_service->owner_node == custom_node) { ret = rmw_destroy_service(node, &custom_service->rmw_service); + + if (RMW_RET_ERROR == ret) { + return ret; + } } } @@ -196,6 +209,10 @@ rmw_ret_t rmw_destroy_node( item = item->next; if (custom_client->owner_node == custom_node) { ret = rmw_destroy_client(node, &custom_client->rmw_client); + + if (RMW_RET_ERROR == ret) { + return ret; + } } } diff --git a/rmw_microxrcedds_c/src/rmw_publisher.c b/rmw_microxrcedds_c/src/rmw_publisher.c index 896342c4..3412b3bb 100644 --- a/rmw_microxrcedds_c/src/rmw_publisher.c +++ b/rmw_microxrcedds_c/src/rmw_publisher.c @@ -67,6 +67,7 @@ rmw_create_publisher( { (void)publisher_options; + rmw_uxrce_publisher_t * custom_publisher = NULL; rmw_publisher_t * rmw_publisher = NULL; if (!node) { RMW_UROS_TRACE_MESSAGE("node handle is null") @@ -85,10 +86,10 @@ rmw_create_publisher( RMW_UROS_TRACE_MESSAGE("Not available memory node") return NULL; } - rmw_uxrce_publisher_t * custom_publisher = (rmw_uxrce_publisher_t *)memory_node->data; + custom_publisher = (rmw_uxrce_publisher_t *)memory_node->data; rmw_publisher = &custom_publisher->rmw_publisher; - rmw_publisher->data = NULL; + rmw_publisher->data = custom_publisher; rmw_publisher->implementation_identifier = rmw_get_implementation_identifier(); rmw_publisher->topic_name = custom_publisher->topic_name; @@ -171,12 +172,9 @@ rmw_create_publisher( custom_node->context, custom_node->context->creation_stream, publisher_req, custom_node->context->creation_timeout)) { - put_memory(&publisher_memory, &custom_publisher->mem); goto fail; } - rmw_publisher->data = custom_publisher; - // Create datawriter custom_publisher->datawriter_id = uxr_object_id( custom_node->context->id_datawriter++, @@ -212,13 +210,16 @@ rmw_create_publisher( custom_node->context, custom_node->context->creation_stream, datawriter_req, custom_node->context->creation_timeout)) { - put_memory(&publisher_memory, &custom_publisher->mem); goto fail; } } return rmw_publisher; fail: + if (custom_publisher != NULL && custom_publisher->topic != NULL) { + rmw_uxrce_fini_topic_memory(custom_publisher->topic); + } + rmw_uxrce_fini_publisher_memory(rmw_publisher); rmw_publisher = NULL; return rmw_publisher; diff --git a/rmw_microxrcedds_c/src/rmw_service.c b/rmw_microxrcedds_c/src/rmw_service.c index dc8c98a0..39fc389c 100644 --- a/rmw_microxrcedds_c/src/rmw_service.c +++ b/rmw_microxrcedds_c/src/rmw_service.c @@ -54,7 +54,7 @@ rmw_create_service( rmw_uxrce_service_t * custom_service = (rmw_uxrce_service_t *)memory_node->data; rmw_service = &custom_service->rmw_service; - rmw_service->data = NULL; + rmw_service->data = custom_service; rmw_service->implementation_identifier = rmw_get_implementation_identifier(); rmw_service->service_name = custom_service->service_name; if ((strlen(service_name) + 1 ) > sizeof(custom_service->service_name)) { @@ -123,7 +123,6 @@ rmw_create_service( RMW_UXRCE_TYPE_NAME_MAX_LENGTH)) { RMW_UROS_TRACE_MESSAGE("Not enough memory for service type names") - put_memory(&service_memory, &custom_service->mem); goto fail; } @@ -134,7 +133,6 @@ rmw_create_service( RMW_UXRCE_TOPIC_NAME_MAX_LENGTH)) { RMW_UROS_TRACE_MESSAGE("Not enough memory for service topic names") - put_memory(&service_memory, &custom_service->mem); goto fail; } @@ -152,14 +150,11 @@ rmw_create_service( UXR_REPLACE | UXR_REUSE); #endif /* ifdef RMW_UXRCE_USE_XML */ - rmw_service->data = custom_service; - if (!run_xrce_session( custom_node->context, custom_node->context->creation_stream, service_req, custom_node->context->creation_timeout)) { RMW_UROS_TRACE_MESSAGE("Issues creating Micro XRCE-DDS entities") - put_memory(&service_memory, &custom_service->mem); goto fail; } diff --git a/rmw_microxrcedds_c/src/rmw_subscription.c b/rmw_microxrcedds_c/src/rmw_subscription.c index 0ee7ed70..6b460497 100644 --- a/rmw_microxrcedds_c/src/rmw_subscription.c +++ b/rmw_microxrcedds_c/src/rmw_subscription.c @@ -65,6 +65,7 @@ rmw_create_subscription( { (void)subscription_options; + rmw_uxrce_subscription_t * custom_subscription = NULL; rmw_subscription_t * rmw_subscription = NULL; if (!node) { RMW_UROS_TRACE_MESSAGE("node handle is null") @@ -85,10 +86,10 @@ rmw_create_subscription( RMW_UROS_TRACE_MESSAGE("Not available memory node") return NULL; } - rmw_uxrce_subscription_t * custom_subscription = (rmw_uxrce_subscription_t *)memory_node->data; + custom_subscription = (rmw_uxrce_subscription_t *)memory_node->data; rmw_subscription = &custom_subscription->rmw_subscription; - rmw_subscription->data = NULL; + rmw_subscription->data = custom_subscription; rmw_subscription->implementation_identifier = rmw_get_implementation_identifier(); rmw_subscription->topic_name = custom_subscription->topic_name; if ((strlen(topic_name) + 1 ) > sizeof(custom_subscription->topic_name)) { @@ -159,7 +160,6 @@ rmw_create_subscription( custom_node->context, custom_node->context->creation_stream, subscriber_req, custom_node->context->creation_timeout)) { - put_memory(&subscription_memory, &custom_subscription->mem); goto fail; } @@ -198,12 +198,9 @@ rmw_create_subscription( custom_node->context->creation_timeout)) { RMW_UROS_TRACE_MESSAGE("Issues creating Micro XRCE-DDS entities") - put_memory(&subscription_memory, &custom_subscription->mem); goto fail; } - rmw_subscription->data = custom_subscription; - uxrDeliveryControl delivery_control; delivery_control.max_samples = UXR_MAX_SAMPLES_UNLIMITED; delivery_control.min_pace_period = 0; @@ -223,6 +220,10 @@ rmw_create_subscription( return rmw_subscription; fail: + if (custom_subscription != NULL && custom_subscription->topic != NULL) { + rmw_uxrce_fini_topic_memory(custom_subscription->topic); + } + rmw_uxrce_fini_subscription_memory(rmw_subscription); rmw_subscription = NULL; return rmw_subscription;