From 226019378b228c316f4607aad251fc3150e28e20 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Thu, 19 Dec 2024 21:29:10 +0000 Subject: [PATCH 01/22] Shorten the delay in test_action_server setup. Instead of waiting 250ms between setting up 10 goals (for at least 2.5 seconds), just wait 100ms which reduces this to 1 second. Signed-off-by: Chris Lalancette --- rcl_action/test/rcl_action/test_action_server.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl_action/test/rcl_action/test_action_server.cpp b/rcl_action/test/rcl_action/test_action_server.cpp index 421a5a9eb..df14a6e31 100644 --- a/rcl_action/test/rcl_action/test_action_server.cpp +++ b/rcl_action/test/rcl_action/test_action_server.cpp @@ -822,7 +822,7 @@ class TestActionServerCancelPolicy : public TestActionServer ret = rcl_action_goal_handle_get_info(goal_handle, &goal_infos_out[i]); ASSERT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str; // Sleep so goals have different acceptance times - std::this_thread::sleep_for(std::chrono::milliseconds(250)); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); } } From 1bf23730598e2f50fb08ae740f96a00d086a01c2 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 20 Dec 2024 13:42:37 +0000 Subject: [PATCH 02/22] Small style cleanups in test_action_server.cpp Signed-off-by: Chris Lalancette --- rcl_action/test/rcl_action/test_action_server.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/rcl_action/test/rcl_action/test_action_server.cpp b/rcl_action/test/rcl_action/test_action_server.cpp index df14a6e31..675b79d30 100644 --- a/rcl_action/test/rcl_action/test_action_server.cpp +++ b/rcl_action/test/rcl_action/test_action_server.cpp @@ -11,6 +11,7 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. + #include #include @@ -461,7 +462,8 @@ TEST_F(TestActionServer, test_action_accept_new_goal) } } -TEST_F(TestActionServer, test_action_server_goal_exists) { +TEST_F(TestActionServer, test_action_server_goal_exists) +{ rcl_action_goal_info_t goal_info_out = rcl_action_get_zero_initialized_goal_info(); EXPECT_FALSE(rcl_action_server_goal_exists(nullptr, &goal_info_out)); EXPECT_TRUE(rcl_error_is_set()); @@ -501,7 +503,8 @@ TEST_F(TestActionServer, test_action_server_goal_exists) { this->action_server.impl->num_goal_handles--; } -TEST_F(TestActionServer, test_action_server_notify_goal_done) { +TEST_F(TestActionServer, test_action_server_notify_goal_done) +{ // Invalid action server EXPECT_EQ(RCL_RET_ACTION_SERVER_INVALID, rcl_action_notify_goal_done(nullptr)); rcl_reset_error(); From 09d26e973a3ee7faeeb6be2490e9d2ca3f30e81a Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 20 Dec 2024 13:44:20 +0000 Subject: [PATCH 03/22] Reset the error in rcl_node_type_cache_register_type(). That is, if rcutils_hash_map_set() fails, it sets its own error, so overriding it with our own will cause a warning to print. Make sure to clear it before setting our own. Signed-off-by: Chris Lalancette --- rcl/src/rcl/node_type_cache.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rcl/src/rcl/node_type_cache.c b/rcl/src/rcl/node_type_cache.c index 4be7c3195..b453f9718 100644 --- a/rcl/src/rcl/node_type_cache.c +++ b/rcl/src/rcl/node_type_cache.c @@ -187,6 +187,8 @@ rcl_ret_t rcl_node_type_cache_register_type( &node->impl->registered_types_by_type_hash, type_hash, &type_info_with_registrations)) { + // Reset the error since rcutils_hash_map_set already set it + rcutils_reset_error(); RCL_SET_ERROR_MSG("Failed to update type info"); type_description_interfaces__msg__TypeDescription__destroy( type_info_with_registrations.type_info.type_description); From 33200b776cb1f361601299490f65fb31f95b4647 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 20 Dec 2024 13:45:24 +0000 Subject: [PATCH 04/22] Only unregister a clock jump callback if we have installed it. This avoids a warning on cleanup in rcl_timer_init2. Signed-off-by: Chris Lalancette --- rcl/src/rcl/timer.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/rcl/src/rcl/timer.c b/rcl/src/rcl/timer.c index efe728608..98f3fbc0f 100644 --- a/rcl/src/rcl/timer.c +++ b/rcl/src/rcl/timer.c @@ -198,9 +198,11 @@ rcl_timer_init2( // Should be impossible RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "Failed to fini guard condition after bad alloc"); } - if (RCL_RET_OK != rcl_clock_remove_jump_callback(clock, _rcl_timer_time_jump, timer)) { - // Should be impossible - RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "Failed to remove callback after bad alloc"); + if (RCL_ROS_TIME == impl.clock->type) { + if (RCL_RET_OK != rcl_clock_remove_jump_callback(clock, _rcl_timer_time_jump, timer)) { + // Should be impossible + RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "Failed to remove callback after bad alloc"); + } } RCL_SET_ERROR_MSG("allocating memory failed"); From aa5abfac1797e23c8a3c585929e953e946b475bb Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 20 Dec 2024 14:14:51 +0000 Subject: [PATCH 05/22] Record the return value from rcl_node_type_cache_register_type. Otherwise, in a failure situation we set the error but we actually return RCL_RET_OK to the upper layers, which is odd. Signed-off-by: Chris Lalancette --- rcl_action/src/rcl_action/action_client.c | 6 +++--- rcl_action/src/rcl_action/action_server.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/rcl_action/src/rcl_action/action_client.c b/rcl_action/src/rcl_action/action_client.c index 5e0ec1fab..25aa480c1 100644 --- a/rcl_action/src/rcl_action/action_client.c +++ b/rcl_action/src/rcl_action/action_client.c @@ -231,11 +231,11 @@ rcl_action_client_init( SUBSCRIPTION_INIT(feedback); SUBSCRIPTION_INIT(status); - if (RCL_RET_OK != rcl_node_type_cache_register_type( + ret = rcl_node_type_cache_register_type( node, type_support->get_type_hash_func(type_support), type_support->get_type_description_func(type_support), - type_support->get_type_description_sources_func(type_support))) - { + type_support->get_type_description_sources_func(type_support)); + if (RCL_RET_OK != ret) { rcutils_reset_error(); RCL_SET_ERROR_MSG("Failed to register type for action"); goto fail; diff --git a/rcl_action/src/rcl_action/action_server.c b/rcl_action/src/rcl_action/action_server.c index dfa82be52..7cc9e78e1 100644 --- a/rcl_action/src/rcl_action/action_server.c +++ b/rcl_action/src/rcl_action/action_server.c @@ -201,11 +201,11 @@ rcl_action_server_init( } // Store type hash - if (RCL_RET_OK != rcl_node_type_cache_register_type( + ret = rcl_node_type_cache_register_type( node, type_support->get_type_hash_func(type_support), type_support->get_type_description_func(type_support), - type_support->get_type_description_sources_func(type_support))) - { + type_support->get_type_description_sources_func(type_support)); + if (RCL_RET_OK != ret) { rcutils_reset_error(); RCL_SET_ERROR_MSG("Failed to register type for action"); goto fail; From 344b4358a1dd6c67bd5693f62c559323e6d0f6e5 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 20 Dec 2024 14:15:59 +0000 Subject: [PATCH 06/22] Get rid of completely unnecessary return value translation. This generated code was translating an RCL error to an RCL error, which doesn't make much sense. Just remove the duplicate code. Signed-off-by: Chris Lalancette --- rcl_action/src/rcl_action/action_client.c | 22 ++----------------- rcl_action/src/rcl_action/action_server.c | 26 ++--------------------- 2 files changed, 4 insertions(+), 44 deletions(-) diff --git a/rcl_action/src/rcl_action/action_client.c b/rcl_action/src/rcl_action/action_client.c index 25aa480c1..c634ca24a 100644 --- a/rcl_action/src/rcl_action/action_client.c +++ b/rcl_action/src/rcl_action/action_client.c @@ -114,11 +114,6 @@ _rcl_action_client_fini_impl( if (RCL_RET_OK != ret) { \ rcl_reset_error(); \ RCL_SET_ERROR_MSG("failed to get " #Type " service name"); \ - if (RCL_RET_BAD_ALLOC == ret) { \ - ret = RCL_RET_BAD_ALLOC; \ - } else { \ - ret = RCL_RET_ERROR; \ - } \ goto fail; \ } \ rcl_client_options_t Type ## _service_client_options = { \ @@ -133,12 +128,8 @@ _rcl_action_client_fini_impl( &Type ## _service_client_options); \ allocator.deallocate(Type ## _service_name, allocator.state); \ if (RCL_RET_OK != ret) { \ - if (RCL_RET_BAD_ALLOC == ret) { \ - ret = RCL_RET_BAD_ALLOC; \ - } else if (RCL_RET_SERVICE_NAME_INVALID == ret) { \ + if (RCL_RET_SERVICE_NAME_INVALID == ret) { \ ret = RCL_RET_ACTION_NAME_INVALID; \ - } else { \ - ret = RCL_RET_ERROR; \ } \ goto fail; \ } @@ -150,11 +141,6 @@ _rcl_action_client_fini_impl( if (RCL_RET_OK != ret) { \ rcl_reset_error(); \ RCL_SET_ERROR_MSG("failed to get " #Type " topic name"); \ - if (RCL_RET_BAD_ALLOC == ret) { \ - ret = RCL_RET_BAD_ALLOC; \ - } else { \ - ret = RCL_RET_ERROR; \ - } \ goto fail; \ } \ rcl_subscription_options_t Type ## _topic_subscription_options = \ @@ -170,12 +156,8 @@ _rcl_action_client_fini_impl( &Type ## _topic_subscription_options); \ allocator.deallocate(Type ## _topic_name, allocator.state); \ if (RCL_RET_OK != ret) { \ - if (RCL_RET_BAD_ALLOC == ret) { \ - ret = RCL_RET_BAD_ALLOC; \ - } else if (RCL_RET_TOPIC_NAME_INVALID == ret) { \ + if (RCL_RET_TOPIC_NAME_INVALID == ret) { \ ret = RCL_RET_ACTION_NAME_INVALID; \ - } else { \ - ret = RCL_RET_ERROR; \ } \ goto fail; \ } diff --git a/rcl_action/src/rcl_action/action_server.c b/rcl_action/src/rcl_action/action_server.c index 7cc9e78e1..60cfbbb00 100644 --- a/rcl_action/src/rcl_action/action_server.c +++ b/rcl_action/src/rcl_action/action_server.c @@ -53,13 +53,6 @@ rcl_action_get_zero_initialized_server(void) char * Type ## _service_name = NULL; \ ret = rcl_action_get_ ## Type ## _service_name(action_name, allocator, &Type ## _service_name); \ if (RCL_RET_OK != ret) { \ - if (RCL_RET_BAD_ALLOC == ret) { \ - ret = RCL_RET_BAD_ALLOC; \ - } else if (RCL_RET_ACTION_NAME_INVALID == ret) { \ - ret = RCL_RET_ACTION_NAME_INVALID; \ - } else { \ - ret = RCL_RET_ERROR; \ - } \ goto fail; \ } \ rcl_service_options_t Type ## _service_options = { \ @@ -73,12 +66,8 @@ rcl_action_get_zero_initialized_server(void) &Type ## _service_options); \ allocator.deallocate(Type ## _service_name, allocator.state); \ if (RCL_RET_OK != ret) { \ - if (RCL_RET_BAD_ALLOC == ret) { \ - ret = RCL_RET_BAD_ALLOC; \ - } else if (RCL_RET_SERVICE_NAME_INVALID == ret) { \ + if (RCL_RET_SERVICE_NAME_INVALID == ret) { \ ret = RCL_RET_ACTION_NAME_INVALID; \ - } else { \ - ret = RCL_RET_ERROR; \ } \ goto fail; \ } @@ -87,13 +76,6 @@ rcl_action_get_zero_initialized_server(void) char * Type ## _topic_name = NULL; \ ret = rcl_action_get_ ## Type ## _topic_name(action_name, allocator, &Type ## _topic_name); \ if (RCL_RET_OK != ret) { \ - if (RCL_RET_BAD_ALLOC == ret) { \ - ret = RCL_RET_BAD_ALLOC; \ - } else if (RCL_RET_ACTION_NAME_INVALID == ret) { \ - ret = RCL_RET_ACTION_NAME_INVALID; \ - } else { \ - ret = RCL_RET_ERROR; \ - } \ goto fail; \ } \ rcl_publisher_options_t Type ## _publisher_options = { \ @@ -107,12 +89,8 @@ rcl_action_get_zero_initialized_server(void) &Type ## _publisher_options); \ allocator.deallocate(Type ## _topic_name, allocator.state); \ if (RCL_RET_OK != ret) { \ - if (RCL_RET_BAD_ALLOC == ret) { \ - ret = RCL_RET_BAD_ALLOC; \ - } else if (RCL_RET_TOPIC_NAME_INVALID == ret) { \ + if (RCL_RET_TOPIC_NAME_INVALID == ret) { \ ret = RCL_RET_ACTION_NAME_INVALID; \ - } else { \ - ret = RCL_RET_ERROR; \ } \ goto fail; \ } From 25ede715fa3595bf358d5429235b75bdcaa28ca3 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 20 Dec 2024 14:18:19 +0000 Subject: [PATCH 07/22] Use the rcl_timer_init2 functionality to start the timer disabled. Rather than starting it enabled, and then immediately canceling it. Signed-off-by: Chris Lalancette --- rcl_action/src/rcl_action/action_server.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/rcl_action/src/rcl_action/action_server.c b/rcl_action/src/rcl_action/action_server.c index 60cfbbb00..9568d64cd 100644 --- a/rcl_action/src/rcl_action/action_server.c +++ b/rcl_action/src/rcl_action/action_server.c @@ -158,15 +158,10 @@ rcl_action_server_init( // Store reference to clock action_server->impl->clock = clock; - // Initialize Timer + // Initialize Timer, disabled by default so it doesn't start firing ret = rcl_timer_init2( &action_server->impl->expire_timer, action_server->impl->clock, node->context, - options->result_timeout.nanoseconds, NULL, allocator, true); - if (RCL_RET_OK != ret) { - goto fail; - } - // Cancel timer so it doesn't start firing - ret = rcl_timer_cancel(&action_server->impl->expire_timer); + options->result_timeout.nanoseconds, NULL, allocator, false); if (RCL_RET_OK != ret) { goto fail; } From 3b4be6d284426fe0a1e9432fbaed2ee06a5651f1 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 20 Dec 2024 14:19:29 +0000 Subject: [PATCH 08/22] Don't overwrite the error from rcl_action_goal_handle_get_info() It already sets the error, so rcl_action_server_goal_exists() should not set it again. Signed-off-by: Chris Lalancette --- rcl_action/src/rcl_action/action_server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl_action/src/rcl_action/action_server.c b/rcl_action/src/rcl_action/action_server.c index 9568d64cd..7173b5f8a 100644 --- a/rcl_action/src/rcl_action/action_server.c +++ b/rcl_action/src/rcl_action/action_server.c @@ -921,7 +921,7 @@ rcl_action_server_goal_exists( for (size_t i = 0u; i < action_server->impl->num_goal_handles; ++i) { ret = rcl_action_goal_handle_get_info(action_server->impl->goal_handles[i], &gh_goal_info); if (RCL_RET_OK != ret) { - RCL_SET_ERROR_MSG("failed to get info for goal handle"); + // error is already set return false; } // Compare UUIDs From 7d8c0cd42aba160faf009f450b24b714abd217f4 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 20 Dec 2024 14:20:03 +0000 Subject: [PATCH 09/22] Reset errors before setting new ones when checking action validity That way we avoid an ugly warning in the error paths. Signed-off-by: Chris Lalancette --- rcl_action/src/rcl_action/action_server.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/rcl_action/src/rcl_action/action_server.c b/rcl_action/src/rcl_action/action_server.c index 7173b5f8a..cac6be765 100644 --- a/rcl_action/src/rcl_action/action_server.c +++ b/rcl_action/src/rcl_action/action_server.c @@ -973,22 +973,27 @@ rcl_action_server_is_valid_except_context(const rcl_action_server_t * action_ser RCL_CHECK_FOR_NULL_WITH_MSG( action_server->impl, "action server implementation is invalid", return false); if (!rcl_service_is_valid(&action_server->impl->goal_service)) { + rcl_reset_error(); RCL_SET_ERROR_MSG("goal service is invalid"); return false; } if (!rcl_service_is_valid(&action_server->impl->cancel_service)) { + rcl_reset_error(); RCL_SET_ERROR_MSG("cancel service is invalid"); return false; } if (!rcl_service_is_valid(&action_server->impl->result_service)) { + rcl_reset_error(); RCL_SET_ERROR_MSG("result service is invalid"); return false; } if (!rcl_publisher_is_valid_except_context(&action_server->impl->feedback_publisher)) { + rcl_reset_error(); RCL_SET_ERROR_MSG("feedback publisher is invalid"); return false; } if (!rcl_publisher_is_valid_except_context(&action_server->impl->status_publisher)) { + rcl_reset_error(); RCL_SET_ERROR_MSG("status publisher is invalid"); return false; } From c1fe643a7ce72fb92babfab60d5ace656b3c0a99 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 20 Dec 2024 14:21:48 +0000 Subject: [PATCH 10/22] Move the copying of the options earlier in rcl_subscription_init. That way when we go to cleanup in the "fail" case, the options actually exist and are valid. This avoids an ugly warning during cleanup. Signed-off-by: Chris Lalancette --- rcl/src/rcl/subscription.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c index 5eb6260cd..3c05f51cc 100644 --- a/rcl/src/rcl/subscription.c +++ b/rcl/src/rcl/subscription.c @@ -100,8 +100,11 @@ rcl_subscription_init( 1, sizeof(rcl_subscription_impl_t), allocator->state); RCL_CHECK_FOR_NULL_WITH_MSG( subscription->impl, "allocating memory failed", ret = RCL_RET_BAD_ALLOC; goto cleanup); + + // options + subscription->impl->options = *options; + // Fill out the implemenation struct. - // rmw_handle // TODO(wjwwood): pass allocator once supported in rmw api. subscription->impl->rmw_handle = rmw_create_subscription( rcl_node_get_rmw_handle(node), @@ -123,8 +126,6 @@ rcl_subscription_init( } subscription->impl->actual_qos.avoid_ros_namespace_conventions = options->qos.avoid_ros_namespace_conventions; - // options - subscription->impl->options = *options; if (RCL_RET_OK != rcl_node_type_cache_register_type( node, type_support->get_type_hash_func(type_support), From 52a0c77bb29cea4ed3497d3a0bd422726ae390fe Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 20 Dec 2024 14:25:09 +0000 Subject: [PATCH 11/22] Make sure to set the error on failure of rcl_action_get_##_service_name This makes it match the generated code for the action_client. Signed-off-by: Chris Lalancette --- rcl_action/src/rcl_action/action_server.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rcl_action/src/rcl_action/action_server.c b/rcl_action/src/rcl_action/action_server.c index cac6be765..8fed25816 100644 --- a/rcl_action/src/rcl_action/action_server.c +++ b/rcl_action/src/rcl_action/action_server.c @@ -53,6 +53,8 @@ rcl_action_get_zero_initialized_server(void) char * Type ## _service_name = NULL; \ ret = rcl_action_get_ ## Type ## _service_name(action_name, allocator, &Type ## _service_name); \ if (RCL_RET_OK != ret) { \ + rcl_reset_error(); \ + RCL_SET_ERROR_MSG("failed to get " #Type " service name"); \ goto fail; \ } \ rcl_service_options_t Type ## _service_options = { \ @@ -76,6 +78,8 @@ rcl_action_get_zero_initialized_server(void) char * Type ## _topic_name = NULL; \ ret = rcl_action_get_ ## Type ## _topic_name(action_name, allocator, &Type ## _topic_name); \ if (RCL_RET_OK != ret) { \ + rcl_reset_error(); \ + RCL_SET_ERROR_MSG("failed to get " #Type " topic name"); \ goto fail; \ } \ rcl_publisher_options_t Type ## _publisher_options = { \ From 6d81f9c79b4b919254da8867e02216e718dd5b35 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 20 Dec 2024 14:25:57 +0000 Subject: [PATCH 12/22] Reset the errors during RCUTILS_FAULT_INJECTION testing. That way subsequent failures won't print out ugly error strings. Signed-off-by: Chris Lalancette --- rcl_action/test/rcl_action/test_action_server.cpp | 4 ++++ rcl_action/test/rcl_action/test_graph.cpp | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/rcl_action/test/rcl_action/test_action_server.cpp b/rcl_action/test/rcl_action/test_action_server.cpp index 675b79d30..8c0bdeaeb 100644 --- a/rcl_action/test/rcl_action/test_action_server.cpp +++ b/rcl_action/test/rcl_action/test_action_server.cpp @@ -1026,6 +1026,10 @@ TEST_F(TestActionServer, action_server_init_fini_maybe_fail) if (RCL_RET_OK == ret) { ret = rcl_action_server_fini(&action_server, &node); } + + // Always reset the error, because either rcl_action_server_init() or + // rcl_action_server_fini() may have failed above. + rcl_reset_error(); }); } diff --git a/rcl_action/test/rcl_action/test_graph.cpp b/rcl_action/test/rcl_action/test_graph.cpp index 76497857b..11d667d48 100644 --- a/rcl_action/test/rcl_action/test_graph.cpp +++ b/rcl_action/test/rcl_action/test_graph.cpp @@ -632,11 +632,11 @@ TEST_F(TestActionGraphMultiNodeFixture, action_client_init_maybe_fail) if (RCL_RET_OK == ret) { ret = rcl_action_client_fini(&action_client, &this->remote_node); - if (RCL_RET_OK != ret) { - // This isn't always set, but just in case reset anyway - rcutils_reset_error(); - } } + + // Always reset the error, because either rcl_action_client_init() or + // rcl_action_client_fini() may have failed above. + rcl_reset_error(); }); } From d46b8f485e066c90f5b7f7d77a5a1656088b80ef Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 23 Dec 2024 16:14:53 +0000 Subject: [PATCH 13/22] Make sure to return errors in _rcl_parse_resource_match . That is, if rcl_lexer_lookahead2_expect() returns an error, we should pass that along to higher layers rather than just ignoring it. Signed-off-by: Chris Lalancette --- rcl/src/rcl/arguments.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index fdbb4f9da..a9e5e6882 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -1213,10 +1213,10 @@ _rcl_parse_resource_match_token(rcl_lexer_lookahead2_t * lex_lookahead) ret = rcl_lexer_lookahead2_accept(lex_lookahead, NULL, NULL); } else if (RCL_LEXEME_WILD_ONE == lexeme) { RCL_SET_ERROR_MSG("Wildcard '*' is not implemented"); - return RCL_RET_ERROR; + ret = RCL_RET_ERROR; } else if (RCL_LEXEME_WILD_MULTI == lexeme) { RCL_SET_ERROR_MSG("Wildcard '**' is not implemented"); - return RCL_RET_ERROR; + ret = RCL_RET_ERROR; } else { RCL_SET_ERROR_MSG("Expecting token or wildcard"); ret = RCL_RET_WRONG_LEXEME; @@ -1276,6 +1276,9 @@ _rcl_parse_resource_match( ret = rcl_lexer_lookahead2_expect(lex_lookahead, RCL_LEXEME_FORWARD_SLASH, NULL, NULL); if (RCL_RET_WRONG_LEXEME == ret) { return RCL_RET_INVALID_REMAP_RULE; + } else if (RCL_RET_OK != ret) { + // Another failure + return ret; } ret = _rcl_parse_resource_match_token(lex_lookahead); if (RCL_RET_OK != ret) { From 9ed7a91ad0f8f86d7cb40ac4da7a0e025623371f Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 23 Dec 2024 17:52:21 +0000 Subject: [PATCH 14/22] Don't overwrite error by rcl_validate_enclave_name. It leads to ugly warnings. Signed-off-by: Chris Lalancette --- rcl/src/rcl/init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index f3998ee63..a42b85a38 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -235,7 +235,7 @@ rcl_init( &validation_result, &invalid_index); if (RCL_RET_OK != ret) { - RCL_SET_ERROR_MSG("rcl_validate_enclave_name() failed"); + // rcl_validate_enclave_name already set the error fail_ret = ret; goto fail; } From 89d072eb2f8fc01e50899747b0455fca97cce41e Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 23 Dec 2024 17:52:46 +0000 Subject: [PATCH 15/22] Add acomment that rmw_validate_namespace_with_size sets the error Signed-off-by: Chris Lalancette --- rcl/src/rcl/validate_enclave_name.c | 1 + 1 file changed, 1 insertion(+) diff --git a/rcl/src/rcl/validate_enclave_name.c b/rcl/src/rcl/validate_enclave_name.c index 8cd421cba..74ec04ee6 100644 --- a/rcl/src/rcl/validate_enclave_name.c +++ b/rcl/src/rcl/validate_enclave_name.c @@ -56,6 +56,7 @@ rcl_validate_enclave_name_with_size( rmw_ret_t ret = rmw_validate_namespace_with_size( enclave, enclave_length, &tmp_validation_result, &tmp_invalid_index); if (ret != RMW_RET_OK) { + // rmw_validate_namespace_with_size already set the error return rcl_convert_rmw_ret_to_rcl_ret(ret); } From 96ef397ea6676b2ef067f4e63c62e882ab15f76e Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 23 Dec 2024 18:04:52 +0000 Subject: [PATCH 16/22] Make sure to reset error in rcl_node_type_cache_init. Otherwise we get a warning about overwriting the error from rcutils_hash_map_init. Signed-off-by: Chris Lalancette --- rcl/src/rcl/node_type_cache.c | 1 + 1 file changed, 1 insertion(+) diff --git a/rcl/src/rcl/node_type_cache.c b/rcl/src/rcl/node_type_cache.c index b453f9718..120076bab 100644 --- a/rcl/src/rcl/node_type_cache.c +++ b/rcl/src/rcl/node_type_cache.c @@ -59,6 +59,7 @@ rcl_ret_t rcl_node_type_cache_init(rcl_node_t * node) &node->context->impl->allocator); if (RCUTILS_RET_OK != ret) { + rcl_reset_error(); RCL_SET_ERROR_MSG("Failed to initialize type cache hash map"); return RCL_RET_ERROR; } From 4d3fcaaeef8a92cd82e1e1e188162f296eb5dfac Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 23 Dec 2024 18:08:42 +0000 Subject: [PATCH 17/22] Conditionally set error message in rcl_publisher_is_valid. Only when rcl_context_is_valid doesn't set the error. Signed-off-by: Chris Lalancette --- rcl/src/rcl/publisher.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/rcl/src/rcl/publisher.c b/rcl/src/rcl/publisher.c index acd0df370..d8bc31d28 100644 --- a/rcl/src/rcl/publisher.c +++ b/rcl/src/rcl/publisher.c @@ -421,7 +421,11 @@ rcl_publisher_is_valid(const rcl_publisher_t * publisher) return false; // error already set } if (!rcl_context_is_valid(publisher->impl->context)) { - RCL_SET_ERROR_MSG("publisher's context is invalid"); + if (!rcl_error_is_set()) { + // rcl_context_is_valid can return false both in the error case, and when the context + // hasn't been initialized. It will only set the error message in the first case. + RCL_SET_ERROR_MSG("publisher's context is invalid"); + } return false; } RCL_CHECK_FOR_NULL_WITH_MSG( From 19a61c9bf75287d87388a74d5c3bae7513d3fedb Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 23 Dec 2024 18:10:44 +0000 Subject: [PATCH 18/22] Don't overwrite error from rcl_node_get_logger_name. It already sets the error in the failure case. Signed-off-by: Chris Lalancette --- rcl/src/rcl/logging_rosout.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl/src/rcl/logging_rosout.c b/rcl/src/rcl/logging_rosout.c index 760ca3b14..72cb0106e 100644 --- a/rcl/src/rcl/logging_rosout.c +++ b/rcl/src/rcl/logging_rosout.c @@ -213,7 +213,7 @@ rcl_ret_t rcl_logging_rosout_init_publisher_for_node(rcl_node_t * node) RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_NODE_INVALID); logger_name = rcl_node_get_logger_name(node); if (NULL == logger_name) { - RCL_SET_ERROR_MSG("Logger name was null."); + // rcl_node_get_logger_name already set the error return RCL_RET_ERROR; } if (rcutils_hash_map_key_exists(&__logger_map, &logger_name)) { From 97124e307223b77e0abe4e938ff0b40ff919aeba Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 23 Dec 2024 18:26:26 +0000 Subject: [PATCH 19/22] Make sure to reset errors when testing network flow endpoints. That's because some of the RMW implementations may not support this feature, and thus set errors. Signed-off-by: Chris Lalancette --- rcl/test/rcl/test_network_flow_endpoints.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rcl/test/rcl/test_network_flow_endpoints.cpp b/rcl/test/rcl/test_network_flow_endpoints.cpp index 857a4516c..5c6d2d8c8 100644 --- a/rcl/test/rcl/test_network_flow_endpoints.cpp +++ b/rcl/test/rcl/test_network_flow_endpoints.cpp @@ -247,6 +247,7 @@ TEST_F(TestPublisherNetworkFlowEndpoints, test_publisher_get_network_flow_endpoi ret_1 = rcl_publisher_get_network_flow_endpoints( &this->publisher_1, &allocator, &network_flow_endpoint_array_1); EXPECT_TRUE(ret_1 == RCL_RET_OK || ret_1 == RCL_RET_UNSUPPORTED); + rcl_reset_error(); // Get network flow endpoints of publisher with unique network flow endpoints rcl_network_flow_endpoint_array_t network_flow_endpoint_array_2 = @@ -259,6 +260,7 @@ TEST_F(TestPublisherNetworkFlowEndpoints, test_publisher_get_network_flow_endpoi ret_2 = rcl_publisher_get_network_flow_endpoints( &this->publisher_2, &allocator, &network_flow_endpoint_array_2); EXPECT_TRUE(ret_2 == RCL_RET_OK || ret_2 == RCL_RET_UNSUPPORTED); + rcl_reset_error(); } else { ret_2 = RCL_RET_ERROR; } @@ -348,6 +350,7 @@ TEST_F(TestSubscriptionNetworkFlowEndpoints, test_subscription_get_network_flow_ ret_1 = rcl_subscription_get_network_flow_endpoints( &this->subscription_1, &allocator, &network_flow_endpoint_array_1); EXPECT_TRUE(ret_1 == RCL_RET_OK || ret_1 == RCL_RET_UNSUPPORTED); + rcl_reset_error(); // Get network flow endpoints of subscription with unique network flow endpoints rcl_network_flow_endpoint_array_t network_flow_endpoint_array_2 = @@ -358,6 +361,7 @@ TEST_F(TestSubscriptionNetworkFlowEndpoints, test_subscription_get_network_flow_ ret_2 = rcl_subscription_get_network_flow_endpoints( &this->subscription_2, &allocator, &network_flow_endpoint_array_2); EXPECT_TRUE(ret_2 == RCL_RET_OK || ret_2 == RCL_RET_UNSUPPORTED); + rcl_reset_error(); } else { ret_2 = RCL_RET_ERROR; } From 2d39a3e70ce837bf71544ae51ecd563b9be4e3c4 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 23 Dec 2024 18:29:54 +0000 Subject: [PATCH 20/22] Make sure to reset errors in rcl_expand_topic_name. That way we can set more useful errors for the upper layers. Signed-off-by: Chris Lalancette --- rcl/src/rcl/expand_topic_name.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rcl/src/rcl/expand_topic_name.c b/rcl/src/rcl/expand_topic_name.c index ace1bcde0..d5c11999d 100644 --- a/rcl/src/rcl/expand_topic_name.c +++ b/rcl/src/rcl/expand_topic_name.c @@ -104,6 +104,7 @@ rcl_expand_topic_name( *output_topic_name = rcutils_strdup(input_topic_name, allocator); if (!*output_topic_name) { *output_topic_name = NULL; + rcl_reset_error(); RCL_SET_ERROR_MSG("failed to allocate memory for output topic"); return RCL_RET_BAD_ALLOC; } @@ -176,6 +177,7 @@ rcl_expand_topic_name( rcutils_strndup(next_opening_brace, substitution_substr_len, allocator); if (!next_substitution) { *output_topic_name = NULL; + rcl_reset_error(); RCL_SET_ERROR_MSG("failed to allocate memory for substitution"); allocator.deallocate(local_output, allocator.state); return RCL_RET_BAD_ALLOC; @@ -186,6 +188,7 @@ rcl_expand_topic_name( allocator.deallocate(original_local_output, allocator.state); // free no matter what if (!local_output) { *output_topic_name = NULL; + rcl_reset_error(); RCL_SET_ERROR_MSG("failed to allocate memory for expanded topic"); return RCL_RET_BAD_ALLOC; } From 7fb850b969f2740ede866b25db38c936c3725217 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 23 Dec 2024 19:45:00 +0000 Subject: [PATCH 21/22] Cleanup wait.c error handling. In particular, make sure to not overwrite errors as we get into error-handling paths, which should clean up warnings we get. Signed-off-by: Chris Lalancette --- rcl/src/rcl/wait.c | 46 +++++++++++++++++++------------------- rcl/test/rcl/test_wait.cpp | 3 +-- 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/rcl/src/rcl/wait.c b/rcl/src/rcl/wait.c index 40d258710..145c1eae9 100644 --- a/rcl/src/rcl/wait.c +++ b/rcl/src/rcl/wait.c @@ -74,18 +74,6 @@ rcl_wait_set_is_valid(const rcl_wait_set_t * wait_set) return wait_set && wait_set->impl; } -static void -__wait_set_clean_up(rcl_wait_set_t * wait_set) -{ - rcl_ret_t ret = rcl_wait_set_resize(wait_set, 0, 0, 0, 0, 0, 0); - (void)ret; // NO LINT - assert(RCL_RET_OK == ret); // Defensive, shouldn't fail with size 0. - if (wait_set->impl) { - wait_set->impl->allocator.deallocate(wait_set->impl, wait_set->impl->allocator.state); - wait_set->impl = NULL; - } -} - rcl_ret_t rcl_wait_set_init( rcl_wait_set_t * wait_set, @@ -103,7 +91,7 @@ rcl_wait_set_init( "'%zu' subscriptions, '%zu' guard conditions, '%zu' timers, '%zu' clients, '%zu' services", number_of_subscriptions, number_of_guard_conditions, number_of_timers, number_of_clients, number_of_services); - rcl_ret_t fail_ret = RCL_RET_ERROR; + rcl_ret_t rcl_ret = RCL_RET_ERROR; RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "invalid allocator", return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(wait_set, RCL_RET_INVALID_ARGUMENT); @@ -149,27 +137,30 @@ rcl_wait_set_init( wait_set->impl->rmw_wait_set = rmw_create_wait_set(&(context->impl->rmw_context), num_conditions); if (!wait_set->impl->rmw_wait_set) { + rcl_ret = RCL_RET_BAD_ALLOC; goto fail; } // Initialize subscription space. - rcl_ret_t ret = rcl_wait_set_resize( + rcl_ret = rcl_wait_set_resize( wait_set, number_of_subscriptions, number_of_guard_conditions, number_of_timers, number_of_clients, number_of_services, number_of_events); - if (RCL_RET_OK != ret) { - fail_ret = ret; + if (RCL_RET_OK != rcl_ret) { goto fail; } return RCL_RET_OK; + fail: - if (rcl_wait_set_is_valid(wait_set)) { - rmw_ret_t ret = rmw_destroy_wait_set(wait_set->impl->rmw_wait_set); - if (ret != RMW_RET_OK) { - fail_ret = RCL_RET_WAIT_SET_INVALID; + if (wait_set->impl->rmw_wait_set != NULL) { + rmw_ret_t rmw_ret = rmw_destroy_wait_set(wait_set->impl->rmw_wait_set); + if (rmw_ret != RMW_RET_OK) { + rcl_ret = RCL_RET_WAIT_SET_INVALID; } } - __wait_set_clean_up(wait_set); - return fail_ret; + allocator.deallocate(wait_set->impl, wait_set->impl->allocator.state); + wait_set->impl = NULL; + + return rcl_ret; } rcl_ret_t @@ -184,7 +175,16 @@ rcl_wait_set_fini(rcl_wait_set_t * wait_set) RCL_SET_ERROR_MSG(rmw_get_error_string().str); result = RCL_RET_WAIT_SET_INVALID; } - __wait_set_clean_up(wait_set); + + rcl_ret_t resize_result = rcl_wait_set_resize(wait_set, 0, 0, 0, 0, 0, 0); + if (result == RCL_RET_OK) { + // Only return the error here if we had no earlier errors. + result = resize_result; + } + if (wait_set->impl) { + wait_set->impl->allocator.deallocate(wait_set->impl, wait_set->impl->allocator.state); + wait_set->impl = NULL; + } } return result; } diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index f0b7a3f96..841a0fec1 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -850,8 +850,7 @@ TEST_F(WaitSetTestFixture, wait_set_failed_init) { "lib:rcl", rmw_create_wait_set, nullptr); rcl_ret_t ret = rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 0, context_ptr, rcl_get_default_allocator()); - EXPECT_EQ(RCL_RET_WAIT_SET_INVALID, ret); - EXPECT_TRUE(rcl_error_is_set()); + EXPECT_EQ(RCL_RET_BAD_ALLOC, ret); rcl_reset_error(); } From 8c3817f52923a13f3d92c861ef6015248fa0fdb3 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 24 Dec 2024 16:08:01 +0000 Subject: [PATCH 22/22] Make sure to reset errors in rcl_lifecycle tests. That way we won't get ugly "overwritten" warnings on subsequent tests. Signed-off-by: Chris Lalancette --- rcl_lifecycle/src/transition_map.c | 2 +- rcl_lifecycle/test/test_default_state_machine.cpp | 2 ++ rcl_lifecycle/test/test_rcl_lifecycle.cpp | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/rcl_lifecycle/src/transition_map.c b/rcl_lifecycle/src/transition_map.c index e21c5c555..04ea24595 100644 --- a/rcl_lifecycle/src/transition_map.c +++ b/rcl_lifecycle/src/transition_map.c @@ -107,7 +107,7 @@ rcl_lifecycle_register_state( new_states_size * sizeof(rcl_lifecycle_state_t), allocator->state); RCL_CHECK_FOR_NULL_WITH_MSG( - new_states, "failed to reallocate memory for new states\n", return RCL_RET_BAD_ALLOC); + new_states, "failed to reallocate memory for new states", return RCL_RET_BAD_ALLOC); transition_map->states_size = new_states_size; transition_map->states = new_states; transition_map->states[transition_map->states_size - 1] = state; diff --git a/rcl_lifecycle/test/test_default_state_machine.cpp b/rcl_lifecycle/test/test_default_state_machine.cpp index 58f61b2e6..ce80e340c 100644 --- a/rcl_lifecycle/test/test_default_state_machine.cpp +++ b/rcl_lifecycle/test/test_default_state_machine.cpp @@ -879,5 +879,7 @@ TEST_F(TestDefaultStateMachine, init_fini_maybe_fail) { rcl_lifecycle_state_machine_fini(&sm, this->node_ptr)); } } + + rcl_reset_error(); }); } diff --git a/rcl_lifecycle/test/test_rcl_lifecycle.cpp b/rcl_lifecycle/test/test_rcl_lifecycle.cpp index 72cc66259..bcb5126de 100644 --- a/rcl_lifecycle/test/test_rcl_lifecycle.cpp +++ b/rcl_lifecycle/test/test_rcl_lifecycle.cpp @@ -549,5 +549,7 @@ TEST(TestRclLifecycle, init_fini_maybe_fail) { EXPECT_EQ(RCL_RET_OK, rcl_lifecycle_state_machine_fini(&sm, &node)); } } + + rcl_reset_error(); }); }