eclipse-cyclonedds / cyclonedds

Eclipse Cyclone DDS project
https://projects.eclipse.org/projects/iot.cyclonedds
Other
854 stars 352 forks source link

ddsi_serdata_from_sample_t : how to detect if sample is loaned or not #1249

Open sjames opened 2 years ago

sjames commented 2 years ago

Hi!

I'm implementing shared memory and zero copy support for the CycloneDDS Rust binding (https://github.com/sjames/cyclonedds-rs) .

I've managed to get things working when only shared memory is used. Some bugs still remain, but the basic mechanism works. I'm facing an issue when I have a combination of subscribers - one subscriber that enables shared memory and another subscriber that does not.

When writing a loaned buffer, the sample points to the loaned buffer directly allocated by Iceoryx. Cyclone internally checks whether this pointer is loaned or not by searching in an internal table that has the list of all loaned pointers.

I'm facing a problem here. https://github.com/eclipse-cyclonedds/cyclonedds/blob/e5d772b2306eaa368a5eb6266ab960c6a0e447aa/src/core/ddsc/src/dds_write.c#L491

There is no way for the ddsi_serdata_from_sample to tell whether the sample is a raw pointer handed over from iceoryx, or whether this is of the wrapped sample that is used by the Rust binding. (https://github.com/sjames/cyclonedds-rs/blob/6776fa44384fe639968344e9ac2ade37216cd812/src/serdes.rs#L575)

Any suggestions on how I can get this information? Should this API also include a flag to indicate the sample type?

Thanks in advance! Sojan

eboasson commented 2 years ago

I've managed to get things working when only shared memory is used. Some bugs still remain, but the basic mechanism works. I'm facing an issue when I have a combination of subscribers - one subscriber that enables shared memory and another subscriber that does not.

Yay! 😃

There are indeed some leaks in how it abstracts Iceoryx if some readers are compatible with Iceoryx while others in the same process aren't, e.g., if you have one reader with a QoS that disallows the use of Iceoryx whereas the other does allow it. We figured (hoped?) that that is rare enough not to be a problem in practice, but in test cases you could easily run into it. Can it be that you are running into this particular phenomenon?

There is no way for the ddsi_serdata_from_sample to tell whether the sample is a raw pointer handed over from iceoryx, or whether this is of the wrapped sample that is used by the Rust binding. (https://github.com/sjames/cyclonedds-rs/blob/6776fa44384fe639968344e9ac2ade37216cd812/src/serdes.rs#L575)

Any suggestions on how I can get this information? Should this API also include a flag to indicate the sample type?

That particular call to ddsi_serdata_from_sample is for the case where Cyclone needs its own copy anyway so the assumption was that it then doesn't matter where the data resides. We've been looking at improving the abstraction (right now, the signs of it starting out as a PoC are still pretty obvious) with an eye towards adding support for other pub/sub mechanisms (Kafka integration would be a neat toy, to just name one possibility), and the interfaces almost certainly need to be extended to properly distinguish between serialised/non-serialised data and ownership. (Imagine trying to do Iceoryx and Kafka at the same time ...)

It is not a real answer to your question, I am afraid, and obviously it is not a great idea to extend things by adding ifdef's. It doesn't seem unreasonable to me to take a stab at improving the interface by subsuming ddsi_serdata_from_loaned_sample in ddsi_serdata_from_sample and adding a "source type" parameter (that's something we were looking at anyway) and a hint whether it should be copied/serialised or not

ROS 2 Humble is pinned to the 0.9 release branch, so we're kinda free to do things right now without having to be too concerned about keeping ROS 2 compatible with these details. Taking some steps to dealing with this particular pain point is something we want for 1.0 anyway, and right now the number of affected language bindings is still pretty small. So I'd definitely be happy to merge improvements in this area.

sjames commented 2 years ago

There are indeed some leaks in how it abstracts Iceoryx if some readers are compatible with Iceoryx while others in the same process aren't, e.g., if you have one reader with a QoS that disallows the use of Iceoryx whereas the other does allow it. We figured (hoped?) that that is rare enough not to be a problem in practice, but in test cases you could easily run into it. Can it be that you are running into this particular phenomenon?

This is exactly the case. The publisher uses shared memory. One subscriber uses shared memory and a second subscriber uses the network only. The call into ddsi_serdata_from_sample causes a crash as the Rust bunding expects the sample pointer to be a structure it created, but it receives a raw pointer to an iceoryx buffer.

ROS 2 Humble is pinned to the 0.9 release branch, so we're kinda free to do things right now without having to be too concerned about keeping ROS 2 compatible with these details. Taking some steps to dealing with this particular pain point is something we want for 1.0 anyway, and right now the number of affected language bindings is still pretty small. So I'd definitely be happy to merge improvements in this area.

My first shot at this: https://github.com/sjames/cyclonedds/commit/fefe7f7463c9a0374eb0e15dff712839ee1c96f4

Is this the change you suggested?