iceoryx icon indicating copy to clipboard operation
iceoryx copied to clipboard

Listener causes easily race conditions

Open elfenpiff opened this issue 2 years ago • 3 comments

Required information

When using the listener it starts a background thread and operates on the attached objects concurrently whenever the specified event occurs. This causes often the same bug when it is being used - race conditions since the background thread is well hidden from the user and they are not aware that they may operate on the object concurrently. And most objects are not thread safe.

To fix this API bug causing issue I would propose an API change on the listener so that the listener takes the ownership of an object when it is being attached and release it when it is detached.

// returns some kind of handle to the attached event
cxx::expected<Handle, ListenerError> attachEvent(T&& eventOrigin, // <- takes the ownership of the attached object
                                             const EventType eventType,
                                             const NotificationCallback<T, ContextDataType>& eventCallback) noexcept;

// returns the ownership of the attached object
T&& detachEvent(Handle eventHandle, const EventType eventType) noexcept;

This would solve the problem and cause another problem. Then the listener must provide enough memory so that arbitrary objects can be attached to it and this makes it to a memory hungry monster.

Does anyone has some inspirational idea how we can fix the memory part? @elBoberido @MatthiasKillat @FerdinandSpitzschnueffler

elfenpiff avatar Feb 17 '22 08:02 elfenpiff

@elfenpiff No idea right now, will think about it later. However, if the listener has ownership of the object it cannot be used directly anymore? This is not good, especially as the object might be thread-safe and this would be no problem for the user.

I would be fine with the user being informed that he needs to use thread-safe calls only in the callback. I am not sure there is a nice solution which does not impair usability greatly. If there was a simple way to make multi-threading fool-proof it would be well-known by now ....

MatthiasKillat avatar Feb 17 '22 10:02 MatthiasKillat

If there was a simple way to make multi-threading fool-proof it would be well-known by now ....

It's called Rust ;) ... well, maybe not fully fool proof but it comes close

elBoberido avatar Feb 17 '22 12:02 elBoberido

@elBoberido I am still learning Rust so I am not qualified to judge. However, you could also eliminate many problems by essentially serializing all calls (hello synchronized... ). But this defeats (much of) the purpose of having a multi-processor architecture ...

Moving ownership around kind of goes in this direction (it is a compromise).

MatthiasKillat avatar Feb 18 '22 08:02 MatthiasKillat