Fix memory release on entities creation (#280)

* Update CI * Fix memory release on entity creation Signed-off-by: acuadros95 <[email protected]> * Update node destroy on error Signed-off-by: acuadros95 <[email protected]> * Uncrustify Signed-off-by: acuadros95 <[email protected]> Signed-off-by: acuadros95 <[email protected]>
micro-ROS · Jan 26, 2023 · 6df0003 · 6df0003
1 parent 40c1750
commit 6df0003
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 33 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;