Skip to content

[foxy backport] Make service wait for response reader (#390)#412

Merged
jacobperron merged 1 commit intofoxyfrom
jacob/backport_390
Jul 21, 2020
Merged

[foxy backport] Make service wait for response reader (#390)#412
jacobperron merged 1 commit intofoxyfrom
jacob/backport_390

Conversation

@jacobperron
Copy link
Copy Markdown
Member

@jacobperron jacobperron commented Jul 21, 2020

Backport #390 to Foxy.

Note, this breaks ABI compatibility, but only below the RMW layer. IMO, the breakage is acceptable considering the improvement to service discovery.

I'm still checking if this change is backports compatible over the wire.

@MiguelCompany @jacobperron
Make service wait for response reader (#390)
d1942c0 
* Sending response subscriber guid with request.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Server ensures that response reader is matched.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Addressing review.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Linters

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Using unordered_set

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Linters

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Additional checks on rmw_service_server_is_available.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Suggestions on guid_utils

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Added TODO mentioning DDS-RPC.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* linters again.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@jacobperron jacobperron requested review from hidmic and ivanpauno and removed request for ivanpauno July 21, 2020 18:26
ivanpauno
ivanpauno approved these changes Jul 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

IMO, the breakage is acceptable considering the improvement to service discovery.

I agree.
We should comment the breakage in the release notes, clarifying the it only affects users making use of FastRTPS native handles.

I'm still checking if this change is backports compatible over the wire.

I think we should check the following:

  • A client of the new version can make requests to a service of the old version.
  • A client of the old version can make requests to a service of the new version.
  • Service and clients of the old version are listed correctly by a ros2cli tool of the new version.
  • Service and clients of the new version are listed correctly by a ros2cli tool of the old version.

That should be enough to confirm wire compatibility.

@jacobperron
Copy link
Copy Markdown
Member Author

jacobperron commented Jul 21, 2020

  • A client of the new version can make requests to a service of the old version.
  • A client of the old version can make requests to a service of the new version.
  • Service and clients of the old version are listed correctly by a ros2cli tool of the new version.
  • Service and clients of the new version are listed correctly by a ros2cli tool of the old version.

All of these checks LGTM; I did checks on my local machine and between two hosts over a VPN.

@jacobperron jacobperron merged commit 75554ec into foxy Jul 21, 2020
@delete-merged-branch delete-merged-branch bot deleted the jacob/backport_390 branch July 21, 2020 19:26
@jacobperron
Copy link
Copy Markdown
Member Author

jacobperron commented Jul 21, 2020

Note, I've also backported #409 which fixes a compiler warning introduced by this change (#413)

@jacobperron jacobperron changed the title Make service wait for response reader (#390) [foxy backport] Make service wait for response reader (#390) Jul 21, 2020
@MiguelCompany
Copy link
Copy Markdown
Collaborator

MiguelCompany commented Jul 22, 2020

We should comment the breakage in the release notes, clarifying the it only affects users making use of FastRTPS native handles.

This is only affecting rmw, so users making use of native handles will NOT be affected, right?

@jacobperron
Copy link
Copy Markdown
Member Author

jacobperron commented Jul 22, 2020

This is only affecting rmw, so users making use of native handles will NOT be affected, right?

I think what @ivanpauno meant, is it only affects users making use of the handles in rmw_fastrtps (instead of the generic rmw structs). IIUC, only people using the structs CustomClientInfo and/or CustomServiceInfo will be affected. Does that sound right?

@MiguelCompany
Copy link
Copy Markdown
Collaborator

MiguelCompany commented Jul 22, 2020

Does that sound right?

Perfect!

@ros-discourse
Copy link
Copy Markdown

ros-discourse commented Jul 23, 2020

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/new-packages-for-foxy-fitzroy-2020-07-23/15570/2

@ros-discourse
Copy link
Copy Markdown

ros-discourse commented Aug 12, 2020

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/new-packages-and-patch-release-for-ros-2-foxy-fitzroy-2020-08-07/15818/1

@JLBuenoLopez JLBuenoLopez mentioned this pull request Oct 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivanpauno ivanpauno ivanpauno approved these changes

@hidmic hidmic Awaiting requested review from hidmic

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jacobperron @MiguelCompany @ros-discourse @ivanpauno