Add a reference-counted version of SampleMut

[phil-opp]

Allows to create owned samples when the publisher is reference-counted.

Note: This is only a first draft to request feedback. I'm happy to polish this PR if this change is desired.

Pre-Review Checklist for the PR Author

1. Code follows the Rust coding style and is formatted with rustfmt
2. Branch follows the naming format (iox-123-this-is-a-branch)
3. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#42 commit text)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
4. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
5. Relevant issues are linked
6. Add sensible notes for the reviewer
7. All checks have passed (except task-list-completed)
8. Assign PR to reviewer

Notes for Reviewer

Checklist for the PR Reviewer

• Commits are properly organized and messages are according to the guideline
• Code according to our coding style and naming conventions
• PR title describes the changes

Post-review Checklist for the PR Author

1. All open points are addressed and tracked via issues

References

• Closes TBD

[phil-opp]

I'm not sure if GitHub sends out any notification for draft PRs, so cc @elBoberido.

[mossmaurice]

@phil-opp Thanks for the PR and the ping! @elBoberido is on vacation this week. He'll be back in CW41.

[phil-opp]

Thanks for the notice! There is no rush, I just wanted to make sure that this PR is noticed.

[elBoberido]

@phil-opp thanks for trying to improve the iceoryx Rust binding :)

May I ask for the use case you want to address?

I just skimmed over the PR and did not yet do a proper review. Is your intention to publish from multiple threads or just to be able to access the sample from multiple threads?

[elBoberido]

@phil-opp btw. I just realized who you are. I read a few of your blog posts. Great stuff :)

[phil-opp]

@elBoberido Thanks a lot :).

May I ask for the use case you want to address?

Our main use case is passing the sample over an FFI boundary to let a shared library fill in some data without copying. This is much safer with owned data because lifetimes are lost at the FFI boundary.

I just skimmed over the PR and did not yet do a proper review. Is your intention to publish from multiple threads or just to be able to access the sample from multiple threads?

The PR is just a first draft that copies most of the SampleMut code with minor modifications. The fundamental change is that the its publisher field is of type Arc<Publisher<T>> instead of &'a Publisher<T>. To get this new SampleMutArc type, one can use the new Publisher::loan_arc method, which requires that the self type is behind an Arc. This way, we can get an owned sample without any lifetimes, which can be passed through FFI or to other threads in a safer and easier way.

If such a reference-counted sample type is okay with you, I'll try to remove some of the duplication in the implementation.

[elBoberido]

@phil-opp so you basically need it for FFI. Would something similar to the untyped C++ API which uses void* fit your needs? I think it would be less work to have a unsafe into_raw method for SampleMut and additionally a publish method for raw pointer. What do you think about this?

[phil-opp]

Yeah, that would help for our use case! And it should be much simpler to implement.

I still think that a reference-counted sample type would be useful too. There are other non-FFI use cases that would profit from such a type. For example, consider an actor framework that communicates using mpsc channels. In such a system, an IPC actor could be responsible for communicating with other processes via iceoryx. Other actors could then request a sample from the IPC actor, fill in their data, and send the filled sample back to the IPC actor for publishing. This approach does not work with borrowed data, so it might make sense to add support for an owned/reference-counted sample type in the future.

[elBoberido]

@phil-opp good point. That's definitely a good use case but it is not as easy as it seems. The problem is that the underlying publisher is not thread safe which means loaning, publishing and releasing must happen from the same thread. While this is not a problem for loan and publish, it is a problem for release since the destructor of ref-counted sample type could call the publisher from any thread and corrupt the publisher. I was quite happy that this is not possible with the Rust binding but while typing this, I might have introduced this unsoundness in the subscriber. I have to take a look for that.

Anyway, I will think of a possible API for the raw pointer access. If you have an idea, don't hesitate to write a proposal.

Btw. tomorrow is our bi-weekly developer meetup. Would be great to have you there :)

Out of curiosity, can you reveal what you are using iceoryx for and are there other missing APIs for your use case?

[phil-opp]

The problem is that the underlying publisher is not thread safe which means loaning, publishing and releasing must happen from the same thread. While this is not a problem for loan and publish, it is a problem for release since the destructor of ref-counted sample type could call the publisher from any thread and corrupt the publisher.

Ah, that makes things more difficult. I guess I can close this PR then as it would result in undefined behavior.

Anyway, I will think of a possible API for the raw pointer access. If you have an idea, don't hesitate to write a proposal.

Thanks! Maybe a RawSample type that wraps a *mut T together with a std::thread::ThreadId would work? We could then create a unsafe publish_raw method that compares the thread ID to guard against publish operations from the wrong thread. We probably want to guard against publishing the sample on a different publisher too?

Btw. tomorrow is our bi-weekly developer meetup. Would be great to have you there :)

Ah sorry, I just read the notification email today...

Out of curiosity, can you reveal what you are using iceoryx for and are there other missing APIs for your use case?

