Fix memory release on entities creation (#280) (#283)

mergify[bot] · Acuadros95 · web-flow · commit 8cfa3bcfd949 · 2023-01-26T08:24:02.000+01:00
* Update CI * Fix memory release on entity creation Signed-off-by: acuadros95 <acuadros1995@gmail.com> * Update node destroy on error Signed-off-by: acuadros95 <acuadros1995@gmail.com> * Uncrustify Signed-off-by: acuadros95 <acuadros1995@gmail.com> Signed-off-by: acuadros95 <acuadros1995@gmail.com> (cherry picked from commit 6df0003) Co-authored-by: Antonio Cuadros <49162117+Acuadros95@users.noreply.github.com>
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
@@ -173,6 +173,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;
+      }
     }
   }
 
@@ -182,6 +187,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;
+      }
     }
   }
 
@@ -191,6 +200,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;
+      }
     }
   }
 
@@ -200,6 +213,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;

Original file line number	Diff line number	Diff line change
`@@ -33,6 +33,7 @@ rmw_create_client(`
`33`	`33`	`const char * service_name,`
`34`	`34`	`const rmw_qos_profile_t * qos_policies)`
`35`	`35`	`{`
	`36`	`+ rmw_uxrce_client_t * custom_client = NULL;`
`36`	`37`	`rmw_client_t * rmw_client = NULL;`
`37`	`38`	`if (!node) {`
`38`	`39`	`RMW_UROS_TRACE_MESSAGE("node handle is null")`
`@@ -51,10 +52,10 @@ rmw_create_client(`
`51`	`52`	`RMW_UROS_TRACE_MESSAGE("Not available memory node")`
`52`	`53`	`return NULL;`
`53`	`54`	`}`
`54`		`- rmw_uxrce_client_t * custom_client = (rmw_uxrce_client_t *)memory_node->data;`
	`55`	`+ custom_client = (rmw_uxrce_client_t *)memory_node->data;`
`55`	`56`
`56`	`57`	`rmw_client = &custom_client->rmw_client;`
`57`		`- rmw_client->data = NULL;`
	`58`	`+ rmw_client->data = custom_client;`
`58`	`59`	`rmw_client->implementation_identifier = rmw_get_implementation_identifier();`
`59`	`60`	`rmw_client->service_name = custom_client->service_name;`
`60`	`61`	`if ((strlen(service_name) + 1 ) > sizeof(custom_client->service_name)) {`
`@@ -125,7 +126,6 @@ rmw_create_client(`
`125`	`126`	`RMW_UXRCE_TYPE_NAME_MAX_LENGTH))`
`126`	`127`	`{`
`127`	`128`	`RMW_UROS_TRACE_MESSAGE("Not enough memory for service type names")`
`128`		`- put_memory(&client_memory, &custom_client->mem);`
`129`	`129`	`goto fail;`
`130`	`130`	`}`
`131`	`131`
`@@ -136,7 +136,6 @@ rmw_create_client(`
`136`	`136`	`RMW_UXRCE_TOPIC_NAME_MAX_LENGTH))`
`137`	`137`	`{`
`138`	`138`	`RMW_UROS_TRACE_MESSAGE("Not enough memory for service topic names")`
`139`		`- put_memory(&client_memory, &custom_client->mem);`
`140`	`139`	`goto fail;`
`141`	`140`	`}`
`142`	`141`
`@@ -154,13 +153,10 @@ rmw_create_client(`
`154`	`153`	`UXR_REPLACE \| UXR_REUSE);`
`155`	`154`	`#endif /* ifdef RMW_UXRCE_USE_XML */`
`156`	`155`
`157`		`- rmw_client->data = custom_client;`
`158`		`-`
`159`	`156`	`if (!run_xrce_session(`
`160`	`157`	`custom_node->context, custom_node->context->creation_stream, client_req,`
`161`	`158`	`custom_node->context->creation_timeout))`
`162`	`159`	`{`
`163`		`- put_memory(&client_memory, &custom_client->mem);`
`164`	`160`	`goto fail;`
`165`	`161`	`}`
`166`	`162`
Original file line number	Diff line number	Diff line change
`@@ -58,8 +58,6 @@ create_topic(`
`58`	`58`	`sizeof(rmw_uxrce_entity_naming_buffer)))`
`59`	`59`	`{`
`60`	`60`	`RMW_UROS_TRACE_MESSAGE("failed to generate xml request for node creation")`
`61`		`- rmw_uxrce_fini_topic_memory(custom_topic);`
`62`		`- custom_topic = NULL;`
`63`	`61`	`goto fail;`
`64`	`62`	`}`
`65`	`63`
`@@ -73,15 +71,11 @@ create_topic(`
`73`	`71`
`74`	`72`	`if (!generate_topic_name(topic_name, full_topic_name, sizeof(full_topic_name))) {`
`75`	`73`	`RMW_UROS_TRACE_MESSAGE("Error creating topic name");`
`76`		`- rmw_uxrce_fini_topic_memory(custom_topic);`
`77`		`- custom_topic = NULL;`
`78`	`74`	`goto fail;`
`79`	`75`	`}`
`80`	`76`
`81`	`77`	`if (!generate_type_name(message_type_support_callbacks, type_name, sizeof(type_name))) {`
`82`	`78`	`RMW_UROS_TRACE_MESSAGE("Error creating type name");`
`83`		`- rmw_uxrce_fini_topic_memory(custom_topic);`
`84`		`- custom_topic = NULL;`
`85`	`79`	`goto fail;`
`86`	`80`	`}`
`87`	`81`
`@@ -99,12 +93,17 @@ create_topic(`
`99`	`93`	`custom_node->context, custom_node->context->creation_stream, topic_req,`
`100`	`94`	`custom_node->context->creation_timeout))`
`101`	`95`	`{`
`102`		`- rmw_uxrce_fini_topic_memory(custom_topic);`
`103`		`- custom_topic = NULL;`
`104`	`96`	`goto fail;`
`105`	`97`	`}`
`106`	`98`
	`99`	`+ return custom_topic;`
	`100`	`+`
`107`	`101`	`fail:`
	`102`	`+ if (custom_topic != NULL) {`
	`103`	`+ rmw_uxrce_fini_topic_memory(custom_topic);`
	`104`	`+ }`
	`105`	`+`
	`106`	`+ custom_topic = NULL;`
`108`	`107`	`return custom_topic;`
`109`	`108`	`}`
`110`	`109`
Original file line number	Diff line number	Diff line change
`@@ -173,6 +173,11 @@ rmw_ret_t rmw_destroy_node(`
`173`	`173`	`item = item->next;`
`174`	`174`	`if (custom_publisher->owner_node == custom_node) {`
`175`	`175`	`ret = rmw_destroy_publisher(node, &custom_publisher->rmw_publisher);`
	`176`	`+`
	`177`	`+ // We should not early return on a RMW_RET_TIMEOUT, as it may be an expected output`
	`178`	`+ if (RMW_RET_ERROR == ret) {`
	`179`	`+ return ret;`
	`180`	`+ }`
`176`	`181`	`}`
`177`	`182`	`}`
`178`	`183`
`@@ -182,6 +187,10 @@ rmw_ret_t rmw_destroy_node(`
`182`	`187`	`item = item->next;`
`183`	`188`	`if (custom_subscription->owner_node == custom_node) {`
`184`	`189`	`ret = rmw_destroy_subscription(node, &custom_subscription->rmw_subscription);`
	`190`	`+`
	`191`	`+ if (RMW_RET_ERROR == ret) {`
	`192`	`+ return ret;`
	`193`	`+ }`
`185`	`194`	`}`
`186`	`195`	`}`
`187`	`196`
`@@ -191,6 +200,10 @@ rmw_ret_t rmw_destroy_node(`
`191`	`200`	`item = item->next;`
`192`	`201`	`if (custom_service->owner_node == custom_node) {`
`193`	`202`	`ret = rmw_destroy_service(node, &custom_service->rmw_service);`
	`203`	`+`
	`204`	`+ if (RMW_RET_ERROR == ret) {`
	`205`	`+ return ret;`
	`206`	`+ }`
`194`	`207`	`}`
`195`	`208`	`}`
`196`	`209`
`@@ -200,6 +213,10 @@ rmw_ret_t rmw_destroy_node(`
`200`	`213`	`item = item->next;`
`201`	`214`	`if (custom_client->owner_node == custom_node) {`
`202`	`215`	`ret = rmw_destroy_client(node, &custom_client->rmw_client);`
	`216`	`+`
	`217`	`+ if (RMW_RET_ERROR == ret) {`
	`218`	`+ return ret;`
	`219`	`+ }`
`203`	`220`	`}`
`204`	`221`	`}`
`205`	`222`
Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ rmw_create_service(`
`54`	`54`	`rmw_uxrce_service_t * custom_service = (rmw_uxrce_service_t *)memory_node->data;`
`55`	`55`
`56`	`56`	`rmw_service = &custom_service->rmw_service;`
`57`		`- rmw_service->data = NULL;`
	`57`	`+ rmw_service->data = custom_service;`
`58`	`58`	`rmw_service->implementation_identifier = rmw_get_implementation_identifier();`
`59`	`59`	`rmw_service->service_name = custom_service->service_name;`
`60`	`60`	`if ((strlen(service_name) + 1 ) > sizeof(custom_service->service_name)) {`
`@@ -123,7 +123,6 @@ rmw_create_service(`
`123`	`123`	`RMW_UXRCE_TYPE_NAME_MAX_LENGTH))`
`124`	`124`	`{`
`125`	`125`	`RMW_UROS_TRACE_MESSAGE("Not enough memory for service type names")`
`126`		`- put_memory(&service_memory, &custom_service->mem);`
`127`	`126`	`goto fail;`
`128`	`127`	`}`
`129`	`128`
`@@ -134,7 +133,6 @@ rmw_create_service(`
`134`	`133`	`RMW_UXRCE_TOPIC_NAME_MAX_LENGTH))`
`135`	`134`	`{`
`136`	`135`	`RMW_UROS_TRACE_MESSAGE("Not enough memory for service topic names")`
`137`		`- put_memory(&service_memory, &custom_service->mem);`
`138`	`136`	`goto fail;`
`139`	`137`	`}`
`140`	`138`
`@@ -152,14 +150,11 @@ rmw_create_service(`
`152`	`150`	`UXR_REPLACE \| UXR_REUSE);`
`153`	`151`	`#endif /* ifdef RMW_UXRCE_USE_XML */`
`154`	`152`
`155`		`- rmw_service->data = custom_service;`
`156`		`-`
`157`	`153`	`if (!run_xrce_session(`
`158`	`154`	`custom_node->context, custom_node->context->creation_stream, service_req,`
`159`	`155`	`custom_node->context->creation_timeout))`
`160`	`156`	`{`
`161`	`157`	`RMW_UROS_TRACE_MESSAGE("Issues creating Micro XRCE-DDS entities")`
`162`		`- put_memory(&service_memory, &custom_service->mem);`
`163`	`158`	`goto fail;`
`164`	`159`	`}`
`165`	`160`