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

Do not crash Executor when send_response fails due to client failure. #2215

Closed
wants to merge 2 commits into from
Closed

Do not crash Executor when send_response fails due to client failure. #2215

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e98dcc5
Do not crash Executor when send_response fails due to client failure.
kghost Jun 16, 2023
4d1f18c
Update rclcpp/include/rclcpp/service.hpp
kghost Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions rclcpp/include/rclcpp/service.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,14 @@ class Service
{
rcl_ret_t ret = rcl_send_response(get_service_handle().get(), &req_id, &response);

if (ret == RCL_RET_TIMEOUT) {
RCLCPP_WARN(
rclcpp::get_node_logger(node_handle_.get()).get_child("rclcpp"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not using node_logger_ here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that there is access to the node logger in this class right now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a protected member of ServiceBase

rclcpp/rclcpp/include/rclcpp/service.hpp

Line 278 in 4d1f18c

rclcpp::Logger node_logger_;

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good call. In that case, yeah, we should use that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kghost can you address this one? and then i think we are good to go.

"failed to send response (timeout): %s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to add the service name in the log message

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the problem with trying to do that here is that not all of the constructors have access to the service name. So this may not be possible to do without changing all of the constructors, or exposing service_name in rcl_service_t. Personally, I think this is nice but I wouldn't hold up this PR for that.

rcl_get_error_string().str);
rcl_reset_error();
return;
}
if (ret != RCL_RET_OK) {
rclcpp::exceptions::throw_from_rcl_error(ret, "failed to send response");
}
Expand Down