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

kghost · 2023-06-16T05:18:27Z

It is not sane that a faulty client can crash our service Executor, as discussed in the referred issue, if the client is not setup properly, send_response may return RCL_RET_TIMEOUT, we should not throw an error in this case.

fujitatomoya

@kghost thanks for creating PR on this issue.

rclcpp/include/rclcpp/service.hpp

fujitatomoya · 2023-06-29T18:31:29Z

@kghost thank you for being patient on this.

I will be checking the following and then start CI all together.

underlying tier 1 rmw implementation returns timeout when it detects the timeout? (unless, it will generate the exception)
rclpy counterpart PR against this rclcpp.

fujitatomoya · 2023-06-29T22:10:54Z

underlying tier 1 rmw implementation returns timeout when it detects the timeout? (unless, it will generate the exception)

see ros2/rcl#1048 (comment)

rclpy counterpart PR against this rclcpp.

see ros2/rclpy#1136

fujitatomoya · 2023-07-06T07:18:51Z

CI:

Linux
Linux-aarch64
Windows

clalancette

This needs the one change inline, and also likely needs to be rebased. Once those are done, I'm happy to approve this.

rclcpp/include/rclcpp/service.hpp

milidam · 2023-07-07T11:38:07Z

I am experiencing the same kind of crash using the fastrtps RMW implementation with ros2 humble.
However, the proposed patch does not avoid the crash, as the RMW_RET_TIMEOUT error code returned by the fastrtps' __rmw_send_response is changed to RCL_RET_ERROR in rcl_send_response.
The same is done in rolling : https://github.com/ros2/rcl/blob/rolling/rcl/src/rcl/service.c#L403

Am I missing somthing? or are there any other patches required for it to work?

clalancette · 2023-07-07T12:33:51Z

Am I missing somthing? or are there any other patches required for it to work?

Yes, you also need ros2/rcl#1048

fujitatomoya · 2023-07-08T07:08:47Z

@kghost are you willing to work on this? see #2215 (review)

kghost · 2023-07-08T09:18:45Z

@kghost are you willing to work on this? see #2215 (review)

no problem!

fujitatomoya · 2023-07-11T00:46:20Z

@kghost thank you very much for the effort, can you address DCO error? see https://github.com/ros2/rclcpp/pull/2215/checks?check_run_id=14906339355

fujitatomoya · 2023-07-11T01:41:52Z

@kghost this also need to be rebased on rolling, see https://ci.ros2.org/job/ci_linux/19079/console CI failed on building rclcpp.

kghost · 2023-07-11T01:44:59Z

@fujitatomoya Yes, it is already rebased on rolling branch.

fujitatomoya · 2023-07-11T01:46:07Z

CI:

Linux
Linux-aarch64
Windows

fujitatomoya · 2023-07-14T06:18:07Z

CI:

Linux

windows failure is unrelated.

Related to ros2/ros2#1253 It is not sane that a faulty client can crash our service Executor, as discussed in the referred issue, if the client is not setup properly, send_response may return RCL_RET_TIMEOUT, we should not throw an error in this case. Signed-off-by: Zang MingJie <zealot0630@gmail.com>

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Zang MingJie <zealot0630@gmail.com>

kghost · 2023-07-18T01:51:32Z

@clalancette This is ready to merge.

alsora · 2023-07-20T10:54:58Z

rclcpp/include/rclcpp/service.hpp

@@ -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"),


why not using node_logger_ here?

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

It's a protected member of ServiceBase

rclcpp/rclcpp/include/rclcpp/service.hpp

Line 278 in 4d1f18c

rclcpp::Logger node_logger_;

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

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

alsora · 2023-07-20T10:55:22Z

rclcpp/include/rclcpp/service.hpp

+    if (ret == RCL_RET_TIMEOUT) {
+      RCLCPP_WARN(
+        rclcpp::get_node_logger(node_handle_.get()).get_child("rclcpp"),
+        "failed to send response (timeout): %s",


I suggest to add the service name in the log message

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.

mauropasse · 2023-07-20T11:35:22Z

Should changes in rclcpp/include/rclcpp/service.hpp be also applied also here?:

rclcpp/rclcpp_action/src/server.cpp

Line 340 in c0d72c3

ret = rcl_action_send_goal_response(

rclcpp/rclcpp_action/src/server.cpp

Line 482 in c0d72c3

ret = rcl_action_send_cancel_response(

rclcpp/rclcpp_action/src/server.cpp

Line 539 in c0d72c3

rcl_ret_t rcl_ret = rcl_action_send_result_response(

rclcpp/rclcpp_action/src/server.cpp

Line 672 in c0d72c3

rcl_ret_t ret = rcl_action_send_result_response(

Related to this topic, we're in conversations to set the timeout in DDS from an XML, so we can increase the timeout before returning RCL_RET_TIMEOUT. If the client is discovered before it, it will get the response from the service. Otherwise, it'll hang forever waiting a response (at least, that happens with actions).

clalancette

I think that @mauropasse makes a good point and we should do the same thing for the action side of things as well. Once that is fixed I'm happy to approve this PR.

clalancette · 2023-07-20T19:17:21Z

rclcpp/include/rclcpp/service.hpp

+    if (ret == RCL_RET_TIMEOUT) {
+      RCLCPP_WARN(
+        rclcpp::get_node_logger(node_handle_.get()).get_child("rclcpp"),
+        "failed to send response (timeout): %s",


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.

clalancette · 2023-07-20T19:18:20Z

rclcpp/include/rclcpp/service.hpp

@@ -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"),


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

fujitatomoya · 2023-07-23T05:24:00Z

I think that @mauropasse makes a good point and we should do the same thing for the action side of things as well. Once that is fixed I'm happy to approve this PR.

@clalancette @mauropasse can we manage this with another issue before adding the implementation in this PR? i would like to check if that happens in detail.

fujitatomoya · 2023-07-23T05:29:32Z

I think that @mauropasse makes a good point and we should do the same thing for the action side of things as well. Once that is fixed I'm happy to approve this PR.

@clalancette @mauropasse can we manage this with another issue before adding the implementation in this PR? i would like to check if that happens in detail.

ros2/ros2#1462

fujitatomoya · 2023-07-26T21:12:58Z

@clalancette @mauropasse friendly ping.

mauropasse · 2023-07-27T11:40:10Z

@fujitatomoya : i would like to check if that happens in detail.

What do you mean with "that happens in detail"?

I know it happens with actions since that's where we observed this issue first, and come with the temporary solution of increasing the timeout.

With regards of applying the action side of these fixes in a different PR, I'm OK with that. Maybe adding tests for these situations also.

fujitatomoya · 2023-07-28T17:37:38Z

@mauropasse i was not sure if this problem can be actually reproducible with actions, thanks for the information.

With regards of applying the action side of these fixes in a different PR, I'm OK with that

👍 @clalancette what do you think?

clalancette · 2023-08-01T17:51:05Z

👍 @clalancette what do you think?

Yeah, that's fine. Let's keep this one just about the service server, and we can do the follow-up with ros2/ros2#1462 for action servers.

I'll go ahead and re-review this.

fujitatomoya · 2023-08-17T05:47:00Z

@kghost since you are not responding, i did create an another PR for this to proceed. I keep the all commits that you made. it would be appreciated if you can review it. see #2276

fujitatomoya · 2023-08-17T05:49:36Z

@clalancette @kghost i will go ahead to close this, instead can we proceed with #2276?

kghost requested review from ivanpauno, hidmic and wjwwood as code owners June 16, 2023 05:18

fujitatomoya reviewed Jun 16, 2023

View reviewed changes

rclcpp/include/rclcpp/service.hpp Outdated Show resolved Hide resolved

fujitatomoya mentioned this pull request Jun 16, 2023

rcl_send_response returns RCL_RET_TIMEOUT. ros2/rcl#1080

Closed

clalancette assigned fujitatomoya Jun 29, 2023

fujitatomoya approved these changes Jun 29, 2023

View reviewed changes

rclcpp/include/rclcpp/service.hpp Outdated Show resolved Hide resolved

fujitatomoya mentioned this pull request Jun 29, 2023

Avoid generating the exception when rcl_send_response times out. ros2/rclpy#1136

Merged

clalancette requested changes Jul 6, 2023

View reviewed changes

rclcpp/include/rclcpp/service.hpp Outdated Show resolved Hide resolved

kghost requested a review from clalancette July 10, 2023 10:03

kghost force-pushed the rolling branch from 7ba1c37 to e25d45e Compare July 11, 2023 01:34

kghost and others added 2 commits July 14, 2023 18:32

Update rclcpp/include/rclcpp/service.hpp

4d1f18c

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Zang MingJie <zealot0630@gmail.com>

kghost force-pushed the rolling branch from e25d45e to 4d1f18c Compare July 14, 2023 10:32

alsora reviewed Jul 20, 2023

View reviewed changes

clalancette requested changes Jul 20, 2023

View reviewed changes

fujitatomoya mentioned this pull request Jul 23, 2023

Do not generate the exception when action service response failed. ros2/ros2#1462

Closed

fujitatomoya mentioned this pull request Aug 17, 2023

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

Merged

fujitatomoya closed this Aug 17, 2023

alireza-moayyedi mentioned this pull request Mar 22, 2024

Added WARN for action server goal response callback in case of timeout instead of raising exception #2463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kghost commented Jun 16, 2023

fujitatomoya left a comment

fujitatomoya commented Jun 29, 2023

fujitatomoya commented Jun 29, 2023

fujitatomoya commented Jul 6, 2023

clalancette left a comment

milidam commented Jul 7, 2023

clalancette commented Jul 7, 2023

fujitatomoya commented Jul 8, 2023

kghost commented Jul 8, 2023

fujitatomoya commented Jul 11, 2023

fujitatomoya commented Jul 11, 2023

kghost commented Jul 11, 2023

fujitatomoya commented Jul 11, 2023

fujitatomoya commented Jul 14, 2023

kghost commented Jul 18, 2023

alsora Jul 20, 2023

clalancette Jul 20, 2023

alsora Jul 21, 2023

clalancette Jul 24, 2023

fujitatomoya Aug 1, 2023

alsora Jul 20, 2023

clalancette Jul 20, 2023

mauropasse commented Jul 20, 2023

clalancette left a comment

clalancette Jul 20, 2023

clalancette Jul 20, 2023

fujitatomoya commented Jul 23, 2023

fujitatomoya commented Jul 23, 2023

fujitatomoya commented Jul 26, 2023

mauropasse commented Jul 27, 2023 •

edited

Loading

fujitatomoya commented Jul 28, 2023

clalancette commented Aug 1, 2023

fujitatomoya commented Aug 17, 2023

fujitatomoya commented Aug 17, 2023

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

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

Conversation

kghost commented Jun 16, 2023

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Jun 29, 2023

fujitatomoya commented Jun 29, 2023

fujitatomoya commented Jul 6, 2023

clalancette left a comment

Choose a reason for hiding this comment

milidam commented Jul 7, 2023

clalancette commented Jul 7, 2023

fujitatomoya commented Jul 8, 2023

kghost commented Jul 8, 2023

fujitatomoya commented Jul 11, 2023

fujitatomoya commented Jul 11, 2023

kghost commented Jul 11, 2023

fujitatomoya commented Jul 11, 2023

fujitatomoya commented Jul 14, 2023

kghost commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauropasse commented Jul 20, 2023

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujitatomoya commented Jul 23, 2023

fujitatomoya commented Jul 23, 2023

fujitatomoya commented Jul 26, 2023

mauropasse commented Jul 27, 2023 • edited Loading

fujitatomoya commented Jul 28, 2023

clalancette commented Aug 1, 2023

fujitatomoya commented Aug 17, 2023

fujitatomoya commented Aug 17, 2023

mauropasse commented Jul 27, 2023 •

edited

Loading