From 43dfecc4bccc5789429752cd2264e0f726e97329 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 26 Oct 2023 20:30:16 +0000 Subject: [PATCH 1/5] Make the fini of the type description service explicit This makes the caller responsible for cleaning up the service. Signed-off-by: Michael Carroll --- rcl/include/rcl/node.h | 6 ++++++ rcl/src/rcl/node.c | 8 +------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/rcl/include/rcl/node.h b/rcl/include/rcl/node.h index e0213b7de..5d6878807 100644 --- a/rcl/include/rcl/node.h +++ b/rcl/include/rcl/node.h @@ -562,6 +562,9 @@ rcl_get_disable_loaned_message(bool * disable_loaned_message); * must register rcl_node_type_description_service_handle_request or a custom callback * to handle incoming requests, via that client's executor/waitset capabilities. * + * Note that the underlying service must be cleaned up by the caller by calling + * rcl_node_get_type_description_service and rcl_service_fini. + * * This will initialize the node's type cache, if it has not been initialized already. * *
@@ -612,6 +615,9 @@ rcl_ret_t rcl_node_type_description_service_fini(rcl_node_t * node); * On success, sets service_out to the initialized service. * rcl_node_type_description_service_init must be called before this. * + * Note the caller of this function is responsible for calling + * rcl_service_fini on the returned service. + * *
* Attribute | Adherence * ------------------ | ------------- diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index 35f995e42..3b9d1a057 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -628,13 +628,7 @@ rcl_ret_t rcl_node_type_description_service_fini(rcl_node_t * node) return RCL_RET_NOT_INIT; } - const rcl_ret_t ret = - rcl_service_fini(&node->impl->get_type_description_service, node); - if (RCL_RET_OK == ret) { - node->impl->get_type_description_service = rcl_get_zero_initialized_service(); - } - - return ret; + return RCL_RET_OK; } rcl_ret_t rcl_node_get_type_description_service( From 3d215ebdb8c03806792cdb2156cecd2e8fe295fc Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 27 Oct 2023 17:32:51 +0000 Subject: [PATCH 2/5] Remove fini and get methods Signed-off-by: Michael Carroll --- rcl/include/rcl/node.h | 64 +++-------------------------------------- rcl/src/rcl/node.c | 41 ++++---------------------- rcl/src/rcl/node_impl.h | 1 - 3 files changed, 10 insertions(+), 96 deletions(-) diff --git a/rcl/include/rcl/node.h b/rcl/include/rcl/node.h index 5d6878807..c75302f2d 100644 --- a/rcl/include/rcl/node.h +++ b/rcl/include/rcl/node.h @@ -562,10 +562,8 @@ rcl_get_disable_loaned_message(bool * disable_loaned_message); * must register rcl_node_type_description_service_handle_request or a custom callback * to handle incoming requests, via that client's executor/waitset capabilities. * - * Note that the underlying service must be cleaned up by the caller by calling - * rcl_node_get_type_description_service and rcl_service_fini. - * - * This will initialize the node's type cache, if it has not been initialized already. + * Note that the returned service must be cleaned up by the caller by calling + * rcl_service_fini. * *
* Attribute | Adherence @@ -584,62 +582,8 @@ rcl_get_disable_loaned_message(bool * disable_loaned_message); */ RCL_PUBLIC RCL_WARN_UNUSED -rcl_ret_t rcl_node_type_description_service_init(rcl_node_t * node); - -/// Finalizes the node's ~/get_type_description service. -/** - * This function finalizes the node's private ~/get_type_description service. - * - *
- * Attribute | Adherence - * ------------------ | ------------- - * Allocates Memory | No - * Thread-Safe | No - * Uses Atomics | No - * Lock-Free | Yes - * - * \param[in] node the handle to the node whose type cache should be initialized - * \return #RCL_RET_OK if service was deinitialized successfully, or - * \return #RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or - * \return #RCL_RET_SERVICE_INVALID if the service is invalid, or - * \return #RCL_RET_NODE_INVALID if the node is invalid, or - * \return #RCL_RET_ERROR if an unspecified error occurs. - */ -RCL_PUBLIC -RCL_WARN_UNUSED -rcl_ret_t rcl_node_type_description_service_fini(rcl_node_t * node); - - -/// Returns a pointer to the node's ~/get_type_description service. -/** - * On success, sets service_out to the initialized service. - * rcl_node_type_description_service_init must be called before this. - * - * Note the caller of this function is responsible for calling - * rcl_service_fini on the returned service. - * - *
- * Attribute | Adherence - * ------------------ | ------------- - * Allocates Memory | No - * Thread-Safe | No - * Uses Atomics | No - * Lock-Free | Yes - * - * \param[in] node the handle to the node - * \param[out] service_out Handle to pointer that will be set - * \return #RCL_RET_OK if valid service was returned successfully, or - * \return #RCL_RET_NODE_INVALID if node is invalid, or - * \return #RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or - * \return #RCL_RET_NOT_INIT if the service hasn't yet been initialized, or - * \return #RCL_RET_ERROR if an unspecified error occurs. - */ -RCL_PUBLIC -RCL_WARN_UNUSED -rcl_ret_t rcl_node_get_type_description_service( - const rcl_node_t * node, - rcl_service_t ** service_out); - +rcl_ret_t rcl_node_type_description_service_init(rcl_service_t * service, + const rcl_node_t * node); /// Process a single pending request to the GetTypeDescription service. /** diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index 3b9d1a057..14229a2f2 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -194,7 +194,6 @@ rcl_node_init( node->impl, "allocating memory failed", ret = RCL_RET_BAD_ALLOC; goto fail); node->impl->options = rcl_node_get_default_options(); node->impl->registered_types_by_type_hash = rcutils_get_zero_initialized_hash_map(); - node->impl->get_type_description_service = rcl_get_zero_initialized_service(); node->context = context; // Initialize node impl. ret = rcl_node_options_copy(options, &(node->impl->options)); @@ -580,14 +579,14 @@ void rcl_node_type_description_service_handle_request( response->successful = true; } -rcl_ret_t rcl_node_type_description_service_init(rcl_node_t * node) +rcl_ret_t rcl_node_type_description_service_init(rcl_service_t * service, + const rcl_node_t * node) { + RCL_CHECK_ARGUMENT_FOR_NULL(service, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(node->impl, RCL_RET_NODE_INVALID); - rcl_ret_t ret; - - if (rcl_service_is_valid(&node->impl->get_type_description_service)) { + if (rcl_service_is_valid(service)) { return RCL_RET_ALREADY_INIT; } rcl_reset_error(); // Reset the error message set by rcl_service_is_valid() @@ -601,7 +600,7 @@ rcl_ret_t rcl_node_type_description_service_init(rcl_node_t * node) rcl_allocator_t allocator = node->context->impl->allocator; // Construct service name - ret = rcl_node_resolve_name( + rcl_ret_t ret = rcl_node_resolve_name( node, "~/get_type_description", allocator, true, true, &service_name); if (RCL_RET_OK != ret) { @@ -612,37 +611,9 @@ rcl_ret_t rcl_node_type_description_service_init(rcl_node_t * node) // Initialize service ret = rcl_service_init( - &node->impl->get_type_description_service, node, + service, node, type_support, service_name, &service_ops); allocator.deallocate(service_name, allocator.state); return ret; } - -rcl_ret_t rcl_node_type_description_service_fini(rcl_node_t * node) -{ - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(node->impl, RCL_RET_NODE_INVALID); - if (!rcl_service_is_valid(&node->impl->get_type_description_service)) { - rcl_reset_error(); - return RCL_RET_NOT_INIT; - } - - return RCL_RET_OK; -} - -rcl_ret_t rcl_node_get_type_description_service( - const rcl_node_t * node, - rcl_service_t ** service_out) -{ - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(node->impl, RCL_RET_NODE_INVALID); - RCL_CHECK_ARGUMENT_FOR_NULL(service_out, RCL_RET_SERVICE_INVALID); - - if (!rcl_service_is_valid(&node->impl->get_type_description_service)) { - return RCL_RET_NOT_INIT; - } - - *service_out = &node->impl->get_type_description_service; - return RCL_RET_OK; -} diff --git a/rcl/src/rcl/node_impl.h b/rcl/src/rcl/node_impl.h index 8d4f5847e..5445c5aab 100644 --- a/rcl/src/rcl/node_impl.h +++ b/rcl/src/rcl/node_impl.h @@ -31,7 +31,6 @@ struct rcl_node_impl_s const char * logger_name; const char * fq_name; rcutils_hash_map_t registered_types_by_type_hash; - rcl_service_t get_type_description_service; }; #endif // RCL__NODE_IMPL_H_ From 90888a2efaa027788a6bae6260e42edcc178a338 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 27 Oct 2023 18:39:18 +0000 Subject: [PATCH 3/5] Update tests Signed-off-by: Michael Carroll --- rcl/include/rcl/node.h | 5 +- rcl/src/rcl/node.c | 5 +- .../rcl/test_get_type_description_service.cpp | 53 ++++++++++++------- 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/rcl/include/rcl/node.h b/rcl/include/rcl/node.h index c75302f2d..c383707ae 100644 --- a/rcl/include/rcl/node.h +++ b/rcl/include/rcl/node.h @@ -582,8 +582,9 @@ rcl_get_disable_loaned_message(bool * disable_loaned_message); */ RCL_PUBLIC RCL_WARN_UNUSED -rcl_ret_t rcl_node_type_description_service_init(rcl_service_t * service, - const rcl_node_t * node); +rcl_ret_t rcl_node_type_description_service_init( + rcl_service_t * service, + const rcl_node_t * node); /// Process a single pending request to the GetTypeDescription service. /** diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index 14229a2f2..1d4305bf2 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -579,8 +579,9 @@ void rcl_node_type_description_service_handle_request( response->successful = true; } -rcl_ret_t rcl_node_type_description_service_init(rcl_service_t * service, - const rcl_node_t * node) +rcl_ret_t rcl_node_type_description_service_init( + rcl_service_t * service, + const rcl_node_t * node) { RCL_CHECK_ARGUMENT_FOR_NULL(service, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); diff --git a/rcl/test/rcl/test_get_type_description_service.cpp b/rcl/test/rcl/test_get_type_description_service.cpp index baac08013..2679368cd 100644 --- a/rcl/test/rcl/test_get_type_description_service.cpp +++ b/rcl/test/rcl/test_get_type_description_service.cpp @@ -163,8 +163,6 @@ class CLASSNAME (TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION) : public ::testi rcl_node_options_t node_options = rcl_node_get_default_options(); ret = rcl_node_init(this->node_ptr, name, "", this->context_ptr, &node_options); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; - ret = rcl_node_type_description_service_init(node_ptr); - ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; const char * node_fqn = rcl_node_get_fully_qualified_name(this->node_ptr); snprintf( @@ -174,9 +172,7 @@ class CLASSNAME (TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION) : public ::testi void TearDown() { - rcl_ret_t ret = rcl_node_type_description_service_fini(node_ptr); - EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; - ret = rcl_node_fini(this->node_ptr); + rcl_ret_t ret = rcl_node_fini(this->node_ptr); delete this->node_ptr; EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; ret = rcl_shutdown(this->context_ptr); @@ -197,23 +193,26 @@ class CLASSNAME (TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION) : public ::testi TEST_F( CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION), test_service_init_and_fini_functions) { + rcl_service_t service = rcl_get_zero_initialized_service(); + + // Service does not initially exist EXPECT_TRUE( - service_exists( + service_not_exists( this->node_ptr, this->get_type_description_service_name, GET_TYPE_DESCRIPTION_SRV_TYPE_NAME, std::chrono::seconds(5))); - EXPECT_EQ(RCL_RET_OK, rcl_node_type_description_service_fini(this->node_ptr)); + + EXPECT_EQ(RCL_RET_OK, rcl_node_type_description_service_init(&service, this->node_ptr)); EXPECT_TRUE( - service_not_exists( + service_exists( this->node_ptr, this->get_type_description_service_name, GET_TYPE_DESCRIPTION_SRV_TYPE_NAME, std::chrono::seconds(5))); - EXPECT_EQ(RCL_RET_NOT_INIT, rcl_node_type_description_service_fini(this->node_ptr)); - EXPECT_EQ(RCL_RET_OK, rcl_node_type_description_service_init(this->node_ptr)); + EXPECT_EQ(RCL_RET_OK, rcl_service_fini(&service, this->node_ptr)); EXPECT_TRUE( - service_exists( + service_not_exists( this->node_ptr, this->get_type_description_service_name, GET_TYPE_DESCRIPTION_SRV_TYPE_NAME, std::chrono::seconds(5))); - EXPECT_EQ(RCL_RET_ALREADY_INIT, rcl_node_type_description_service_init(this->node_ptr)); + EXPECT_EQ(RCL_RET_NOT_INIT, rcl_service_fini(&service, this->node_ptr)); } /* Basic nominal test of the ~/get_type_description service. */ @@ -222,6 +221,10 @@ TEST_F(CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION), test_service_no const rosidl_service_type_support_t * ts = ROSIDL_GET_SRV_TYPE_SUPPORT( type_description_interfaces, srv, GetTypeDescription); + // Create type description service. + auto service = rcl_get_zero_initialized_service(); + EXPECT_EQ(RCL_RET_OK, rcl_node_type_description_service_init(&service, this->node_ptr)); + // Create client. rcl_client_t client = rcl_get_zero_initialized_client(); rcl_client_options_t client_options = rcl_client_get_default_options(); @@ -233,6 +236,9 @@ TEST_F(CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION), test_service_no { rcl_ret_t ret = rcl_client_fini(&client, this->node_ptr); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + + ret = rcl_service_fini(&service, this->node_ptr); + EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; }); ASSERT_TRUE(wait_for_server_to_be_available(this->node_ptr, &client, 10, 1000)); @@ -262,8 +268,8 @@ TEST_F(CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION), test_service_no // This scope simulates handling request in a different context { - auto service = &node_ptr->impl->get_type_description_service; - ASSERT_TRUE(wait_for_service_to_be_ready(service, context_ptr, 10, 100)); + auto * service_ptr = &service; + ASSERT_TRUE(wait_for_service_to_be_ready(service_ptr, context_ptr, 10, 100)); type_description_interfaces__srv__GetTypeDescription_Response service_response; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( @@ -278,7 +284,7 @@ TEST_F(CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION), test_service_no type_description_interfaces__srv__GetTypeDescription_Request__fini(&service_request); }); rmw_service_info_t header; - ret = rcl_take_request_with_info(service, &header, &service_request); + ret = rcl_take_request_with_info(service_ptr, &header, &service_request); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; rcl_node_type_description_service_handle_request( @@ -287,7 +293,7 @@ TEST_F(CLASSNAME(TestGetTypeDescSrvFixture, RMW_IMPLEMENTATION), test_service_no &service_request, &service_response); - ret = rcl_send_response(service, &header.request_id, &service_response); + ret = rcl_send_response(service_ptr, &header.request_id, &service_response); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; } @@ -315,6 +321,10 @@ TEST_F( const rosidl_service_type_support_t * ts = ROSIDL_GET_SRV_TYPE_SUPPORT( type_description_interfaces, srv, GetTypeDescription); + // Create type description service. + auto service = rcl_get_zero_initialized_service(); + EXPECT_EQ(RCL_RET_OK, rcl_node_type_description_service_init(&service, this->node_ptr)); + // Create client. rcl_client_t client = rcl_get_zero_initialized_client(); rcl_client_options_t client_options = rcl_client_get_default_options(); @@ -326,6 +336,9 @@ TEST_F( { rcl_ret_t ret = rcl_client_fini(&client, this->node_ptr); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + + ret = rcl_service_fini(&service, this->node_ptr); + EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; }); ASSERT_TRUE(wait_for_server_to_be_available(this->node_ptr, &client, 10, 1000)); @@ -346,8 +359,8 @@ TEST_F( // This scope simulates handling request in a different context { - auto service = &node_ptr->impl->get_type_description_service; - ASSERT_TRUE(wait_for_service_to_be_ready(service, context_ptr, 10, 100)); + auto * service_ptr = &service; + ASSERT_TRUE(wait_for_service_to_be_ready(service_ptr, context_ptr, 10, 100)); type_description_interfaces__srv__GetTypeDescription_Response service_response; OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT( @@ -362,7 +375,7 @@ TEST_F( type_description_interfaces__srv__GetTypeDescription_Request__fini(&service_request); }); rmw_service_info_t header; - ret = rcl_take_request_with_info(service, &header, &service_request); + ret = rcl_take_request_with_info(service_ptr, &header, &service_request); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; rcl_node_type_description_service_handle_request( @@ -371,7 +384,7 @@ TEST_F( &service_request, &service_response); - ret = rcl_send_response(service, &header.request_id, &service_response); + ret = rcl_send_response(service_ptr, &header.request_id, &service_response); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; } From 6f7da3a16387b59e799e33463cb6daecb4de7a19 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Sat, 28 Oct 2023 02:33:24 +0000 Subject: [PATCH 4/5] Update documentation based on reviewer feedback Signed-off-by: Michael Carroll --- rcl/include/rcl/node.h | 1 + 1 file changed, 1 insertion(+) diff --git a/rcl/include/rcl/node.h b/rcl/include/rcl/node.h index c383707ae..58a2163ec 100644 --- a/rcl/include/rcl/node.h +++ b/rcl/include/rcl/node.h @@ -573,6 +573,7 @@ rcl_get_disable_loaned_message(bool * disable_loaned_message); * Uses Atomics | No * Lock-Free | Yes * + * \param[in] service the handle to the type description service to be initialized * \param[in] node handle to the node for which to initialize the service * \return #RCL_RET_OK if the service was successfully initialized, or * \return #RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or From 25174dfc4bb3079974bafe087fddb61e9647f94e Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Sat, 28 Oct 2023 02:53:36 +0000 Subject: [PATCH 5/5] Fix and document test Signed-off-by: Michael Carroll --- rcl/test/rcl/test_get_type_description_service.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/rcl/test/rcl/test_get_type_description_service.cpp b/rcl/test/rcl/test_get_type_description_service.cpp index 2679368cd..235e7937a 100644 --- a/rcl/test/rcl/test_get_type_description_service.cpp +++ b/rcl/test/rcl/test_get_type_description_service.cpp @@ -201,18 +201,22 @@ TEST_F( this->node_ptr, this->get_type_description_service_name, GET_TYPE_DESCRIPTION_SRV_TYPE_NAME, std::chrono::seconds(5))); + // Once the type descrition service is init, then it appear in the graph EXPECT_EQ(RCL_RET_OK, rcl_node_type_description_service_init(&service, this->node_ptr)); EXPECT_TRUE( service_exists( this->node_ptr, this->get_type_description_service_name, GET_TYPE_DESCRIPTION_SRV_TYPE_NAME, std::chrono::seconds(5))); + // Once the type description service is fini, then it no longer appears in the graph EXPECT_EQ(RCL_RET_OK, rcl_service_fini(&service, this->node_ptr)); EXPECT_TRUE( service_not_exists( this->node_ptr, this->get_type_description_service_name, GET_TYPE_DESCRIPTION_SRV_TYPE_NAME, std::chrono::seconds(5))); - EXPECT_EQ(RCL_RET_NOT_INIT, rcl_service_fini(&service, this->node_ptr)); + + // Repeatedly destroying the service should not cause faults. + EXPECT_EQ(RCL_RET_OK, rcl_service_fini(&service, this->node_ptr)); } /* Basic nominal test of the ~/get_type_description service. */