Implementation of Publish Subscribe Message Exchange interface

[dpotman]

As a replacement for the current Iceoryx integration in Cyclone, this introduces the Publish Subscribe Message Exchange (PSMX) interface that allows implementing additional methods of pub-sub data-exchange as an alternative to Cyclone's networked transport. The PSMX interface allows to load implementations as plugins, so there is no need for compile-time linking.

One of the PSMX implementations is the Iceoryx plugin, which allows using Iceoryx as shared memory transport for Cyclone. For testing, a Cyclone based PSMX implementation is included. This also includes some rework of the loan mechanism, so that it better integrates with the PSMX mechanism.

I've created this as a draft PR, because some things still need to be improved and fixed. But in the current state the mechanism is fully working using the Cyclone plugin implementation (for testing purposes), I think it is useful to get some feedback at this point. For the Iceoryx implementation I need to do some work, I'll add a commit with that plugin included later this week.

[github-advanced-security]

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

[reicheratwork]

I am having issues with the following sequence of events:

• a sample with kind = SDK_KEY is written with serdata_default_from_loaned_sample and serialization does not occur
  • a ddsi_serdata is created with kind = SDK_KEY
  • the sample_state field in the metadata is set to DDS_LOANED_SAMPLE_STATE_RAW because no serialization
• it is received with serdata_default_from_psmx
  • this sees that the metadata sample_state says DDS_LOANED_SAMPLE_STATE_RAW
  • therefore the loaned_sample_state_to_serdata_kind sets the kind for the ddsi_serdata to be created to be SDK_DATA

I see an asymmetry in this flow, but I do not know whether this may cause real issues with readers/history caches/etc?
@eboasson , @dpotman could you please check?

[eboasson]

I am having issues with the following sequence of events:

• a sample with kind = SDK_KEY is written with serdata_default_from_loaned_sample and serialization does not occur

  • a ddsi_serdata is created with kind = SDK_KEY
  • the sample_state field in the metadata is set to DDS_LOANED_SAMPLE_STATE_RAW because no serialization

• it is received with serdata_default_from_psmx

  • this sees that the metadata sample_state says DDS_LOANED_SAMPLE_STATE_RAW
  • therefore the loaned_sample_state_to_serdata_kind sets the kind for the ddsi_serdata to be created to be SDK_DATA

I see an asymmetry in this flow, but I do not know whether this may cause real issues with readers/history caches/etc? @eboasson , @dpotman could you please check?

It seems to me that that would be a problem. For example, the difference between a dispose and a writedispose here is whether the serdata is SDK_KEY or SDK_DATA and that in turn determines whether data can be pushed out of the reader history cache by it.

