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

Avoid setting error message twice. #887

Merged
merged 2 commits into from
Jan 28, 2021

Conversation

iuhilnehc-ynos
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos commented Jan 26, 2021

Just a minor fix.

$ ./build/rcl_action/test_action_client
...

>>> [rcutils|error_handling.c:108] rcutils_set_error_state()
This error state is being overwritten:

  'failed to allocate memory for action goal service name, at /home/chenlh/Projects/ROS2/ros2-master/src/ros2/rcl/rcl_action/src/rcl_action/names.c:46'

with this new error message:

  'failed to get goal service name, at /home/chenlh/Projects/ROS2/ros2-master/src/ros2/rcl/rcl_action/src/rcl_action/action_client.c:215'

rcutils_reset_error() should be called after error handling to avoid this.
...

There exist two methods to fix,

  1. call rcl_reset_error(); before RCL_SET_ERROR_MSG. (the old error message LOST)
  2. use RCUTILS_SAFE_FWRITE_TO_STDERR instead of RCL_SET_ERROR_MSG if error already set. (Current PR)

Signed-off-by: Chen Lihui Lihui.Chen@sony.com

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>
@iuhilnehc-ynos
Copy link
Collaborator Author

iuhilnehc-ynos commented Jan 26, 2021

CI with

ros2.repos
--packages-up-to rcl_action
--packages-select rcl_action

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@iuhilnehc-ynos
Copy link
Collaborator Author

I found there are three ways to deal with an error message.

  1. call rcl_reset_error() to ignore the root error message

if (!rcl_service_is_valid(&action_server->impl->goal_service)) {
rcl_reset_error();
RCL_SET_ERROR_MSG("goal service is invalid");

  1. not to call rcl_reset_error(), just call RCUTILS_SAFE_FWRITE_TO_STDERR

rcl/rcl/src/rcl/publisher.c

Lines 144 to 147 in 2883ce4

if (RMW_RET_OK != rmw_fail_ret) {
RCUTILS_SAFE_FWRITE_TO_STDERR(rmw_get_error_string().str);
RCUTILS_SAFE_FWRITE_TO_STDERR("\n");
}

  1. use RCUTILS_SAFE_FWRITE_TO_STDERR to encapsulate the rcl_get_error_string().str, and then rcl_reset_error()

rcl/rcl/src/rcl/logging.c

Lines 213 to 216 in 2883ce4

RCUTILS_SAFE_FWRITE_TO_STDERR("failed to finalize char array: ");
RCUTILS_SAFE_FWRITE_TO_STDERR(rcl_get_error_string().str);
rcl_reset_error();
RCUTILS_SAFE_FWRITE_TO_STDERR("\n");

Is there a specific rule?

@iuhilnehc-ynos
Copy link
Collaborator Author

I will update action_client to keep the same processing about RCL_SET_ERROR_MSG consistent with action_server.

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Fine by me with green CI.

@iuhilnehc-ynos iuhilnehc-ynos merged commit 7a25a74 into ros2:master Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants