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.

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

Restore dashing/eloquent behaviour of "service_is_available" #190

Merged
merged 2 commits into from
Jun 1, 2020

Conversation

eboasson
Copy link
Collaborator

This "mostly fixes" the lost service responses (#183, #74) by reinstating the check for a matching reader and writer in rmw_service_server_is_available that was accidentally deleted when the 1:1 mapping of nodes to participants was replaced by a 1:1 mapping of contexts to participants. This effectively restores dashing/eloquent behaviour.

It doesn't truly fix the problem and in the presence of packet loss the likelihood of failure remains significant. However, the workaround proposed in #187 is a bit controversial and this restoration of the old code at least should be non-controversial.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM!

@hidmic hidmic requested review from mjcarroll and nuclearsandwich and removed request for mjcarroll and nuclearsandwich May 29, 2020 13:54
@ivanpauno
Copy link
Member

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

@ivanpauno ivanpauno added in review bug Something isn't working labels May 29, 2020
@ivanpauno
Copy link
Member

@hidmic can we merge this one already?

I think that no matter if we go ahead with #187 or not, merging this one first is a good idea.

@ivanpauno
Copy link
Member

@hidmic friendly ping

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

@ivanpauno agreed.

@hidmic
Copy link
Contributor

hidmic commented Jun 1, 2020

@jacobperron ?

@jacobperron
Copy link
Member

Have you investigated the failing test? I guess it's something that we don't normally test for, since it would typically run with rmw_fastrtps_cpp.

@hidmic
Copy link
Contributor

hidmic commented Jun 1, 2020

@jacobperron it's a cppcheck error about macro configuration.

@jacobperron
Copy link
Member

@hidmic
Copy link
Contributor

hidmic commented Jun 1, 2020

I see. I have not. I'll check locally.

@ivanpauno
Copy link
Member

It's happening in build.ros2.org too: http://build.ros2.org/view/Fci/job/Fci__nightly-cyclonedds_ubuntu_focal_amd64/128/testReport/junit/(root)/rclcpp/test_subscription_topic_statistics_gtest_missing_result/.

Seems unrelated to me

@hidmic
Copy link
Contributor

hidmic commented Jun 1, 2020

It's happening in build.ros2.org too

Indeed. Fix incoming. In the meantime, let's merge.

@hidmic hidmic merged commit c424bc1 into ros2:master Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working in review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants