From 9275c533ca283778e3542985fdba4d2deb94f044 Mon Sep 17 00:00:00 2001 From: Sam Lantinga Date: Mon, 16 Sep 2024 22:45:37 -0700 Subject: [PATCH] Added thread-safe initialization/cleanup support Also went through and removed inappropriate uses of spinlocks. Fixes https://github.com/libsdl-org/SDL/issues/10872 --- src/SDL_log.c | 36 +++---- src/SDL_utils.c | 35 ++++++- src/SDL_utils_c.h | 18 ++++ src/audio/SDL_audioresample.c | 13 +-- src/core/linux/SDL_dbus.c | 91 +++++++++-------- src/joystick/hidapi/SDL_hidapijoystick.c | 28 ++++-- src/timer/SDL_timer.c | 119 +++++++++++++---------- 7 files changed, 208 insertions(+), 132 deletions(-) diff --git a/src/SDL_log.c b/src/SDL_log.c index f4fc429e82..aec4d6f00e 100644 --- a/src/SDL_log.c +++ b/src/SDL_log.c @@ -57,8 +57,7 @@ static void SDLCALL SDL_LogOutput(void *userdata, int category, SDL_LogPriority static void CleanupLogPriorities(void); static void CleanupLogPrefixes(void); -static SDL_AtomicInt SDL_log_initializing; -static SDL_AtomicInt SDL_log_initialized; +static SDL_InitState SDL_log_init; static SDL_Mutex *SDL_log_lock; static SDL_Mutex *SDL_log_function_lock; static SDL_LogLevel *SDL_loglevels SDL_GUARDED_BY(SDL_log_lock); @@ -128,16 +127,7 @@ static void SDLCALL SDL_LoggingChanged(void *userdata, const char *name, const c void SDL_InitLog(void) { - if (SDL_AtomicGet(&SDL_log_initialized)) { - return; - } - - // Do a little tap dance to make sure only one thread initializes logging - if (!SDL_AtomicCompareAndSwap(&SDL_log_initializing, false, true)) { - // Wait for the other thread to complete initialization - while (!SDL_AtomicGet(&SDL_log_initialized)) { - SDL_Delay(1); - } + if (!SDL_ShouldInit(&SDL_log_init)) { return; } @@ -147,13 +137,15 @@ void SDL_InitLog(void) SDL_AddHintCallback(SDL_HINT_LOGGING, SDL_LoggingChanged, NULL); - SDL_AtomicSet(&SDL_log_initializing, false); - - SDL_AtomicSet(&SDL_log_initialized, true); + SDL_SetInitialized(&SDL_log_init, true); } void SDL_QuitLog(void) { + if (!SDL_ShouldQuit(&SDL_log_init)) { + return; + } + SDL_RemoveHintCallback(SDL_HINT_LOGGING, SDL_LoggingChanged, NULL); CleanupLogPriorities(); @@ -167,15 +159,19 @@ void SDL_QuitLog(void) SDL_DestroyMutex(SDL_log_function_lock); SDL_log_function_lock = NULL; } - SDL_AtomicSet(&SDL_log_initialized, false); + + SDL_SetInitialized(&SDL_log_init, false); } -static void SDL_CheckInitLog() +static void SDL_CheckInitLog(void) { - if (!SDL_AtomicGet(&SDL_log_initialized) && - !SDL_AtomicGet(&SDL_log_initializing)) { - SDL_InitLog(); + int status = SDL_AtomicGet(&SDL_log_init.status); + if (status == SDL_INIT_STATUS_INITIALIZED || + (status == SDL_INIT_STATUS_INITIALIZING && SDL_log_init.thread == SDL_GetCurrentThreadID())) { + return; } + + SDL_InitLog(); } static void CleanupLogPriorities(void) diff --git a/src/SDL_utils.c b/src/SDL_utils.c index 0af6460e9e..c120a38eda 100644 --- a/src/SDL_utils.c +++ b/src/SDL_utils.c @@ -121,7 +121,40 @@ bool SDL_endswith(const char *string, const char *suffix) return false; } -// Assume we can wrap SDL_AtomicInt values and cast to Uint32 +bool SDL_ShouldInit(SDL_InitState *state) +{ + while (SDL_AtomicGet(&state->status) != SDL_INIT_STATUS_INITIALIZED) { + if (SDL_AtomicCompareAndSwap(&state->status, SDL_INIT_STATUS_UNINITIALIZED, SDL_INIT_STATUS_INITIALIZING)) { + state->thread = SDL_GetCurrentThreadID(); + return true; + } + + // Wait for the other thread to complete transition + SDL_Delay(1); + } + return false; +} + +bool SDL_ShouldQuit(SDL_InitState *state) +{ + if (SDL_AtomicCompareAndSwap(&state->status, SDL_INIT_STATUS_INITIALIZED, SDL_INIT_STATUS_UNINITIALIZING)) { + state->thread = SDL_GetCurrentThreadID(); + return true; + } + return false; +} + +void SDL_SetInitialized(SDL_InitState *state, bool initialized) +{ + SDL_assert(state->thread == SDL_GetCurrentThreadID()); + + if (initialized) { + SDL_AtomicSet(&state->status, SDL_INIT_STATUS_INITIALIZED); + } else { + SDL_AtomicSet(&state->status, SDL_INIT_STATUS_UNINITIALIZED); + } +} + SDL_COMPILE_TIME_ASSERT(sizeof_object_id, sizeof(int) == sizeof(Uint32)); Uint32 SDL_GetNextObjectID(void) diff --git a/src/SDL_utils_c.h b/src/SDL_utils_c.h index 500f804fe3..0a0cfef274 100644 --- a/src/SDL_utils_c.h +++ b/src/SDL_utils_c.h @@ -47,6 +47,24 @@ extern bool SDL_endswith(const char *string, const char *suffix); */ extern int SDL_URIToLocal(const char *src, char *dst); +typedef enum SDL_InitStatus +{ + SDL_INIT_STATUS_UNINITIALIZED, + SDL_INIT_STATUS_INITIALIZING, + SDL_INIT_STATUS_INITIALIZED, + SDL_INIT_STATUS_UNINITIALIZING +} SDL_InitStatus; + +typedef struct SDL_InitState +{ + SDL_AtomicInt status; + SDL_ThreadID thread; +} SDL_InitState; + +extern bool SDL_ShouldInit(SDL_InitState *state); +extern bool SDL_ShouldQuit(SDL_InitState *state); +extern void SDL_SetInitialized(SDL_InitState *state, bool initialized); + typedef enum { SDL_OBJECT_TYPE_UNKNOWN, diff --git a/src/audio/SDL_audioresample.c b/src/audio/SDL_audioresample.c index 79aa8851e6..07d0b4a638 100644 --- a/src/audio/SDL_audioresample.c +++ b/src/audio/SDL_audioresample.c @@ -574,16 +574,11 @@ static void SetupAudioResampler(void) void SDL_SetupAudioResampler(void) { - static SDL_SpinLock running = 0; + static SDL_InitState init; - if (!ResampleFrame[0]) { - SDL_LockSpinlock(&running); - - if (!ResampleFrame[0]) { - SetupAudioResampler(); - } - - SDL_UnlockSpinlock(&running); + if (SDL_ShouldInit(&init)) { + SetupAudioResampler(); + SDL_SetInitialized(&init, true); } } diff --git a/src/core/linux/SDL_dbus.c b/src/core/linux/SDL_dbus.c index 01032334a3..b13d27bfbb 100644 --- a/src/core/linux/SDL_dbus.c +++ b/src/core/linux/SDL_dbus.c @@ -122,60 +122,61 @@ static bool LoadDBUSLibrary(void) return result; } -static SDL_SpinLock spinlock_dbus_init = 0; +static SDL_InitState dbus_init; -// you must hold spinlock_dbus_init before calling this! -static void SDL_DBus_Init_Spinlocked(void) +void SDL_DBus_Init(void) { static bool is_dbus_available = true; + if (!is_dbus_available) { return; // don't keep trying if this fails. } - if (!dbus.session_conn) { - DBusError err; - - if (!LoadDBUSLibrary()) { - is_dbus_available = false; // can't load at all? Don't keep trying. - return; - } - - if (!dbus.threads_init_default()) { - is_dbus_available = false; - return; - } - - dbus.error_init(&err); - // session bus is required - - dbus.session_conn = dbus.bus_get_private(DBUS_BUS_SESSION, &err); - if (dbus.error_is_set(&err)) { - dbus.error_free(&err); - SDL_DBus_Quit(); - is_dbus_available = false; - return; // oh well - } - dbus.connection_set_exit_on_disconnect(dbus.session_conn, 0); - - // system bus is optional - dbus.system_conn = dbus.bus_get_private(DBUS_BUS_SYSTEM, &err); - if (!dbus.error_is_set(&err)) { - dbus.connection_set_exit_on_disconnect(dbus.system_conn, 0); - } - - dbus.error_free(&err); + if (!SDL_ShouldInit(&dbus_init)) { + return; } -} -void SDL_DBus_Init(void) -{ - SDL_LockSpinlock(&spinlock_dbus_init); // make sure two threads can't init at same time, since this can happen before SDL_Init. - SDL_DBus_Init_Spinlocked(); - SDL_UnlockSpinlock(&spinlock_dbus_init); + if (!LoadDBUSLibrary()) { + goto error; + } + + if (!dbus.threads_init_default()) { + goto error; + } + + DBusError err; + dbus.error_init(&err); + // session bus is required + + dbus.session_conn = dbus.bus_get_private(DBUS_BUS_SESSION, &err); + if (dbus.error_is_set(&err)) { + dbus.error_free(&err); + goto error; + } + dbus.connection_set_exit_on_disconnect(dbus.session_conn, 0); + + // system bus is optional + dbus.system_conn = dbus.bus_get_private(DBUS_BUS_SYSTEM, &err); + if (!dbus.error_is_set(&err)) { + dbus.connection_set_exit_on_disconnect(dbus.system_conn, 0); + } + + dbus.error_free(&err); + SDL_SetInitialized(&dbus_init, true); + return; + +error: + is_dbus_available = false; + SDL_SetInitialized(&dbus_init, true); + SDL_DBus_Quit(); } void SDL_DBus_Quit(void) { + if (!SDL_ShouldQuit(&dbus_init)) { + return; + } + if (dbus.system_conn) { dbus.connection_close(dbus.system_conn); dbus.connection_unref(dbus.system_conn); @@ -193,8 +194,12 @@ void SDL_DBus_Quit(void) SDL_zero(dbus); UnloadDBUSLibrary(); - SDL_free(inhibit_handle); - inhibit_handle = NULL; + if (inhibit_handle) { + SDL_free(inhibit_handle); + inhibit_handle = NULL; + } + + SDL_SetInitialized(&dbus_init, false); } SDL_DBusContext *SDL_DBus_GetContext(void) diff --git a/src/joystick/hidapi/SDL_hidapijoystick.c b/src/joystick/hidapi/SDL_hidapijoystick.c index b3c3a1263d..d1e5c1a937 100644 --- a/src/joystick/hidapi/SDL_hidapijoystick.c +++ b/src/joystick/hidapi/SDL_hidapijoystick.c @@ -87,7 +87,7 @@ static SDL_HIDAPI_DeviceDriver *SDL_HIDAPI_drivers[] = { #endif }; static int SDL_HIDAPI_numdrivers = 0; -static SDL_SpinLock SDL_HIDAPI_spinlock; +static SDL_AtomicInt SDL_HIDAPI_updating_devices; static bool SDL_HIDAPI_hints_changed = false; static Uint32 SDL_HIDAPI_change_count = 0; static SDL_HIDAPI_Device *SDL_HIDAPI_devices SDL_GUARDED_BY(SDL_joystick_lock); @@ -1243,6 +1243,16 @@ static bool HIDAPI_IsEquivalentToDevice(Uint16 vendor_id, Uint16 product_id, SDL return false; } +static bool HIDAPI_StartUpdatingDevices() +{ + return SDL_AtomicCompareAndSwap(&SDL_HIDAPI_updating_devices, false, true); +} + +static void HIDAPI_FinishUpdatingDevices() +{ + SDL_AtomicSet(&SDL_HIDAPI_updating_devices, false); +} + bool HIDAPI_IsDeviceTypePresent(SDL_GamepadType type) { SDL_HIDAPI_Device *device; @@ -1253,9 +1263,9 @@ bool HIDAPI_IsDeviceTypePresent(SDL_GamepadType type) return false; } - if (SDL_TryLockSpinlock(&SDL_HIDAPI_spinlock)) { + if (HIDAPI_StartUpdatingDevices()) { HIDAPI_UpdateDeviceList(); - SDL_UnlockSpinlock(&SDL_HIDAPI_spinlock); + HIDAPI_FinishUpdatingDevices(); } SDL_LockJoysticks(); @@ -1298,9 +1308,9 @@ bool HIDAPI_IsDevicePresent(Uint16 vendor_id, Uint16 product_id, Uint16 version, } #endif // SDL_JOYSTICK_HIDAPI_XBOX360 || SDL_JOYSTICK_HIDAPI_XBOXONE if (supported) { - if (SDL_TryLockSpinlock(&SDL_HIDAPI_spinlock)) { + if (HIDAPI_StartUpdatingDevices()) { HIDAPI_UpdateDeviceList(); - SDL_UnlockSpinlock(&SDL_HIDAPI_spinlock); + HIDAPI_FinishUpdatingDevices(); } } @@ -1360,13 +1370,13 @@ SDL_GamepadType HIDAPI_GetGamepadTypeFromGUID(SDL_GUID guid) static void HIDAPI_JoystickDetect(void) { - if (SDL_TryLockSpinlock(&SDL_HIDAPI_spinlock)) { + if (HIDAPI_StartUpdatingDevices()) { Uint32 count = SDL_hid_device_change_count(); if (SDL_HIDAPI_change_count != count) { SDL_HIDAPI_change_count = count; HIDAPI_UpdateDeviceList(); } - SDL_UnlockSpinlock(&SDL_HIDAPI_spinlock); + HIDAPI_FinishUpdatingDevices(); } } @@ -1379,7 +1389,7 @@ void HIDAPI_UpdateDevices(void) // Update the devices, which may change connected joysticks and send events // Prepare the existing device list - if (SDL_TryLockSpinlock(&SDL_HIDAPI_spinlock)) { + if (HIDAPI_StartUpdatingDevices()) { for (device = SDL_HIDAPI_devices; device; device = device->next) { if (device->parent) { continue; @@ -1393,7 +1403,7 @@ void HIDAPI_UpdateDevices(void) } } } - SDL_UnlockSpinlock(&SDL_HIDAPI_spinlock); + HIDAPI_FinishUpdatingDevices(); } } diff --git a/src/timer/SDL_timer.c b/src/timer/SDL_timer.c index 8c93a46f33..3a8e22bd9e 100644 --- a/src/timer/SDL_timer.c +++ b/src/timer/SDL_timer.c @@ -50,6 +50,7 @@ typedef struct SDL_TimerMap typedef struct { // Data used by the main thread + SDL_InitState init; SDL_Thread *thread; SDL_TimerMap *timermap; SDL_Mutex *timermap_lock; @@ -209,29 +210,35 @@ bool SDL_InitTimers(void) { SDL_TimerData *data = &SDL_timer_data; - if (!SDL_AtomicGet(&data->active)) { - const char *name = "SDLTimer"; - data->timermap_lock = SDL_CreateMutex(); - if (!data->timermap_lock) { - return false; - } - - data->sem = SDL_CreateSemaphore(0); - if (!data->sem) { - SDL_DestroyMutex(data->timermap_lock); - return false; - } - - SDL_AtomicSet(&data->active, 1); - - // Timer threads use a callback into the app, so we can't set a limited stack size here. - data->thread = SDL_CreateThread(SDL_TimerThread, name, data); - if (!data->thread) { - SDL_QuitTimers(); - return false; - } + if (!SDL_ShouldInit(&data->init)) { + return true; } + + data->timermap_lock = SDL_CreateMutex(); + if (!data->timermap_lock) { + goto error; + } + + data->sem = SDL_CreateSemaphore(0); + if (!data->sem) { + goto error; + } + + SDL_AtomicSet(&data->active, true); + + // Timer threads use a callback into the app, so we can't set a limited stack size here. + data->thread = SDL_CreateThread(SDL_TimerThread, "SDLTimer", data); + if (!data->thread) { + goto error; + } + + SDL_SetInitialized(&data->init, true); return true; + +error: + SDL_SetInitialized(&data->init, true); + SDL_QuitTimers(); + return false; } void SDL_QuitTimers(void) @@ -240,37 +247,52 @@ void SDL_QuitTimers(void) SDL_Timer *timer; SDL_TimerMap *entry; - if (SDL_AtomicCompareAndSwap(&data->active, 1, 0)) { // active? Move to inactive. - // Shutdown the timer thread - if (data->thread) { - SDL_SignalSemaphore(data->sem); - SDL_WaitThread(data->thread, NULL); - data->thread = NULL; - } + if (!SDL_ShouldQuit(&data->init)) { + return; + } + SDL_AtomicSet(&data->active, false); + + // Shutdown the timer thread + if (data->thread) { + SDL_SignalSemaphore(data->sem); + SDL_WaitThread(data->thread, NULL); + data->thread = NULL; + } + + if (data->sem) { SDL_DestroySemaphore(data->sem); data->sem = NULL; + } - // Clean up the timer entries - while (data->timers) { - timer = data->timers; - data->timers = timer->next; - SDL_free(timer); - } - while (data->freelist) { - timer = data->freelist; - data->freelist = timer->next; - SDL_free(timer); - } - while (data->timermap) { - entry = data->timermap; - data->timermap = entry->next; - SDL_free(entry); - } + // Clean up the timer entries + while (data->timers) { + timer = data->timers; + data->timers = timer->next; + SDL_free(timer); + } + while (data->freelist) { + timer = data->freelist; + data->freelist = timer->next; + SDL_free(timer); + } + while (data->timermap) { + entry = data->timermap; + data->timermap = entry->next; + SDL_free(entry); + } + if (data->timermap_lock) { SDL_DestroyMutex(data->timermap_lock); data->timermap_lock = NULL; } + + SDL_SetInitialized(&data->init, false); +} + +static bool SDL_CheckInitTimers(void) +{ + return SDL_InitTimers(); } static SDL_TimerID SDL_CreateTimer(Uint64 interval, SDL_TimerCallback callback_ms, SDL_NSTimerCallback callback_ns, void *userdata) @@ -284,14 +306,11 @@ static SDL_TimerID SDL_CreateTimer(Uint64 interval, SDL_TimerCallback callback_m return 0; } - SDL_LockSpinlock(&data->lock); - if (!SDL_AtomicGet(&data->active)) { - if (!SDL_InitTimers()) { - SDL_UnlockSpinlock(&data->lock); - return 0; - } + if (!SDL_CheckInitTimers()) { + return 0; } + SDL_LockSpinlock(&data->lock); timer = data->freelist; if (timer) { data->freelist = timer->next;