Skip to content
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.

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

RCL overwriting error state after rmw_create_node #983

Open
allsey87 opened this issue May 13, 2022 · 2 comments
Open

RCL overwriting error state after rmw_create_node #983

allsey87 opened this issue May 13, 2022 · 2 comments
Labels
backlog bug Something isn't working

Comments

@allsey87
Copy link

allsey87 commented May 13, 2022

Bug report

Required Info:

  • Operating System: WebAssembly
  • Installation type: Source
  • Version or commit hash: Galactic
  • DDS implementation: Custom
  • Client library (if applicable): rclcpp

Expected behavior

RCL should fail gracefully when rmw_create_node returns NULL and sets the error state via RMW_SET_ERROR_MSG

Actual behavior

RCL ignores the fact the rmw_create_node set an error and returned NULL and then proceeds to overwrite the error state as follows:

failed to initialize rcl node: rcl node's rmw handle is invalid, at /home/developer/workspace/src/ros2/rcl/rcl/src/rcl/node.c:414
>>> [rcutils|error_handling.c:108] rcutils_set_error_state()
This error state is being overwritten:
'RMW_ERROR_MESSAGE, at RMW_FILENAME:0, at /home/developer/workspace/src/ros2/rcl/rcl/src/rcl/node.c:262'
with this new error message:
'rcl node's rmw handle is invalid, at /home/developer/workspace/src/ros2/rcl/rcl/src/rcl/node.c:414'
rcutils_reset_error() should be called after error handling to avoid this.
<<<

[ERROR] [0000000003.694284999] [rcl]: Failed to fini publisher for node: 1
@fujitatomoya
Copy link
Collaborator

i think this is bug.

rcl/rcl/src/rcl/node.c

Lines 251 to 261 in 493ebf3

// node logger name
node->impl->logger_name = rcl_create_node_logger_name(name, local_namespace_, allocator);
RCL_CHECK_FOR_NULL_WITH_MSG(
node->impl->logger_name, "creating logger name failed", goto fail);
RCUTILS_LOG_DEBUG_NAMED(
ROS_PACKAGE_NAME, "Using domain ID of '%zu'", context->impl->rmw_context.actual_domain_id);
node->impl->rmw_node_handle = rmw_create_node(
&(node->context->impl->rmw_context),
name, local_namespace_);

logger_name is assigned before rmw_create_node above. and if rmw_create_node fails, it goes to

rcl/rcl/src/rcl/node.c

Lines 305 to 311 in 493ebf3

fail:
if (node->impl) {
if (rcl_logging_rosout_enabled() &&
node->impl->options.enable_rosout &&
node->impl->logger_name)
{
ret = rcl_logging_rosout_fini_publisher_for_node(node);

and call rcl_logging_rosout_fini_publisher_for_node w/o calling rcl_logging_rosout_init_publisher_for_node.

this leads to call rcl_node_is_valid_except_context to overwrite the error message.

@fujitatomoya
Copy link
Collaborator

#985 can fix the problem, but according to #985 (review), it would be nice to refactor the rcl_node_init and rcl_node_fini process to fix the complication and possible problem in the future.

@fujitatomoya fujitatomoya added the enhancement New feature or request label May 17, 2022
@clalancette clalancette added backlog and removed enhancement New feature or request labels Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants