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.

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

Jump to bottom

Correctly clean up arguments structure. #459

Merged
merged 5 commits into from
Apr 11, 2018
Merged

Correctly clean up arguments structure. #459

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b0ee260
Correctly clean up arguments structure.
mjcarroll Apr 4, 2018
f6f5794
Use zero-initialized structure.
mjcarroll Apr 5, 2018
f284100
Avoid repeated cleanup.
mjcarroll Apr 5, 2018
c960ce2
Address reviewer comments.
mjcarroll Apr 6, 2018
d714b38
Adress reviewer feedback on naming.
mjcarroll Apr 6, 2018
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
27 changes: 25 additions & 2 deletions rclcpp/src/rclcpp/utilities.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,16 @@ rclcpp::remove_ros_arguments(int argc, char const * const argv[])

ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
if (RCL_RET_OK != ret) {
exceptions::throw_from_rcl_error(ret, "Failed to parse arguments");
// Not using throw_from_rcl_error, because we may need to append deallocation failures.
exceptions::RCLErrorBase base_exec(ret, rcl_get_error_state());
rcl_reset_error();
if (RCL_RET_OK != rcl_arguments_fini(&parsed_args)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like rcl_parse_arguments cleans up after itself when something goes wrong. I would expect this call to rcl_arguments_fini to always fail here.

base_exec.formatted_message += std::string(
", failed also to cleanup parsed arguments, leaking memory: ") +
rcl_get_error_string_safe();
rcl_reset_error();
}
throw base_exec;
}

int nonros_argc = 0;
Expand All @@ -224,10 +233,19 @@ rclcpp::remove_ros_arguments(int argc, char const * const argv[])
&nonros_argv);

if (RCL_RET_OK != ret) {
// Not using throw_from_rcl_error, because we may need to append deallocation failures.
exceptions::RCLErrorBase base_exec(ret, rcl_get_error_state());
Copy link
Member

Choose a reason for hiding this comment

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

unless it's a convention that I haven't seen before, exec doesn't seem like an appropriate variable name (base_exc instead?)

rcl_reset_error();
if (NULL != nonros_argv) {
alloc.deallocate(nonros_argv, alloc.state);
}
exceptions::throw_from_rcl_error(ret, "Failed to remove ROS arguments: ");
if (RCL_RET_OK != rcl_arguments_fini(&parsed_args)) {
base_exec.formatted_message += std::string(
", failed also to cleanup parsed arguments, leaking memory: ") +
rcl_get_error_string_safe();
rcl_reset_error();
}
throw base_exec;
Copy link
Member

Choose a reason for hiding this comment

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

I think our default behaviour is to throw an RCLError not an RCLErrorBase. I would change this to

throw RCLError(base_exc, "");

following
https://github.com/ros2/rclcpp/blob/c70f2f145239ca4dfe86c6d806f331602a0e5c68/rclcpp/src/rclcpp/exceptions.cpp#L71..L72

}

std::vector<std::string> return_arguments;
Expand All @@ -241,6 +259,11 @@ rclcpp::remove_ros_arguments(int argc, char const * const argv[])
alloc.deallocate(nonros_argv, alloc.state);
}

ret = rcl_arguments_fini(&parsed_args);
if (RCL_RET_OK != ret) {
exceptions::throw_from_rcl_error(ret, "failed to cleanup parsed arguments, leaking memory: ");
Copy link
Member

Choose a reason for hiding this comment

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

Is the : needed in this case? I think this adds it automatically:

rclcpp/rclcpp/src/rclcpp/exceptions.cpp

Line 60 in 45dcd0c

formated_prefix += ": ";

}

return return_arguments;
}

Expand Down