fix rclc_example: memory leaking in msg.data allocation (backport #386)…

… (#387) * fix rclc_example: memory leaking in msg.data allocation (#386) Signed-off-by: zhangtonghe <[email protected]> Co-authored-by: zhangtonghe <[email protected]> (cherry picked from commit 7af2054) * fix: deprecated rcl_timer_init (#389) * fix: deprecated rcl_timer_init Signed-off-by: Zard-C <[email protected]> Signed-off-by: zhangtonghe <[email protected]> Signed-off-by: Zard-C <[email protected]> * fix: switch rcl_timer_init to rcl_timer_init2 Signed-off-by: Zard-C <[email protected]> Signed-off-by: zhangtonghe <[email protected]> Signed-off-by: Zard-C <[email protected]> * update rclc_timer_init_default and it's invokation Signed-off-by: zhangtonghe <[email protected]> Signed-off-by: Zard-C <[email protected]> * Revert "update rclc_timer_init_default and it's invokation" This reverts commit ebc3edb. Signed-off-by: zhangtonghe <[email protected]> Signed-off-by: Zard-C <[email protected]> * fix: rcl_timer_init usage Combined changes from multiple commits to refactor the usage of rcl_timer_init for better compatibility and clarity. Signed-off-by: Zard-C <[email protected]> Signed-off-by: zhangtonghe <[email protected]> * Update rclc/include/rclc/timer.h Co-authored-by: Jan Staschulat <[email protected]> Signed-off-by: Zard-C <[email protected]> --------- Signed-off-by: Zard-C <[email protected]> Signed-off-by: zhangtonghe <[email protected]> Co-authored-by: zhangtonghe <[email protected]> Co-authored-by: Jan Staschulat <[email protected]> --------- Signed-off-by: Zard-C <[email protected]> Signed-off-by: zhangtonghe <[email protected]> Co-authored-by: Zard-C <[email protected]> Co-authored-by: zhangtonghe <[email protected]> Co-authored-by: Jan Staschulat <[email protected]>
ros2 · Aug 22, 2023 · d263be2 · d263be2
1 parent 14c2c41
commit d263be2
Show file tree

Hide file tree

Showing 11 changed files with 87 additions and 43 deletions.
diff --git a/rclc/include/rclc/timer.h b/rclc/include/rclc/timer.h
@@ -40,11 +40,26 @@ extern "C"
  * \param[in] support the rclc_support_t object
  * \param[in] timeout_ns the time out in nanoseconds of the timer
  * \param[in] callback the callback of the timer
+ * \param[in] autostart the state of the timer at initialization
  * \return `RCL_RET_OK` if successful
  * \return `RCL_ERROR` (or other error code) if an error occurred
  */
 RCLC_PUBLIC
 rcl_ret_t
+rclc_timer_init_default2(
+  rcl_timer_t * timer,
+  rclc_support_t * support,
+  const uint64_t timeout_ns,
+  const rcl_timer_callback_t callback,
+  bool autostart);
+
+/**
+ * \deprecated `rclc_timer_init_default` implementation was removed.
+ *   Refer to `rclc_timer_init_default2`.
+ */
+RCL_PUBLIC
+RCUTILS_DEPRECATED_WITH_MSG("Call rclc_timer_init_default2 instead")
+rcl_ret_t
 rclc_timer_init_default(
   rcl_timer_t * timer,
   rclc_support_t * support,

diff --git a/rclc/src/rclc/timer.c b/rclc/src/rclc/timer.c
@@ -20,29 +20,47 @@
 #include <rcutils/logging_macros.h>
 
 rcl_ret_t
-rclc_timer_init_default(
+rclc_timer_init_default2(
   rcl_timer_t * timer,
   rclc_support_t * support,
   const uint64_t timeout_ns,
-  const rcl_timer_callback_t callback)
+  const rcl_timer_callback_t callback,
+  bool autostart)
 {
   RCL_CHECK_FOR_NULL_WITH_MSG(
     timer, "timer is a null pointer", return RCL_RET_INVALID_ARGUMENT);
   RCL_CHECK_FOR_NULL_WITH_MSG(
     support, "support is a null pointer", return RCL_RET_INVALID_ARGUMENT);
 
   (*timer) = rcl_get_zero_initialized_timer();
-  rcl_ret_t rc = rcl_timer_init(
+  rcl_ret_t rc = rcl_timer_init2(
     timer,
     &support->clock,
     &support->context,
     timeout_ns,
     callback,
-    (*support->allocator));
+    (*support->allocator),
+    autostart);
   if (rc != RCL_RET_OK) {
-    PRINT_RCLC_ERROR(rclc_timer_init_default, rcl_timer_init);
+    PRINT_RCLC_ERROR(rclc_timer_init_default, rcl_timer_init2);
   } else {
     RCUTILS_LOG_INFO("Created a timer with period %ld ms.\n", timeout_ns / 1000000);
   }
   return rc;
 }
+
+rcl_ret_t
+rclc_timer_init_default(
+  rcl_timer_t * timer,
+  rclc_support_t * support,
+  const uint64_t timeout_ns,
+  const rcl_timer_callback_t callback)
+{
+  return rclc_timer_init_default2(
+    timer,
+    support,
+    timeout_ns,
+    callback,
+    true
+  );
+}
diff --git a/rclc/test/rclc/test_executor.cpp b/rclc/test/rclc/test_executor.cpp
@@ -572,9 +572,9 @@ class TestDefaultExecutor : public ::testing::Test
     ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
     this->timer1 = rcl_get_zero_initialized_timer();
     ret =
-      rcl_timer_init(
+      rcl_timer_init2(
       &this->timer1, &this->clock, &this->context, RCL_MS_TO_NS(
-        this->timer1_timeout), my_timer_callback, this->clock_allocator);
+        this->timer1_timeout), my_timer_callback, this->clock_allocator, true);
     ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
   }
 

diff --git a/rclc/test/rclc/test_timer.cpp b/rclc/test/rclc/test_timer.cpp
@@ -22,7 +22,7 @@ void my_callback(rcl_timer_t * timer, int64_t last_call)
   RCLC_UNUSED(last_call);
 }
 
-TEST(Test, rclc_timer_init_default) {
+TEST(Test, rclc_timer_init_default2) {
   rclc_support_t support;
   rcl_ret_t rc;
 
@@ -33,18 +33,19 @@ TEST(Test, rclc_timer_init_default) {
   const char * my_namespace = "test_namespace";
   rcl_node_t node = rcl_get_zero_initialized_node();
   rc = rclc_node_init_default(&node, my_name, my_namespace, &support);
+  bool autostart = true;
 
   // test with valid arguments
 
   rcl_timer_t timer = rcl_get_zero_initialized_timer();
-  rc = rclc_timer_init_default(&timer, &support, 10000000, my_callback);
+  rc = rclc_timer_init_default2(&timer, &support, 10000000, my_callback, autostart);
   EXPECT_EQ(RCL_RET_OK, rc);
 
   // tests with invalid arguments
-  rc = rclc_timer_init_default(nullptr, &support, 10000000, my_callback);
+  rc = rclc_timer_init_default2(nullptr, &support, 10000000, my_callback, autostart);
   EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rc);
   rcutils_reset_error();
-  rc = rclc_timer_init_default(&timer, nullptr, 10000000, my_callback);
+  rc = rclc_timer_init_default2(&timer, nullptr, 10000000, my_callback, autostart);
   EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rc);
   rcutils_reset_error();
 

diff --git a/rclc_examples/src/example_executor.c b/rclc_examples/src/example_executor.c
@@ -94,13 +94,14 @@ int main(int argc, const char * argv[])
   // create a timer, which will call the publisher with period=`timer_timeout` ms in the 'my_timer_callback'
   rcl_timer_t my_timer = rcl_get_zero_initialized_timer();
   const unsigned int timer_timeout = 1000;
-  rc = rclc_timer_init_default(
+  rc = rclc_timer_init_default2(
     &my_timer,
     &support,
     RCL_MS_TO_NS(timer_timeout),
-    my_timer_callback);
+    my_timer_callback,
+    true);
   if (rc != RCL_RET_OK) {
-    printf("Error in rcl_timer_init_default.\n");
+    printf("Error in rclc_timer_init_default2.\n");
     return -1;
   } else {
     printf("Created timer with timeout %d ms.\n", timer_timeout);
@@ -109,7 +110,7 @@ int main(int argc, const char * argv[])
   // assign message to publisher
   std_msgs__msg__String__init(&pub_msg);
   const unsigned int PUB_MSG_CAPACITY = 20;
-  pub_msg.data.data = malloc(PUB_MSG_CAPACITY);
+  pub_msg.data.data = allocator.reallocate(pub_msg.data.data, PUB_MSG_CAPACITY, allocator.state);
   pub_msg.data.capacity = PUB_MSG_CAPACITY;
   snprintf(pub_msg.data.data, pub_msg.data.capacity, "Hello World!");
   pub_msg.data.size = strlen(pub_msg.data.data);

diff --git a/rclc_examples/src/example_executor_only_rcl.c b/rclc_examples/src/example_executor_only_rcl.c
@@ -110,15 +110,16 @@ int main(int argc, const char * argv[])
   }
   rcl_timer_t my_timer = rcl_get_zero_initialized_timer();
   const unsigned int timer_timeout = 1000;
-  rc = rcl_timer_init(
+  rc = rcl_timer_init2(
     &my_timer,
     &clock,
     &context,
     RCL_MS_TO_NS(timer_timeout),
     my_timer_callback,
-    allocator);
+    allocator,
+    true);
   if (rc != RCL_RET_OK) {
-    printf("Error in rcl_timer_init.\n");
+    printf("Error in rcl_timer_init2.\n");
     return -1;
   } else {
     printf("Created timer with timeout %d ms.\n", timer_timeout);
@@ -127,7 +128,7 @@ int main(int argc, const char * argv[])
   // assign message to publisher
   std_msgs__msg__String__init(&pub_msg);
   const unsigned int PUB_MSG_CAPACITY = 20;
-  pub_msg.data.data = malloc(PUB_MSG_CAPACITY);
+  pub_msg.data.data = allocator.reallocate(pub_msg.data.data, PUB_MSG_CAPACITY, allocator.state);
   pub_msg.data.capacity = PUB_MSG_CAPACITY;
   snprintf(pub_msg.data.data, pub_msg.data.capacity, "Hello World!");
   pub_msg.data.size = strlen(pub_msg.data.data);

diff --git a/rclc_examples/src/example_executor_trigger.c b/rclc_examples/src/example_executor_trigger.c
@@ -139,14 +139,15 @@ void my_int_subscriber_callback(const void * msgin)
 void my_timer_string_callback(rcl_timer_t * timer, int64_t last_call_time)
 {
   rcl_ret_t rc;
+  rcl_allocator_t allocator = rcl_get_default_allocator();
   RCLC_UNUSED(last_call_time);
   if (timer != NULL) {
     //printf("Timer: time since last call %d\n", (int) last_call_time);
 
     std_msgs__msg__String pub_msg;
     std_msgs__msg__String__init(&pub_msg);
     const unsigned int PUB_MSG_CAPACITY = 20;
-    pub_msg.data.data = malloc(PUB_MSG_CAPACITY);
+    pub_msg.data.data = allocator.reallocate(pub_msg.data.data, PUB_MSG_CAPACITY, allocator.state);
     pub_msg.data.capacity = PUB_MSG_CAPACITY;
     snprintf(pub_msg.data.data, pub_msg.data.capacity, "Hello World!%d", string_pub_value++);
     pub_msg.data.size = strlen(pub_msg.data.data);
@@ -224,13 +225,14 @@ int main(int argc, const char * argv[])
   // - publishes 'my_string_pub' every 'timer_timeout' ms
   rcl_timer_t my_string_timer = rcl_get_zero_initialized_timer();
   const unsigned int timer_timeout = 100;
-  rc = rclc_timer_init_default(
+  rc = rclc_timer_init_default2(
     &my_string_timer,
     &support,
     RCL_MS_TO_NS(timer_timeout),
-    my_timer_string_callback);
+    my_timer_string_callback,
+    true);
   if (rc != RCL_RET_OK) {
-    printf("Error in rclc_timer_init_default.\n");
+    printf("Error in rclc_timer_init_default2.\n");
     return -1;
   } else {
     printf("Created timer 'my_string_timer' with timeout %d ms.\n", timer_timeout);
@@ -256,13 +258,14 @@ int main(int argc, const char * argv[])
   // - publishes 'my_int_pub' every 'timer_int_timeout' ms
   rcl_timer_t my_int_timer = rcl_get_zero_initialized_timer();
   const unsigned int timer_int_timeout = 10 * timer_timeout;
-  rc = rclc_timer_init_default(
+  rc = rclc_timer_init_default2(
     &my_int_timer,
     &support,
     RCL_MS_TO_NS(timer_int_timeout),
-    my_timer_int_callback);
+    my_timer_int_callback,
+    true);
   if (rc != RCL_RET_OK) {
-    printf("Error in rclc_timer_init_default.\n");
+    printf("Error in rclc_timer_init_default2.\n");
     return -1;
   } else {
     printf("Created timer with timeout %d ms.\n", timer_int_timeout);

diff --git a/rclc_examples/src/example_parameter_server.c b/rclc_examples/src/example_parameter_server.c
@@ -94,11 +94,12 @@ int main()
 
   // create timer,
   rcl_timer_t timer;
-  rclc_timer_init_default(
+  rclc_timer_init_default2(
     &timer,
     &support,
     RCL_MS_TO_NS(1000),
-    timer_callback);
+    timer_callback,
+    true);
 
   // Create executor
   // Note:

diff --git a/rclc_examples/src/example_pingpong.cpp b/rclc_examples/src/example_pingpong.cpp
@@ -166,13 +166,14 @@ int main(int argc, const char * argv[])
   // create a timer, which will call the publisher with period=`timer_timeout` ms in the 'my_timer_callback'
   rcl_timer_t ping_timer ;
   const unsigned int timer_timeout = 50; //50 milliseconds userdefined value
-  rc = rclc_timer_init_default(
+  rc = rclc_timer_init_default2(
     &ping_timer,
     &support,
     RCL_MS_TO_NS(timer_timeout),
-    ping_timer_callback);
+    ping_timer_callback,
+    true);
   if (rc != RCL_RET_OK) {
-    printf("Error in rcl_timer_init_default.\n");
+    printf("Error in rclc_timer_init_default2.\n");
     return -1;
   } else {
     printf("Created timer with timeout %d ms.\n", timer_timeout);
@@ -181,7 +182,7 @@ int main(int argc, const char * argv[])
   // assign message to publisher
   std_msgs__msg__String__init(&pingNode_ping_msg);
   const unsigned int PUB_MSG_CAPACITY = 20;
-  pingNode_ping_msg.data.data = (char *) malloc(PUB_MSG_CAPACITY);
+  pingNode_ping_msg.data.data = (char *) allocator.reallocate(pingNode_ping_msg.data.data, PUB_MSG_CAPACITY, allocator.state);
   pingNode_ping_msg.data.capacity = PUB_MSG_CAPACITY;
   snprintf(pingNode_ping_msg.data.data, pingNode_ping_msg.data.capacity, "AAAAAAAAAAAAAAAAAAA");
   pingNode_ping_msg.data.size = strlen(pingNode_ping_msg.data.data);
@@ -243,13 +244,14 @@ int main(int argc, const char * argv[])
   // create a timer, which will call the publisher with period=`timer_timeout` ms in the 'my_timer_callback'
   rcl_timer_t pong_timer ;
   const unsigned int pong_timer_timeout = 50; //50 milliseconds userdefined value
-  rc = rclc_timer_init_default(
+  rc = rclc_timer_init_default2(
     &pong_timer,
     &support,
     RCL_MS_TO_NS(pong_timer_timeout),
-    pong_timer_callback);
+    pong_timer_callback,
+    true);
   if (rc != RCL_RET_OK) {
-    printf("Error in rcl_timer_init_default.\n");
+    printf("Error in rclc_timer_init_default2.\n");
     return -1;
   } else {
     printf("Created timer with timeout %d ms.\n", timer_timeout);
@@ -258,7 +260,7 @@ int main(int argc, const char * argv[])
   // assign message to publisher
   std_msgs__msg__String__init(&pongNode_pong_msg);
   //const unsigned int PUB_MSG_CAPACITY = 20;
-  pongNode_pong_msg.data.data = (char *) malloc(PUB_MSG_CAPACITY);
+  pongNode_pong_msg.data.data = (char *) allocator.reallocate(pongNode_pong_msg.data.data, PUB_MSG_CAPACITY, allocator.state);
   pongNode_pong_msg.data.capacity = PUB_MSG_CAPACITY;
   snprintf(pongNode_pong_msg.data.data, pongNode_pong_msg.data.capacity, "BAAAAAAAAAAAAAAAAAAA");
   pongNode_pong_msg.data.size = strlen(pongNode_pong_msg.data.data);

diff --git a/rclc_examples/src/example_short_timer_long_subscription.c b/rclc_examples/src/example_short_timer_long_subscription.c
@@ -108,27 +108,29 @@ int main(int argc, const char * argv[])
   // create a timer, which will call the publisher with period=`timer_timeout` ms in the 'my_timer_callback'
   rcl_timer_t my_timer = rcl_get_zero_initialized_timer();
   const unsigned int timer_timeout = 1000;
-  rc = rclc_timer_init_default(
+  rc = rclc_timer_init_default2(
     &my_timer,
     &support,
     RCL_MS_TO_NS(timer_timeout),
-    my_timer_callback);
+    my_timer_callback,
+    true);
   if (rc != RCL_RET_OK) {
-    printf("Error in rcl_timer_init_default.\n");
+    printf("Error in rclc_timer_init_default2.\n");
     return -1;
   } else {
     printf("Created timer with timeout %d ms.\n", timer_timeout);
   }
 
   rcl_timer_t short_timer = rcl_get_zero_initialized_timer();
   const unsigned int short_timer_timeout = 100;
-  rc = rclc_timer_init_default(
+  rc = rclc_timer_init_default2(
     &short_timer,
     &support,
     RCL_MS_TO_NS(short_timer_timeout),
-    short_timer_callback);
+    short_timer_callback,
+    true);
   if (rc != RCL_RET_OK) {
-    printf("Error in rcl_timer_init_default.\n");
+    printf("Error in rclc_timer_init_default2.\n");
     return -1;
   } else {
     printf("Created timer with timeout %d ms.\n", short_timer_timeout);

diff --git a/rclc_examples/src/example_sub_context.c b/rclc_examples/src/example_sub_context.c
@@ -111,7 +111,7 @@ int main(int argc, const char * argv[])
     // assign message to publisher
     std_msgs__msg__String__init(&( pub_msgs[i] ) );
     const unsigned int PUB_MSG_CAPACITY = 40;
-    pub_msgs[i].data.data = malloc(PUB_MSG_CAPACITY);
+    pub_msgs[i].data.data = allocator.reallocate(pub_msgs[i].data.data, PUB_MSG_CAPACITY, allocator.state);
     pub_msgs[i].data.capacity = PUB_MSG_CAPACITY;
     snprintf(
       pub_msgs[i].data.data, pub_msgs[i].data.capacity, "Hello World! on %s",