An obvious way to address it would be to add yet another state but that may well add bloat (I haven't checked the code to see whether I actually expect it to do so in this case, it is more of a general statement on what happens when you add yet another case). The alternative I see is to always serialize keys, which I think would be a reasonable option as they are typically small and sent only rarely.

[reicheratwork]

Ran into the following issue:

Using the Helloworld examples, and adding the following config file in both HelloWorldPublisher and HelloWorldSubscriber's paths:

<CycloneDDS>
<General>
<Interfaces>
<PubSubMessageExchange name'"iox" library="psmx_iox" priority="1000000" />
</Interfaces>
</General>
</CycloneDDS>

I get the following error on the process which is opened first:
src/core/ddsi/src/ddsi_plist.c:2844 is_valid_psmx_instance_id: Assertion gv->interfaces[i].loc.kind == DDSI_LOCATOR_KIND_PSMX failed.

Running the process in gdb I found out that while ddsi_network_interface::is_psmx flag is set to true, ddsi_network_interface::loc::kind is 1 which is the value for DDSI_LOCATOR_KIND_UDPv4

Something is inconsistent here, probably in the discovery/matching part of DDS.

[reicheratwork]

Ran into another "issue":
By compiling CycloneDDS with the iox wrapper and supplying it with a config which indicates that it should use it, the local delivery of data (so in the same process) no longer uses the fastpath delivery. This is caused by get_num_fast_path_readers returning 0 on dds_write.c:588
Writing on a writer with a matching reader in the same process now makes the exchange using PSMX.

While in most cases this will not have a major effect on the behaviour experienced by end-users, the extra delay incurred by having the loaned sample go from program - psmx - program, and it not being immediately available on the reader after the call to dds_write finishes may cause some programs to behave differently.

[eboasson]

Ran into the following issue:

Using the Helloworld examples, and adding the following config file in both HelloWorldPublisher and HelloWorldSubscriber's paths:

<CycloneDDS>
<General>
<Interfaces>
<PubSubMessageExchange name'"iox" library="psmx_iox" priority="1000000" />
</Interfaces>
</General>
</CycloneDDS>

I get the following error on the process which is opened first: src/core/ddsi/src/ddsi_plist.c:2844 is_valid_psmx_instance_id: Assertion gv->interfaces[i].loc.kind == DDSI_LOCATOR_KIND_PSMX failed.

Running the process in gdb I found out that while ddsi_network_interface::is_psmx flag is set to true, ddsi_network_interface::loc::kind is 1 which is the value for DDSI_LOCATOR_KIND_UDPv4

Something is inconsistent here, probably in the discovery/matching part of DDS.

Is this reproducible for you? Because I did the "obvious" thing and that went fine. Of course there are also tons of subtle and less-subtle differences between my platform and yours ... If it is reproducible, a pair of traces would probably be interesting.

[eboasson]

Ran into another "issue": By compiling CycloneDDS with the iox wrapper and supplying it with a config which indicates that it should use it, the local delivery of data (so in the same process) no longer uses the fastpath delivery. This is caused by get_num_fast_path_readers returning 0 on dds_write.c:588 Writing on a writer with a matching reader in the same process now makes the exchange using PSMX.

While in most cases this will not have a major effect on the behaviour experienced by end-users, the extra delay incurred by having the loaned sample go from program - psmx - program, and it not being immediately available on the reader after the call to dds_write finishes may cause some programs to behave differently.

With Iceoryx this is the correct behaviour, despite it being surprising and exhibiting worse performance. The problem is that Iceoryx doesn't give us the ability to prevent it from delivering data to subscribers in the same process as the publisher. So rather than two everything twice, we use the unavoidable path.

The test cases are definitely sensitive to this sort of thing, because there are many tests that depend on the synchronous updating of the RHC, but they are not typical. Applications are less likely to run into this, firstly because of the way DDS applications tend to be structured, and secondly because there is not a formal promise that the RHC update is synchronous.

[eboasson]

Ran into the following issue:
Using the Helloworld examples, and adding the following config file in both HelloWorldPublisher and HelloWorldSubscriber's paths:

<CycloneDDS>
<General>
<Interfaces>
<PubSubMessageExchange name'"iox" library="psmx_iox" priority="1000000" />
</Interfaces>
</General>
</CycloneDDS>

I get the following error on the process which is opened first: src/core/ddsi/src/ddsi_plist.c:2844 is_valid_psmx_instance_id: Assertion gv->interfaces[i].loc.kind == DDSI_LOCATOR_KIND_PSMX failed.
Running the process in gdb I found out that while ddsi_network_interface::is_psmx flag is set to true, ddsi_network_interface::loc::kind is 1 which is the value for DDSI_LOCATOR_KIND_UDPv4
Something is inconsistent here, probably in the discovery/matching part of DDS.

Is this reproducible for you? Because I did the "obvious" thing and that went fine. Of course there are also tons of subtle and less-subtle differences between my platform and yours ... If it is reproducible, a pair of traces would probably be interesting.

Apologies ... I should remember to always have a look before trying ...

Does this fix it:

diff --git a/src/core/ddsi/src/ddsi_nwinterfaces.c b/src/core/ddsi/src/ddsi_nwinterfaces.c
index 169a734fd..3a9cfaba3 100644
--- a/src/core/ddsi/src/ddsi_nwinterfaces.c
+++ b/src/core/ddsi/src/ddsi_nwinterfaces.c
@@ -220,6 +220,7 @@ static enum maybe_add_interface_result maybe_add_interface (struct ddsi_domaingv
   dst->loopback = loopback ? 1 : 0;
   dst->link_local = link_local ? 1 : 0;
   dst->if_index = ifa->index;
+  dst->is_psmx = false;
   if ((dst->name = ddsrt_strdup (ifa->name)) == NULL)
     return MAI_OUT_OF_MEMORY;
   dst->priority = loopback ? 2 : 0;

It fits the bill perfectly. Memory is malloc'd and so the missing initializations means the status of the flag was undefined. Then you're safely in platform-specific territory. Address sanitizer initializes it with 0xbe, it is quite possible that on my device that means it ends up in the correct state; it is also quite likely that you have different build settings or a slightly different memory layout and that it does something else for you.

[reicheratwork]

Ran into another "issue": By compiling CycloneDDS with the iox wrapper and supplying it with a config which indicates that it should use it, the local delivery of data (so in the same process) no longer uses the fastpath delivery. This is caused by get_num_fast_path_readers returning 0 on dds_write.c:588 Writing on a writer with a matching reader in the same process now makes the exchange using PSMX.
While in most cases this will not have a major effect on the behaviour experienced by end-users, the extra delay incurred by having the loaned sample go from program - psmx - program, and it not being immediately available on the reader after the call to dds_write finishes may cause some programs to behave differently.

With Iceoryx this is the correct behaviour, despite it being surprising and exhibiting worse performance. The problem is that Iceoryx doesn't give us the ability to prevent it from delivering data to subscribers in the same process as the publisher. So rather than two everything twice, we use the unavoidable path.

The test cases are definitely sensitive to this sort of thing, because there are many tests that depend on the synchronous updating of the RHC, but they are not typical. Applications are less likely to run into this, firstly because of the way DDS applications tend to be structured, and secondly because there is not a formal promise that the RHC update is synchronous.

I ran into this when having my "custom" config file still being used even when running the tests, because CYCLONEDDS_URI was set.
Maybe we should have the tests which do not touch on the PSMX functionality (so almost all of them) explicitly load a "correct" config, and not rely on what the end-user's config is?

[reicheratwork]

Does this fix it:

I will see what this does on my end.

[eboasson]

Ran into another "issue": By compiling CycloneDDS with the iox wrapper and supplying it with a config which indicates that it should use it, the local delivery of data (so in the same process) no longer uses the fastpath delivery. This is caused by get_num_fast_path_readers returning 0 on dds_write.c:588 Writing on a writer with a matching reader in the same process now makes the exchange using PSMX.
While in most cases this will not have a major effect on the behaviour experienced by end-users, the extra delay incurred by having the loaned sample go from program - psmx - program, and it not being immediately available on the reader after the call to dds_write finishes may cause some programs to behave differently.

With Iceoryx this is the correct behaviour, despite it being surprising and exhibiting worse performance. The problem is that Iceoryx doesn't give us the ability to prevent it from delivering data to subscribers in the same process as the publisher. So rather than two everything twice, we use the unavoidable path.
The test cases are definitely sensitive to this sort of thing, because there are many tests that depend on the synchronous updating of the RHC, but they are not typical. Applications are less likely to run into this, firstly because of the way DDS applications tend to be structured, and secondly because there is not a formal promise that the RHC update is synchronous.

I ran into this when having my "custom" config file still being used even when running the tests, because CYCLONEDDS_URI was set. Maybe we should have the tests which do not touch on the PSMX functionality (so almost all of them) explicitly load a "correct" config, and not rely on what the end-user's config is?

Overriding the configuration has it is own drawbacks: I just stuff <Gen><Interfaces><Netw n="lo0"/></></><Int><LivelinessM>true</></><Tr><Out>cdds.log</></> in CYCLONEDDS_URI in my shell initialisation script so the firewall doesn't get in my way as badly and the thread liveliness monitoring is enabled always, and I also use it to enable tracing in test runs. So simply overriding is a no-go as far as I am concerned.

I'd rather modify the tests that are affected by this to avoid PSMX by setting a QoS.

[reicheratwork]

I'd rather modify the tests that are affected by this to avoid PSMX by setting a QoS.

Let me have look into this.

[eboasson]

I can't think of something else to check, the test runs have been good for a while, the Python binding is unaffected and the C++ has its companion PR ready, so I think it is time to merge this.

For the record, I am not pressing "approve" because that would suggest I am in the position of an impartial reviewer, which I am not. This PR is a joint effort where all work has been reviewed prior to updating this branch.

Also, I am expecting some unknown unknown causing trouble, because no matter how hard I try, it always happens. But I really do not know what it is that will cause said trouble.