Refine modern pub/sub API

[elBoberido]

Originally posted by @budrus in #360 (comment)

How do we prevent this? With the old API people were more aware that they should not copy pointers that are coming from the middleware. In ara::com it's a unique ptr to make this even harder. I guess our Sample is also just an advanced unique_ptr but when I look at this code I fear that people think they must call sample.get() to be able to work with the payload. But in fact get() is the most dangerous method.

We need get for frameworks where you get a Sample from the iceoryx API and have to pass e.g. a void* to the next layer (like in ROS). Then this layer has to return the pointer and the sample must be released with the iceoryx API.
How would this look like? @elBoberido @ithier

• take a sample
• get the raw pointer with sample.get()
• release the sample for not releasing the chunk when it goes out of scope, sample.release()
• pass the raw pointer to the next layer
• something happens
• raw pointer is returned from the next layer
• create a Sample with the raw pointer
• let the Sample go out of scope for releasing the chunk

Shoud we make an example for this? This is a use case a lot people have. Would it be better to have a release() that releases the ownersip but additionally returns the pointer like in the STL and to remove the get() method? Having a get() that gives you a pointer to a shared memory chunk which is released when the Sample goes out of scope is a dangerous thing . The STL unique_ptr also has it but maybe people are more aware there

Here in iceperf we want to copy the payload. How can we do something like auto receivedSample = *(static_cast<const PerfTopic*>(receivedChunk));?

With the Sample class, the get method and the fact that the subscriber has no releaseChunk method any more, the lifetime and ownership question has some complexity. This must be well documented in the new API docu @elfenpiff @FerdinandSpitzschnueffler @MatthiasKillat

To-do:

