Listener addEvent deadlock

[niclar]

Hi we encounter a deadlock in the Listener on b184113 in the follow situation.

We have main thread (M) and an iceoryx listener thread (L) with the following execution trace 1..4;

1(M): M creates a new iceoryx client (C1) and call Listener::attatchEvent and then send a request (Req1).
2(L): We get the reply (Req1) and go into the ReplyHandler (H1). It recursive lock m_events[2].
3(M): M creates a new iceoryx client (C2) and call Listener::attatchEvent, it locks Listener::m_addEventMutex and block on m_events[2]
4(L): H1 creates a new iceoryx client(C3) and call Listener::attatchEvent, it blocks on Listener::m_addEventMutex ==> DEADLOCK

By judging from the attatchEvent comments ("This method can be called from any thread concurrently without any restrictions!") this is an iceoryx bug.

Please advice

Best regards

[elBoberido]

It is not related to the deadlock but after attaching something to the Listener one cannot use it anymore outside of the listener to send/receive data until it is detached. The reason for this is that neither of the endpoint instances are thread safe.

Regarding the deadlock. It looks like there is indeed a case were a deadlock can happen due to the check whether the event is already attached.

legend: ----> function calls from main thread
        ~~~~> function calls from listener thread

main thread      Listener                  m_events[0..3]     eventAddMutex  
    |               |                           ||||                |
    |               |                           ||||                |
    |               |\ listener thread          ||||                |
    |               | \                         ||||                |
    |               |  \                        ||||                |
    |               |  | executeCallback(id=2)  ||||                |
    |               |  ▬~~~~~~~~~~~~~~~~~~~~~~~~~>X|                |
    |               |  ▬<~~~~~~~~~~~~~~~~~~~~~~~~~X|                |
    |               |  ▬                        ||X|                |
    | attach        |  ▬                        ||X|                |
    ▬-------------->| lock                      ||X|                |
    ▬               ▬---------------------------------------------->X
    ▬               ▬<----------------------------------------------X
    ▬               ▬ isEqualTo(id=0)           ||X|                X
    ▬               ▬-------------------------->X|X|                X
    ▬               ▬<--------------------------X|X|                X
    ▬               ▬ isEqualTo(id=1)           ||X|                X
    ▬               ▬--------------------------->XX|                X
    ▬               ▬<---------------------------XX|                X
    ▬               ▬ isEqualTo(id=2)           ||X|                X
    ▬               ▬---------------------------->X|⚡already locked by listener thread
    ▬               ▬  ▬                        ||X|                X
    ▬               ▬  ▬ attach                 ||X|                X
    ▬               ▬  ▬~~~~~~~~~~              ||X|                X
    ▬               ▬  ▬         ~              ||X|                X
    ▬               ▬  ▬▬<~~~~~~~~              ||X|                X
    ▬               ▬  ▬▬ lock                  ||X|                X
    ▬               ▬  ▬▬~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~>X⚡already locked by main thread
    ▬               ▬  ▬▬                       ||X|                X
    ▬               ▬  ▬▬                       ||X|                X

I'm not that familiar with the Listener but maybe a custom event could be added which then adds the clients from within the listener thread.

Unfortunately we cannot use try_lock in this case since it can spuriously fail and it is undefined behavior to call it from a thread that holds the mutex. To fix this we would need to implement a lock-free check for the isEqualTo functionality.

Edit:
Maybe this can be solved by std::shared_mutex with a std::shared_lock in isEqualTo. The executeCallback method requires an additional mutex and in init and reset both mutex need to be owned exclusively.

I'm not sure if there is another issue though. When an event is detached from within the listener thread, it might lead to another deadlock or a race but I have to think more about it. The problem is that the slot is marked as free while the callback is still running and if an event is attached in the callback right after the detach it might get the same slot.

@elfenpiff @budrus @MatthiasKillat you are more familiar with the listener. What do you think about my proposal?