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

rcl_send_response returns RCL_RET_TIMEOUT. #1048

Merged
merged 1 commit into from
Aug 18, 2023
Merged

rcl_send_response returns RCL_RET_TIMEOUT. #1048

merged 1 commit into from
Aug 18, 2023

Conversation

fujitatomoya
Copy link
Collaborator

part of ros2/ros2#1253

@fujitatomoya
Copy link
Collaborator Author

for both rclcpp and rclpy need to check the return code, and throws exception accordingly. we would need to add exception for timeout for client interface. Behavior in client libraries need to be discussed before implementation.

@fujitatomoya
Copy link
Collaborator Author

see ros2/rmw#350 (comment)

@fujitatomoya
Copy link
Collaborator Author

CI:

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

audrow
audrow approved these changes Mar 27, 2023
View reviewed changes
Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks good to me!

kghost pushed a commit to kghost-ros2/rcl that referenced this pull request Jun 16, 2023
rcl_send_response returns RCL_RET_TIMEOUT.
c41f2d5 
part of ros2/ros2#1253

We hit this problem where a broken client crashes the service executor.
With this change we can fix it in rclcpp.

Note: this is same as ros2#1048 which is
not taken attention for a while.
@kghost kghost mentioned this pull request Jun 16, 2023
Closed
kghost pushed a commit to kghost-ros2/rcl that referenced this pull request Jun 16, 2023
rcl_send_response returns RCL_RET_TIMEOUT.
0d141c2 
part of ros2/ros2#1253

We hit this problem where a broken client crashes the service executor.
With this change we can fix it in rclcpp.

Note: this is same as ros2#1048 which is
not taken attention for a while.

Signed-off-by: Zang MingJie <[email protected]>
kghost pushed a commit to kghost-ros2/rcl that referenced this pull request Jun 16, 2023
rcl_send_response returns RCL_RET_TIMEOUT.
0ab9550 
part of ros2/ros2#1253

We hit this problem where a broken client crashes the service executor.
With this change we can fix it in rclcpp.

Note: this is same as ros2#1048 which is
not taken attention for a while.

Signed-off-by: Zang MingJie <[email protected]>
kghost pushed a commit to kghost-ros2/rcl that referenced this pull request Jun 16, 2023
rcl_send_response returns RCL_RET_TIMEOUT.
ba9ceea 
part of ros2/ros2#1253

We hit this problem where a broken client crashes the service executor.
With this change we can fix it in rclcpp.

Note: this is same as ros2#1048 which is
not taken attention for a while.

Signed-off-by: Zang MingJie <[email protected]>
kghost added a commit to kghost-ros2/rcl that referenced this pull request Jun 16, 2023
@kghost
rcl_send_response returns RCL_RET_TIMEOUT.
d8faa66 
part of ros2/ros2#1253

We hit this problem where a broken client crashes the service executor.
With this change we can fix it in rclcpp.

Note: this is same as ros2#1048 which is not been taken attention for a
while.

Signed-off-by: Zang MingJie <[email protected]>
@fujitatomoya fujitatomoya mentioned this pull request Jun 16, 2023
Closed
@fujitatomoya
Copy link
Collaborator Author

@asorbini can i ask your help here?

so what we are trying to do here, if sending response times out, we will print the warning instead of generating exception for the client libraries, so that executor can proceed the next task.

as far as i see, rmw_connextdds does not return RMW_RET_TIMEOUT via rmw_send_response, it always returns RMW_RET_ERROR if anything wrong happens? (rmw_fastrtps and rmw_cyclonedds returns RMW_RET_TIMEOUT)

https://github.com/ros2/rmw_connextdds/blob/rolling/rmw_connextdds_common/src/ndds/dds_api_ndds.cpp#L709-L716

which i think that is fine because that is implementation detail, but just in case i wanted to share this with you since the same problem will exist with rmw_connextdds to generate the exception here.

just checking the behavior of rmw_connextdds to see if my understanding is correct.

thanks in advance,
Tomoya

@fujitatomoya fujitatomoya mentioned this pull request Jun 29, 2023
Merged
@fujitatomoya
Copy link
Collaborator Author

CI:

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

clalancette
clalancette reviewed Jul 6, 2023
View reviewed changes
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.

The code here looks good to me, but likely needs a rebase to pass CI.

@fujitatomoya fujitatomoya force-pushed the fujitatomoya/bugfix-ros2-issues-1253 branch from 9d9c3e9 to 7c18664 Compare July 11, 2023 01:44
@fujitatomoya
Copy link
Collaborator Author

@clalancette thanks for the review, i just did rebase on rolling.

@fujitatomoya
Copy link
Collaborator Author

