Make service wait for response reader

[MiguelCompany]

This addresses ros2/ros2#922

The idea is to send the GUID of the response subscription on each request (using related_sample_identity), so the service could check if that reader is already matched to the response publisher when sending the response.

I've made the changes so they are backwards compatible. The service will only use and check the related guid when received. The client will check the received related guid against both the response reader and the request writer, in case an old service answers with the latter.

[hidmic]

I wonder, does (ab)using the writer GUID in the related sample identity compromises cross-vendor communication? We'll run full CI anyways, but I figured I'd ask.

[hidmic]

@MiguelCompany nit:

Suggested change

	const eprosima::fastrtps::rtps::GUID_t& related_guid =
	const eprosima::fastrtps::rtps::GUID_t & related_guid =

[hidmic]

@MiguelCompany nit:

Suggested change

	if( (related_guid.entityId.value[3] & 0x04) != 0)
	if((related_guid.entityId.value[3] & 0x04) != 0)

[hidmic]

Also, a comment explaining this bit fiddling would be great.

[MiguelCompany]

@hidmic Thanks for your review.

I wonder, does (ab)using the writer GUID in the related sample identity compromises cross-vendor communication? We'll run full CI anyways, but I figured I'd ask.

Cyclone is not interoperable with Fast RTPS on services, and I'm not sure about RTI Connext. The latter would most probably ignore the related_guid from the request, which is covered by the following lines.

rmw_fastrtps/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_client_info.hpp

