-
Notifications
You must be signed in to change notification settings - Fork 406
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
Do not crash Executor when send_response fails due to client failure. #2215
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"), | ||
"failed to send response (timeout): %s", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest to add the service name in the log message There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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"); | ||
} | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.