potential deadlock when using event watch

[1bsyl]

@slouken

Open a new issue to discuss potential deadlock en using event watch.

76560f9 appears to need a generic fixes:
any joystick event delivery combined with an event watcher that locks joysticks

And, in fact this does not only affect joystick but also Touch Lock:

example:

SDLActivitiy thread:
SDL_SendTouchMotion():   SDL_LockTouch();  ..wait long  ... SDL_SendMouseMotion()

SDL Thread C:
event Watch ->  any API that does a SDL_LockTouch() will deadlock.

[1bsyl]

this can happen in any system with callbacks and two differents mutex accross two threads.
so either:

1/ remove threading
2/ force 1 mutex to be superior
3/ remove the need to use the mutex in on thread (then.. just remove the mutex!)
4/ remove callback ? or super-document that callback cannot use some api that lock other sub systems.

for Android:
1 / RPC command #14949 is somehow a way to remove threading

2 A / force a mutex to have priority
solution for joystick:

SDL_LockJoystick() {
   + LockEvent()
   LockJoystick()
}

SDL_UnlockJoystick() {
   UnlockJoystick()
   + UnlockEvent()
}

same for touch maybe:

SDL_LockTouch() {
   + LockEvent()
   LockTouch()
}

SDL_UnlockTouch() {
   UnlockTouch()
   + UnlockEvent()
}

2 B / this can also be done the otherway:

--- a/src/events/SDL_eventwatch.c
+++ b/src/events/SDL_eventwatch.c
@@ -57,6 +57,8 @@ bool SDL_DispatchEventWatchList(SDL_EventWatchList *list, SDL_Event *event)
     }

     SDL_LockMutex(list->lock);
+    SDL_LockTouch();
+    SDL_LockJoystick();
     {
         // Make sure we only dispatch the current watcher list
         int i, count = list->count;
@@ -86,6 +88,8 @@ bool SDL_DispatchEventWatchList(SDL_EventWatchList *list, SDL_Event *event)
             list->removed = false;
         }
     }
+    SDL_UnlockJoystick();
+    SDL_UnlockTouch();
     SDL_UnlockMutex(list->lock);

[1bsyl]

maybe we need to identifying what api allowing callbacks and take two locks consecutively ?

[slouken]

Thank you for writing up this issue. In addition to joystick and touch locking, this is needed for the sensor lock.

I was discussing this with @Packetdancer the other day, and I think the general solution is to make sure we don’t hold any locks during event delivery.

So the general sequence would be:

• lock subsystem
• start processing
• call SDL_Send*
• unlock subsystem
• SDL_PushEvent
• lock subsystem
• continue processing
• unlock subsystem

In the case of joysticks this can result in lots of luck/unlock traffic, so maybe the right solution is to have a preliminary queue for pending events and then finish processing by unlocking and then processing pending events?

[slouken]

Or maybe we just document that you shouldn't take any SDL subsystem locks from an event watcher function?

[1bsyl]

yes I agree maybe just document is better.
I may be wrong but EventWatch are not so much used with SDL Api ? so that would be adding lot of code to fix something unlikely

maybe an SDL_Assert somewhere would be possible ?! so that this can be always be caught ?

[1bsyl]

@slouken
example of assert ....
(this can be assert_release or not ...)

// using SDL_{Joystick/Touch}Lock is OK only if:
//   we are not in an event watcher
SDL_assert_release(     !  SDL_IsEventWatchDispatching()   );

also. I would expect that EventWatch are always executed in SDL Main Thread. (which is not the case!)
if it were true, this could be:

// using SDL_{Joystick/Touch}Lock is OK only if:
//  - we are not in main thread
//  or we aer not in an event watcher
SDL_assert_release(         ! SDL_IsMainThread()    || !    SDL_IsEventWatchDispatching()   );

diff --git a/src/events/SDL_events.c b/src/events/SDL_events.c
index e2ef1a35f..80d3de497 100644
--- a/src/events/SDL_events.c
+++ b/src/events/SDL_events.c
@@ -404,6 +404,11 @@ void SDL_FreeTemporaryMemory(void)
     }
 }

+bool SDL_IsEventWatchDispatching()
+{
+    return SDL_event_watchers.dispatching;
+}
+
 #ifndef SDL_JOYSTICK_DISABLED

 static bool SDL_update_joysticks = true;
diff --git a/src/events/SDL_eventwatch_c.h b/src/events/SDL_eventwatch_c.h
index 5c382a7ca..e56f77307 100644
--- a/src/events/SDL_eventwatch_c.h
+++ b/src/events/SDL_eventwatch_c.h
@@ -43,3 +43,4 @@ extern void SDL_QuitEventWatchList(SDL_EventWatchList *list);
 extern bool SDL_DispatchEventWatchList(SDL_EventWatchList *list, SDL_Event *event);
 extern bool SDL_AddEventWatchList(SDL_EventWatchList *list, SDL_EventFilter filter, void *userdata);
 extern void SDL_RemoveEventWatchList(SDL_EventWatchList *list, SDL_EventFilter filter, void *userdata);
+extern bool SDL_IsEventWatchDispatching();
diff --git a/src/events/SDL_touch.c b/src/events/SDL_touch.c
index b8ebb78ae..5a1205b4c 100644
--- a/src/events/SDL_touch.c
+++ b/src/events/SDL_touch.c
@@ -24,6 +24,7 @@

 #include "SDL_events_c.h"
 #include "../video/SDL_sysvideo.h"
+#include "../events/SDL_eventwatch_c.h"

 static SDL_Mutex *SDL_touch_lock = NULL; // This needs to support recursive locks
 static int SDL_touch_locked = 0;
@@ -56,6 +57,9 @@ bool SDL_InitTouch(void)
 static void SDL_LockTouch(void) SDL_ACQUIRE(SDL_touch_lock)
 {
     SDL_LockMutex(SDL_touch_lock);
+
+    // locking within an EventWatch is a risk of deadlock
+    SDL_assert_release(!SDL_IsEventWatchDispatching());
     ++SDL_touch_locked;
 }

[slouken]

I don't think we want this. It's perfectly fine to call SDL_LockTouch() from an event watcher that is dispatching a touch event, for example.

[1bsyl]

I don't think we want this. It's perfectly fine to call SDL_LockTouch() from an event watcher that is dispatching a touch event, for example.

but if it an event watcher from another king event, this can deadlock with

SDLActivitiy thread:
SDL_SendTouchMotion():   SDL_LockTouch();  ..wait long  ... SDL_SendMouseMotion()

[1bsyl]

for the record:

maybe some deadlock can also happen with HID:

HIDAPI_JoystickDisconnected() {
...
SDL_LockJoysticks()
...
SDL_PrivateJoystickRemoved() ...  SDL_PushEvent()

}