Lines 92 to 93 in fce9022

	if (response.sample_identity_.writer_guid() == info_->reader_guid_ ||
	response.sample_identity_.writer_guid() == info_->writer_guid_) {

[hidmic]

Full CI:

• Linux

[hidmic]

@MiguelCompany mind to fix linter issues?

[jacobperron]

Besides some linter issues, the change LGTM. We should evaluate if this is going to break interoperability with Connext (assuming it was working before).

[jacobperron]

I think this will cause a linter error

Suggested change

	if((related_guid.entityId.value[3] & entity_id_is_reader_bit) != 0)
	{
	if ((related_guid.entityId.value[3] & entity_id_is_reader_bit) != 0) {

[MiguelCompany]

Fixed on 13c540b

[jacobperron]

Suggested change

	if(!info->pub_listener_->wait_for_subscription(related_guid, std::chrono::milliseconds(100))) {
	if (!info->pub_listener_->wait_for_subscription(related_guid, std::chrono::milliseconds(100))) {

[MiguelCompany]

Fixed on 13c540b

[ivanpauno]

I don't think that blocking in rmw_send_response is acceptable.

[ivanpauno]

I think the linter doesn't accept that style (it the linter is happy, no change needed).

Suggested style:

        if (response.sample_identity_.writer_guid() == info_->reader_guid_ ||
          response.sample_identity_.writer_guid() == info_->writer_guid_)
        {

[MiguelCompany]

Fixed on 13c540b

[ivanpauno]

nit: prefer std::unordered_set

[MiguelCompany]

Done in 3c48b25. I had to add a hash function for GUID_t.

[ivanpauno]

rmw_send_response was not blocking, and it should be continue being not blocking.

[wjwwood]

What if we change this to 0 timeout? That way we at least know that it won't work and can react to it.

[MiguelCompany]

Based on the conversation below, I assume the code is ok as it is right now.

If this be changed to an indefinite block, please tell me and I'll do it

[jacobperron]

I don't think that blocking in rmw_send_response is acceptable.

As I understand, blocking is the primary patch being proposed. Though it is not documented in the rmw API, the rcl API does make the assumption that it is a non-blocking call (I think we should update the documentation if we merge this PR). At least there is a timeout so we won't block indefinitely, but I agree it would be better if we did not block at all. However, what are the alternatives? Do you think a patch similar to what's being proposed in ros2/rmw_cyclonedds#187 would work? (Though, it looks like the rmw_cyclonedds patch is still blocking similar to this PR)

[ivanpauno]

As I understand, blocking is the primary patch being proposed.

Yes.

At least there is a timeout so we won't block indefinitely, but I agree it would be better if we did not block at all

👍

However, what are the alternatives?

A mechanism to ensure that rmw_is_service_server_available returns true when the client discovered the server and the server the client.

Do you think a patch similar to what's being proposed in ros2/rmw_cyclonedds#187 would work? (Though, it looks like the rmw_cyclonedds patch is still blocking similar to this PR)

I thought the cyclonedds PR could solve the problem without needing to introduce blocking behavior, but thinking about it again, it isn't true.

---

If many people leave a 👍, I think that introducing the blocking behavior and documenting it is ok.

waiting for other people approval on blocking behavior change

[hidmic]

Same as in ros2/rmw_cyclonedds#187, I think this is OK for a short-term workaround. And while blocking isn't ideal, even if discovery takes up to 100ms to happen we've simply moved the wait from client to server (which isn't strictly worst, performance-wise, since discovery is async).

[wjwwood]

I'm not a fan of the blocking either. It's not a real solution and it's a fragile workaround, just moving the problem to "less frequent" or only on slow computers, which isn't good either.

I would prefer to see us return an intermediate error case and let the higher level deal with waiting or retrying. For me this function should send the response immediately or else fail for some reason.

That means the workaround would need to be in rcl, rclcpp, or the user code instead.

Same as in ros2/rmw_cyclonedds#187, I think this is OK for a short-term workaround.

My fear, based on experience, is that both will become permanent. I would prefer to see a proper solution like a more reliable implementation at the middleware layer (e.g. using keys or maybe having a way to keep the client from assuming the service is ready until we know it should work, i.e. matching isn't enough apparently) or changing our API, like making the rclcpp API so that the user must send the response and then make it possible for that to fail. Allowing the user who implements the service server callback to handle any waiting or retrying that may be required.

[wjwwood]

What if we change this to 0 timeout? That way we at least know that it won't work and can react to it.

[wjwwood]

If others on the team feel that the urgency in fixing this issue outweighs the concerns about the workaround, then I'll go along with it, but I cannot enthusiastically approve this workaround (nor the cyclone one last I looked, but maybe it was updated in the meantime).

[eboasson]

Interesting ... similar approaches to fixing the same problems at the same time 🙂

@wjwwood @hidmic @ivanpauno, I share your dislike about these workarounds. The way I see it, the options are:

1. stick with dashing/eloquent behaviour (broken, but not so badly that it is unusable in practice);
2. accept blocking in rmw_send_response as a workaround (with the risk of it becoming permanent);
3. don't block, instead always handle the first serviceable request in the queue (but this comes at a significant increase in complexity, which is a risk in itself and consumes even more development effort on a throwaway thing).

Evidently, the proper solution is fixing the deficiency in the DDS spec (or the implementation of it) but at least for Cyclone there are process barriers that prevent doing that prior to the Foxy release. It seems to me 1 and 2 are the only sensible options. Pick your poison ...

[MiguelCompany]

@eboasson

1. stick with dashing/eloquent behaviour (broken, but not so badly that it is unusable in practice);
2. accept blocking in rmw_send_response as a workaround (with the risk of it becoming permanent);
3. don't block, instead always handle the first serviceable request in the queue (but this comes at a significant increase in complexity, which is a risk in itself and consumes even more development effort on a throwaway thing).

I think there is a fourth option

4. don't block and return a specific error if we know that the response is not going to be received. This way we let the upper layer decide whether to wait or not.

I can even think of new a new API function allowing the user to wait till the response can be reliably sent.

[eboasson]

@MiguelCompany certainly there is that 4th option, or the 5th of an additional interface like you mention, but those make the application responsible for dealing with the problem. I just don't believe this should be the application's problem.

Furthermore, just like @wjwwood I worry about temporary workarounds becoming permanent, and a workaround that involves changes to the application will definitely never go away ...

[ivanpauno]

@eboasson @MiguelCompany I would stick with the old poison, i.e. option 2.
The issue is easy to document, and users can work it around if they need (and we can work it around in our tests too).

It would be great in early G-Turtle to iterate on a solution of the problem.

Evidently, the proper solution is fixing the deficiency in the DDS spec (or the implementation of it) but at least for Cyclone there are process barriers that prevent doing that prior to the Foxy release. It seems to me 1 and 2 are the only sensible options. Pick your poison ...

I think that more than an issue of the DDS spec, we assumed in the service implementation something that is not ensured: if A discovered B, B has already discovered A.
If needed, we could add a side-channel "discovery mechanism", to ensure rmw_is_service_server_available returns true correctly.

Note: I'm not sure if rmw_connext implementation has the same problems, as we are using an implementation of RPC over DDS they provide.

[hidmic]

I think that more than an issue of the DDS spec, we assumed in the service implementation something that is not ensured: if A discovered B, B has already discovered A.

On that note. OMG DDS-RPC 1.0 spec covers the discovery problem for RPC services in section 7.6. It acknowledges that independent discovery isn't robust and can lead to races (Basic Service Mapping under section 7.6.1). It then suggests an extension to the data type sent over discovery built-in topics and an algorithm (Enhanced Service Mapping under section 7.6.2), which basically boils down to the client not sending requests until it has discovered the server's response data writer and the server not picking up those requests until it has discovered the client's response data reader.

Something like that would make the most sense to me, providing for reliable service operation without cluttering user code with fail-safe/retry logic. But I can understand that's unlikely to land for Foxy. Thus, I'd stick to poison No. 2 too, keeping changes under the rmw hood so we can backport a proper fix in a future patch release.

[jacobperron]

In lieu of a proper fix (like the one suggested by @hidmic), I'd be okay with a version of option 2 landing for Foxy. I agree we should aim to replace the patch with a better solution early in the G-turtle cycle.

[wjwwood]

Ok, option 2 is ok with me, since the caveats are known.

[nuclearsandwich]

I understand why and I'd agree, but I think we don't have 32 bits CI available.

armhf CI is unavailable right now due to issues with kernel/userland mismatch for 32-bit userlands on our CI. You can build and run locally using the osrf/ubuntu_armhf:focal docker image to get a test via QEMU emulation.

[jacobperron]

Given that we're scheduled to release this week, I'd be more comfortable holding this PR for the first patch release to give us more time for testing. This means holding until the end of this week.

[ivanpauno]

Given that we're scheduled to release this week, I'd be more comfortable holding this PR for the first patch release to give us more time for testing. This means holding until the end of this week.

If we do this change in a patch release and somebody updates the machine that has the client side but not the one that has the server side, communication will stop working (@MiguelCompany am I correct?).

IMO, this can't be backported, it's now or no fix for Foxy.

[hidmic]

If we do this change in a patch release and somebody updates the machine that has the client side but not the one that has the server side, communication will stop working (@MiguelCompany am I correct?).

@ivanpauno this patch is backwards compatible IIUC

[ivanpauno]

@ivanpauno this patch is backwards compatible IIUC

The server side for sure, but not the client side.

[ivanpauno]

After talking offline with @hidmic, this looks backward compatible to me too.

[SteveMacenski]

@jacobperron its been a week, would it be OK to merge now?

[hidmic]

Going in.

[ros-discourse]

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]

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