We're currently evaluating iceoryx for the https://github.com/dora-rs/dora project. Our goal is to support multiple pub/sub backends to support different use cases. For example, we also support zenoh for distributed deployments. Iceoryx seems like a great candidate for local comunication.

I don't think that we're missing any other APIs right now. Waiting for multiple subscribers at once (#9) would be useful to us, but not critical.

[elBoberido]

Ah, that makes things more difficult. I guess I can close this PR then as it would result in undefined behavior.

Yes, I think this PR can't be merged and it would be easier to start from scratch than to try to refactor the current code.

Thanks! Maybe a RawSample type that wraps a *mut T together with a std::thread::ThreadId would work? We could then create a unsafe publish_raw method that compares the thread ID to guard against publish operations from the wrong thread. We probably want to guard against publishing the sample on a different publisher too?

Yes, this should work. I'm not sure if we need the thread ID. One could wrap the publisher in a mutex and then it would be save to publish from multiple threads. I have to think more about this. I like the idea with the RawSample though. I think that could be introduced in the iceoryx-sys crate. Currently raw pointer are used but I'm not happy about that. I wanted to use NotNull but some API calls I need are only in nightly available. The RawSample would fix this and could also be used in places I use a Box<T> which must be released into a raw pointer to not call drop. Ideally ´ManuallyDrop´ could be used but again, missing API.

The PR is not ideal for design discussions. I asked the eclipse helpdesk to enable the github Diskussions feature. I think that a bit better.

I just had another idea. Since loan and publish is already safe due to the type system, we would just need to make the release of a sample thread safe. This could be done with a multi pusher single consumer queue. When a sample is dropped, it would not directly call the publisher to release the underlying memory chunk but instead push it into the queue. The next time loan is called, the publisher could then release the sample or even reuse it since it would still be the owner. The memory would not be immediately released but this shouldn't be a big issue.

cc @elfenpiff maybe you also have some ideas?

Btw. tomorrow is our bi-weekly developer meetup. Would be great to have you there :)
Ah sorry, I just read the notification email today...

No problem. There is one every second week. If you are interested, we can put this topic on the agenda for next week.

We're currently evaluating iceoryx for the https://github.com/dora-rs/dora project. Our goal is to support multiple pub/sub backends to support different use cases. For example, we also support zenoh for distributed deployments. Iceoryx seems like a great candidate for local comunication.

Sound great. We are currently also thinking about getting in contact with the zenoh guys to explore if they are interested in using iceoryx in zenoh for local transport. If a day wouldn't be limited to just 24 hours 😅

I don't think that we're missing any other APIs right now. Waiting for multiple subscribers at once (#9) would be useful to us, but not critical.

Good to know. I already hacked something together but it's far from ready.

[phil-opp]

Yes, I think this PR can't be merged and it would be easier to start from scratch than to try to refactor the current code.

Ok, I'll close the PR then.

Ideally ´ManuallyDrop´ could be used but again, missing API.

Which API do you mean exactly?

The PR is not ideal for design discussions. I asked the eclipse helpdesk to enable the github Diskussions feature. I think that a bit better.

Looks like it's up: https://github.com/eclipse-iceoryx/iceoryx-rs/discussions. Should we create a discussion thread there?

I just had another idea. Since loan and publish is already safe due to the type system, we would just need to make the release of a sample thread safe. This could be done with a multi pusher single consumer queue. When a sample is dropped, it would not directly call the publisher to release the underlying memory chunk but instead push it into the queue. The next time loan is called, the publisher could then release the sample or even reuse it since it would still be the owner. The memory would not be immediately released but this shouldn't be a big issue.

Interesting idea! What would happen if the publisher is dropped before the sample? Then the queue would be closed already.

Sound great. We are currently also thinking about getting in contact with the zenoh guys to explore if they are interested in using iceoryx in zenoh for local transport. If a day wouldn't be limited to just 24 hours sweat_smile

That would be cool! I think they also have a custom shared memory implementation, e.g. used in this example. I'm not sure how their design compares to iceoryx, but it might be interesting to explore this further.

Good to know. I already hacked something together but it's far from ready.

Cool, let me know if I can do anything to help.

[elBoberido]

Ideally ´ManuallyDrop´ could be used but again, missing API.

Which API do you mean exactly?

Something like from_raw_parts which is missing in ManuallyDrop and only a nightly feature for NonNull but maybe I oversaw something with ManuallyDrop

Interesting idea! What would happen if the publisher is dropped before the sample? Then the queue would be closed already.

Right. This won't work as long as the lifetime of the sample is coupled to the publisher. There are ideas to couple it to the lifetime of the runtime and then this approach would work.

Good to know. I already hacked something together but it's far from ready.

Cool, let me know if I can do anything to help.

Currently I'm thinking if something similar to the C++ WaitSet/Listener would make sense or maybe to hook it into Rust async. I don't have any experience with async at the moment and would need to gain some knowledge there first. We can open a discussion for this as well to exchange some ideas.