Skip to content

Yield rcl_context_fini() error codes. #763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hidmic merged 2 commits into from
Aug 25, 2020
Merged

Yield rcl_context_fini() error codes. #763

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b081873
Yield rcl_context_fini() error codes.
hidmic Aug 25, 2020
6f3cab1
Address peer review comments.
hidmic Aug 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions rcl/src/rcl/context.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ extern "C"

#include <stdbool.h>

#include "./common.h"
#include "./context_impl.h"
#include "rcutils/stdatomic_helper.h"

Expand Down Expand Up @@ -56,8 +57,7 @@ rcl_context_fini(rcl_context_t * context)
}
RCL_CHECK_ALLOCATOR_WITH_MSG(
&(context->impl->allocator), "invalid allocator", return RCL_RET_INVALID_ARGUMENT);
__cleanup_context(context);
return RCL_RET_OK;
return __cleanup_context(context);
}

// See `rcl_shutdown()` for invalidation of the context.
Expand Down Expand Up @@ -103,9 +103,10 @@ rcl_context_get_rmw_context(rcl_context_t * context)
return &(context->impl->rmw_context);
}

void
rcl_ret_t
__cleanup_context(rcl_context_t * context)
{
rcl_ret_t ret = RCL_RET_OK;
Copy link
Contributor

@brawner brawner Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces a new variable that will cause scoped variables to shadow this one. See line 115.

Copy link
Contributor Author

@hidmic hidmic Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I accidentally omitted line 115.

Copy link
Contributor Author

@hidmic hidmic Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 6f3cab1,

// reset the instance id to 0 to indicate "invalid" (should already be 0, but this is defensive)
rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), 0);

Expand All @@ -129,8 +130,11 @@ __cleanup_context(rcl_context_t * context)

// finalize init options if valid
if (NULL != context->impl->init_options.impl) {
rcl_ret_t ret = rcl_init_options_fini(&(context->impl->init_options));
if (RCL_RET_OK != ret) {
rcl_ret_t inner_ret = rcl_init_options_fini(&(context->impl->init_options));
Copy link
Contributor

@brawner brawner Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of inner_ret maybe init_options_fini_ret

Copy link
Contributor Author

@hidmic hidmic Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. See 6f3cab1.

if (RCL_RET_OK != inner_ret) {
if (RCL_RET_OK == ret) {
ret = inner_ret;
}
RCUTILS_SAFE_FWRITE_TO_STDERR(
"[rcl|context.c:" RCUTILS_STRINGIFY(__LINE__)
"] failed to finalize init options while cleaning up context, memory may be leaked: ");
Expand All @@ -144,6 +148,9 @@ __cleanup_context(rcl_context_t * context)
if (NULL != context->impl->rmw_context.implementation_identifier) {
rmw_ret_t rmw_ret = rmw_context_fini(&(context->impl->rmw_context));
if (RMW_RET_OK != rmw_ret) {
if (RCL_RET_OK == ret) {
ret = rcl_convert_rmw_ret_to_rcl_ret(rmw_ret);
}
RCUTILS_SAFE_FWRITE_TO_STDERR(
"[rcl|context.c:" RCUTILS_STRINGIFY(__LINE__)
"] failed to finalize rmw context while cleaning up context, memory may be leaked: ");
Expand All @@ -168,6 +175,8 @@ __cleanup_context(rcl_context_t * context)

// zero-initialize the context
*context = rcl_get_zero_initialized_context();

return ret;
}

#ifdef __cplusplus
Expand Down
2 changes: 1 addition & 1 deletion rcl/src/rcl/context_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ typedef struct rcl_context_impl_t
} rcl_context_impl_t;

RCL_LOCAL
void
rcl_ret_t
__cleanup_context(rcl_context_t * context);

#ifdef __cplusplus
Expand Down