diff --git a/rcl_action/src/rcl_action/action_client.c b/rcl_action/src/rcl_action/action_client.c index 7551eab..58ab0a9 100644 --- a/rcl_action/src/rcl_action/action_client.c +++ b/rcl_action/src/rcl_action/action_client.c @@ -46,6 +46,58 @@ rcl_action_get_zero_initialized_client(void) return null_action_client; } +rcl_action_client_impl_t +_rcl_action_get_zero_initialized_client_impl(void) +{ + rcl_client_t null_client = rcl_get_zero_initialized_client(); + rcl_subscription_t null_subscription = rcl_get_zero_initialized_subscription(); + rcl_action_client_impl_t null_action_client = { + null_client, + null_client, + null_client, + null_subscription, + null_subscription, + rcl_action_client_get_default_options(), + NULL, + 0, + 0, + 0, + 0, + 0 + }; + return null_action_client; +} + +rcl_ret_t +_rcl_action_client_fini_impl( + rcl_action_client_t * action_client, rcl_node_t * node, rcl_allocator_t allocator) +{ + if (NULL == action_client->impl) { + return RCL_RET_OK; + } + rcl_ret_t ret = RCL_RET_OK; + if (RCL_RET_OK != rcl_client_fini(&action_client->impl->goal_client, node)) { + ret = RCL_RET_ERROR; + } + if (RCL_RET_OK != rcl_client_fini(&action_client->impl->cancel_client, node)) { + ret = RCL_RET_ERROR; + } + if (RCL_RET_OK != rcl_client_fini(&action_client->impl->result_client, node)) { + ret = RCL_RET_ERROR; + } + if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->feedback_subscription, node)) { + ret = RCL_RET_ERROR; + } + if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->status_subscription, node)) { + ret = RCL_RET_ERROR; + } + allocator.deallocate(action_client->impl->action_name, allocator.state); + allocator.deallocate(action_client->impl, allocator.state); + action_client->impl = NULL; + RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Action client finalized"); + return ret; +} + // \internal Initializes an action client specific service client. #define CLIENT_INIT(Type) \ char * Type ## _service_name = NULL; \ @@ -148,9 +200,12 @@ rcl_action_client_init( RCL_CHECK_FOR_NULL_WITH_MSG( action_client->impl, "allocating memory failed", return RCL_RET_BAD_ALLOC); + // Avoid uninitialized pointers should initialization fail + *action_client->impl = _rcl_action_get_zero_initialized_client_impl(); // Copy action client name and options. action_client->impl->action_name = rcutils_strdup(action_name, allocator); if (NULL == action_client->impl->action_name) { + RCL_SET_ERROR_MSG("failed to duplicate action name"); ret = RCL_RET_BAD_ALLOC; goto fail; } @@ -168,7 +223,7 @@ rcl_action_client_init( RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Action client initialized"); return ret; fail: - fini_ret = rcl_action_client_fini(action_client, node); + fini_ret = _rcl_action_client_fini_impl(action_client, node, allocator); if (RCL_RET_OK != fini_ret) { RCL_SET_ERROR_MSG("failed to cleanup action client"); ret = RCL_RET_ERROR; @@ -186,28 +241,8 @@ rcl_action_client_fini(rcl_action_client_t * action_client, rcl_node_t * node) if (!rcl_node_is_valid_except_context(node)) { return RCL_RET_NODE_INVALID; // error already set } - rcl_ret_t ret = RCL_RET_OK; - if (RCL_RET_OK != rcl_client_fini(&action_client->impl->goal_client, node)) { - ret = RCL_RET_ERROR; - } - if (RCL_RET_OK != rcl_client_fini(&action_client->impl->cancel_client, node)) { - ret = RCL_RET_ERROR; - } - if (RCL_RET_OK != rcl_client_fini(&action_client->impl->result_client, node)) { - ret = RCL_RET_ERROR; - } - if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->feedback_subscription, node)) { - ret = RCL_RET_ERROR; - } - if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->status_subscription, node)) { - ret = RCL_RET_ERROR; - } - rcl_allocator_t * allocator = &action_client->impl->options.allocator; - allocator->deallocate(action_client->impl->action_name, allocator->state); - allocator->deallocate(action_client->impl, allocator->state); - action_client->impl = NULL; - RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Action client finalized"); - return ret; + return + _rcl_action_client_fini_impl(action_client, node, action_client->impl->options.allocator); } rcl_action_client_options_t diff --git a/rcl_action/test/rcl_action/test_action_client.cpp b/rcl_action/test/rcl_action/test_action_client.cpp index 66e91eb..5970cfe 100644 --- a/rcl_action/test/rcl_action/test_action_client.cpp +++ b/rcl_action/test/rcl_action/test_action_client.cpp @@ -154,19 +154,33 @@ TEST_F(TestActionClientBaseFixture, test_action_client_init_fini) { EXPECT_EQ(ret, RCL_RET_BAD_ALLOC) << rcl_get_error_string().str; rcl_reset_error(); - // Fail copying action_name string, this will also fail to cleanup and will return generic error + // Fail copying action name time_bomb_state.malloc_count_until_failure = 1; invalid_action_client_options.allocator.state = &time_bomb_state; ret = rcl_action_client_init( &action_client, &this->node, action_typesupport, action_name, &invalid_action_client_options); - EXPECT_EQ(ret, RCL_RET_ERROR) << rcl_get_error_string().str; + EXPECT_EQ(ret, RCL_RET_BAD_ALLOC) << rcl_get_error_string().str; rcl_reset_error(); - // Manually cleanup to setup for next test - action_client_options.allocator.deallocate( - action_client.impl, action_client_options.allocator.state); - action_client.impl = NULL; + ret = RCL_RET_OK; + int i = 0; + do { + time_bomb_state.malloc_count_until_failure = i; + i++; + invalid_action_client_options.allocator.state = &time_bomb_state; + ret = rcl_action_client_init( + &action_client, &this->node, action_typesupport, + action_name, &invalid_action_client_options); + if (RCL_RET_OK != ret) { + EXPECT_TRUE(rcl_error_is_set()); + rcl_reset_error(); + } else { + EXPECT_EQ(RCL_RET_OK, rcl_action_client_fini(&action_client, &this->node)) << + rcl_get_error_string().str; + EXPECT_FALSE(rcl_error_is_set()); + } + } while (ret != RCL_RET_OK); ret = rcl_action_client_init( &action_client, &this->node, action_typesupport,