CI:

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

@fujitatomoya
Copy link
Collaborator Author

CI:

  • Linux Build Status

windows failure is unrelated.

@clalancette clalancette mentioned this pull request Jul 20, 2023
Open
@fujitatomoya fujitatomoya force-pushed the fujitatomoya/bugfix-ros2-issues-1253 branch from 7c18664 to 4374dea Compare August 17, 2023 05:01
@fujitatomoya
Copy link
Collaborator Author

CI:

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

@fujitatomoya fujitatomoya merged commit c7b394a into rolling Aug 18, 2023
@delete-merged-branch delete-merged-branch bot deleted the fujitatomoya/bugfix-ros2-issues-1253 branch August 18, 2023 16:14
@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Aug 18, 2023 •
edited
Loading

Originally this issue was reported with Galactic which is already E.O.L. Now we can see many related issues, i think it would be better to backport these fixes to Humble/Iron as well. and there is no ABI break.

@fujitatomoya
Copy link
Collaborator Author

@Mergifyio backport iron humble

@mergify
Copy link

mergify bot commented Aug 18, 2023 •
edited
Loading

backport iron humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 18, 2023
@fujitatomoya @mergify
rcl_send_response returns RCL_RET_TIMEOUT. (#1048)
d04088e 
Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit c7b394a)
@mergify mergify bot mentioned this pull request Aug 18, 2023
Merged
mergify bot pushed a commit that referenced this pull request Aug 18, 2023
@fujitatomoya @mergify
rcl_send_response returns RCL_RET_TIMEOUT. (#1048)
8613e29 
Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit c7b394a)

# Conflicts:
#	rcl/src/rcl/service.c
@mergify mergify bot mentioned this pull request Aug 18, 2023
Merged
@fujitatomoya
Copy link
Collaborator Author

lets give some soak time like a week for rolling before merging this backport PRs, then i can start the CI for PRs for Humble and Iron backport.

@fujitatomoya fujitatomoya mentioned this pull request Aug 18, 2023
Closed
fujitatomoya added a commit that referenced this pull request Sep 1, 2023
@mergify @fujitatomoya
rcl_send_response returns RCL_RET_TIMEOUT. (#1048) (#1090)
1207535 
Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit c7b394a)

Co-authored-by: Tomoya Fujita <[email protected]>
fujitatomoya added a commit that referenced this pull request Sep 1, 2023
@mergify @fujitatomoya
rcl_send_response returns RCL_RET_TIMEOUT. (backport #1048) (#1091)
887e696 
* rcl_send_response returns RCL_RET_TIMEOUT. (#1048)

Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit c7b394a)

# Conflicts:
#	rcl/src/rcl/service.c

* fix conflict with service introspection.

Signed-off-by: Tomoya Fujita <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
mauropasse pushed a commit to mauropasse/rcl that referenced this pull request Oct 18, 2023
@mergify @fujitatomoya
rcl_send_response returns RCL_RET_TIMEOUT. (backport ros2#1048) (ros2…
6134721 
…#1091)

* rcl_send_response returns RCL_RET_TIMEOUT. (ros2#1048)

Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit c7b394a)

# Conflicts:
#	rcl/src/rcl/service.c

* fix conflict with service introspection.

Signed-off-by: Tomoya Fujita <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
@mauropasse mauropasse mentioned this pull request Oct 18, 2023
Merged
alsora added a commit to irobot-ros/rcl that referenced this pull request Oct 18, 2023
@alsora
Merge pull request #28 from mauropasse/mauro/galactic-events-executor
59c7b07 
rcl_send_response returns RCL_RET_TIMEOUT. (backport ros2#1048) (ros2#1091)
mauropasse pushed a commit to mauropasse/rcl that referenced this pull request Oct 18, 2023
@mergify @fujitatomoya
rcl_send_response returns RCL_RET_TIMEOUT. (backport ros2#1048) (ros2…
82bebd4 
…#1091)

* rcl_send_response returns RCL_RET_TIMEOUT. (ros2#1048)

Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit c7b394a)

# Conflicts:
#	rcl/src/rcl/service.c

* fix conflict with service introspection.

Signed-off-by: Tomoya Fujita <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
@mauropasse mauropasse mentioned this pull request Oct 18, 2023
Merged
alsora added a commit to irobot-ros/rcl that referenced this pull request Oct 18, 2023
@alsora
Merge pull request #29 from mauropasse/mauro/irobot-humble
64503c1 
rcl_send_response returns RCL_RET_TIMEOUT. (backport ros2#1048) (ros2#1091)
@fujitatomoya fujitatomoya mentioned this pull request May 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@audrow audrow audrow approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fujitatomoya @clalancette @audrow