• Add offerAndSubscribeOnCreate to PublisherOptions and enable it as default (will close example multi publisher, subscriber -> missing some data #575)
• Remove Sample from untyped API and make it feel similar to the C-API (releaseChunk called by user)
• Simplify the typed API, but not returning an expected but instead an optional and do the error handling for the user under the hood (fail fast & hard)
• Rename CaPro strings and add terminology table in overview.md
• Create a follow up issue that covers open points
  • define a payload_t or use void*?
  • using the header with the C API -> follow-up Chunk header usage in C #585
  • [x ]add a releaseChunk to the untyped publisher?

[elBoberido]

Original response by @elBoberido in #360 (comment)

This is a tough question and boils down to the raw pointer access like you pointed out. I'm not sure how to fix this, except making it harder to get to a non-owning raw pointer. e.g. by replacing get with dangerousNonOwningRawPointer and hope the user is too lazy to type that much, doesn't have auto-completion and is scared enough to try other things first.

The std::unique_ptr return a pointer with the release call, so this would be the solution when an owning pointer is necessary. For the non-owning use case this would open the door to chunk leaks, which we want to prevent in the first place. At first we need to know what the user wants to do with the raw pointer. If it's just storing for later use, then we have to prevent this and encourage to store the sample. If it's for copying, we should maybe enrich the Sample API, e.g.

T getPayloadCopy();
copyPayload(void* buffer, uint64_t bufferSize);

With this, we could get rid of the get method

[elBoberido]

Original response by @elfenpiff in #360 (comment)

I would clearly state in the docu: Please use the typed API but if you are an expert, you know what you are doing and you want to get your hands dirty then use the untyped one. With this statement and the assumption that the users know what they are doing we could get rid of the Sample and offer an interface which looks like this:

untypedSubscriber->take().and_then([&](const void * payload){
     auto header = freeFunctionToAcquireHeaderPtr(payload);
     doStuff(payload, header);
  
     untypedSubscriber->releaseRawSample(payload);
});

You could also pack this payload and header pointer into a Sample but then the user is maybe mislead to believe that the sample takes care of the release but when having a void* you clearly have to take care of this.

Maybe it is also then useful to not recommend the functional approach and stick to the classic approach and use it like:

auto payload = untypedSubscriber->take();
if ( !payload.has_value() ) {
    doStuff(payload.value()); // acquires void *
    untypedSubscriber->releaseRawSample(*payload);
}

[elBoberido]

From a different PR posted by @elBoberido in #403 (comment)

@ithier what do you think about adjusting the Sample::get method to this

template <typename T>
class Sample {
public:
    template <typename U = T>
    U* get();

private:
    cxx::unique_ptr<T> m_samplePtr{[](T* const) {}};
};

template <typename T>
template <typename U>
U* Sample<T>::get() {
    static_assert(std::is_same<U, T>::value, "incompatible types");
    return m_samplePtr.get();
}

template <>
template <typename U>
U* Sample<void>::get() {
    return static_cast<U*>(m_samplePtr.get(););
}

then we could have

Sample<void> s;
auto untypedPointer = s.get();
auto typedPointer = s.get<Foo>();

[budrus]

Thanks @elBoberido for creating this follow up issue

[dkroenke]

Prasanna have a PR with the adress sanitizer enabled and it's throwing an interesting error when using the publisher
Here the testcode:

     auto result = sut.publishResultOf([](DummyData* allocation) {
     auto data = new (allocation) DummyData();
     data->val = 777;

This is giving the following error:
==51920==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60b0000676c7 at pc 0x0000004eae00 bp 0x7fff5b8d44e0 sp 0x7fff5b8d3c90 WRITE of size 8 at 0x60b0000676c7 thread T0

According to the code the publishResultOf is calling loan which is creating the sample with plamenent new.

inline cxx::expected<AllocationError> TypedPublisher<T, base_publisher_t>::publishResultOf(Callable c,
                                                                                           ArgTypes... args)
{
    static_assert(....)

    return loan().and_then([&](Sample<T>& sample) {
        c(sample.get(), std::forward<ArgTypes>(args)...);
        sample.publish();
    });
}

inline cxx::expected<Sample<T>, AllocationError> TypedPublisher<T, base_publisher_t>::loan() noexcept
{
    // Call default constructor here to ensure the type is immediately ready to use by the caller.
    // There is a risk that the type will be re-constructed by the user (e.g. by using a placement new in
    // publishResultOf(), however the overhead is considered to be insignificant and worth the additional safety.
    return std::move(base_publisher_t::loan(sizeof(T)).and_then([](Sample<T>& sample) { new (sample.get()) T(); }));
}

So we have two places now with placement new and it seems that the one in loan is somehow then overoverwriting the one from the lambda expression. The comment in loan is somehow already assuming it.

[elBoberido]

So we have two places now with placement new and it seems that the one in loan is somehow then overoverwriting the one from the lambda expression. The comment in loan is somehow already assuming it.

How about discouraging the usage of placement new in user code. We could just have a Sample::emplace() method which takes care of this. So if the sample already manages an object, it can call the destructor of the object.

Similarly

     auto result = sut.publishResultOf([](DummyData* allocation) {
     auto data = new (allocation) DummyData();
     data->val = 777;

would become

     auto result = sut.publishResultOf([](Sample<DummyData> sample) {
     sample.emplace(DummyData);
     sample->val = 777;

[budrus]

posted by @MatthiasKillat in #421 comment

Actually, it is not quite as easy with type inference in play like this due to overloads of and_then I think... I will look at the reason, but the call *sample.get() will not work anymore with auto since it is now detected to be an optional of sample, which is not what it should be (incorrect monadic behavior in my book). Now, changing the call to account for this seems to not play nicely with auto and it breaks.

Will need a closer look.
Message: the nested optionals samples and optionals and different overloads for and_then (some incorrect I think) are not working nicely to create an easy user experience.

[budrus]

Here a list of things I stumbled upon. We have to discuss what needs to be done an what maybe not

• capability to configure history is missing in new cpp publishers and subscribers

• there is the modernAPI folder, is this now obsolete and we move the content to posh/include

• I would prefer having the queue capacity in the Subscriber c'tor and not in the subscribe call

[MatthiasKillat]

@elBoberido I agree, emplacement new on the user side is a bad idea. Especially if there is already something constructed at this memory location (which would need to have a destructor called manually).
If we allow this, it should only be via void* I think (not a typed pointer) and if there is already something constructed there we should pass a reference instead.

My reasoning is that as a user I have a harder time to know whether the pointer points to just memory (even if typed) or to an already constructed object. With references I assume it is a valid sample. You can always circumvent this and mislead the user and have dangling references and, but this is bad style.

But we need something with at least the void pointer for interaction with low level APIs.

Accessing the data via the Sample::get method is also not ideal, since a raw pointer is exposed. I was also playing around with the idea of adding T& Sample::operator*, but this is also not straightforward since T can be void, so (quite) a bit template magic is needed (i used SFINAE, there may be an easier way except fully specializing for void) and it just gives a another way to access the data with the same problem.

Anyway, a true monadic and_then must work with the unpacked base type, but what is the unpacked type here? Consider the following:
auto result = subscriber.take();
Result is now something like a monad (not quite but good enough for us here) with some packed type T internally. It most likely should not just be an expected, since this is not specialized enough for the use case I think. Whatever payload type T it augments, I want to get a reference in the function I call in the in the and_then.
auto f = [](T& data) { ... };
It is not useful to have an optional here, we should be able to guarantee that when we are in the and_then case, that we actually have data. Now it is also clear that f works on a reference, it may of course copy it internally, but it neither does own it nor is it responsible for its destruction.

Everything else belongs to different cases.
Now I can call
result.with_data(f).if_empty(g).or_else(h);
with suitable g and h, with both if_empty optional and or_else being optional and the latter being the catch all for the remaining cases when there is an error but. I would not really like to have the if_empty be combined with the `or_else``, since it is no error case.

This is just a rough sketch of how a monadic interface should probably work if we want such an interface. We are also missing something convenient to get all available samples. Currently we need a loop for that, but I propose some function such as
auto numSamplesOrError = subscriber.takeAll(container);
Or even
auto numSamplesOrError = subscriber.takeAll(takeHandler);
I do not advocate returning the container, since it must be constructed every time and must be large enough to hold the maximum samples etc. I would leave it to the user to provide the container or the handler function.

An important question is whether the data should be copied into the container. Ideally ownership is passed somehow, without involving the copy. The handler can specify this on its own. This has the advantage

Just a rough sketch and it is hard to say what is best, but the current interface can be improved. To get a better feel for it I have to use it more.