From 9275c533ca283778e3542985fdba4d2deb94f044 Mon Sep 17 00:00:00 2001
From: Sam Lantinga <[EMAIL REDACTED]>
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 | 81 ++++++++--------
src/joystick/hidapi/SDL_hidapijoystick.c | 28 ++++--
src/timer/SDL_timer.c | 113 +++++++++++++----------
7 files changed, 200 insertions(+), 124 deletions(-)
diff --git a/src/SDL_log.c b/src/SDL_log.c
index f4fc429e82a2a..aec4d6f00ec1b 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 0af6460e9e049..c120a38eda899 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 500f804fe372a..0a0cfef274e28 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 79aa8851e6194..07d0b4a638358 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 01032334a31ec..b13d27bfbba60 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;
- }
+ if (!SDL_ShouldInit(&dbus_init)) {
+ return;
+ }
- dbus.error_init(&err);
- // session bus is required
+ if (!LoadDBUSLibrary()) {
+ goto error;
+ }
- 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);
+ if (!dbus.threads_init_default()) {
+ goto error;
+ }
- // 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);
- }
+ 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);
-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);
+ // 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 b3c3a1263ddf7..d1e5c1a937ced 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 8c93a46f33bc8..3a8e22bd9e8b4 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;
- }
+ if (!SDL_ShouldInit(&data->init)) {
+ return true;
+ }
- data->sem = SDL_CreateSemaphore(0);
- if (!data->sem) {
- SDL_DestroyMutex(data->timermap_lock);
- return false;
- }
+ data->timermap_lock = SDL_CreateMutex();
+ if (!data->timermap_lock) {
+ goto error;
+ }
- SDL_AtomicSet(&data->active, 1);
+ data->sem = SDL_CreateSemaphore(0);
+ if (!data->sem) {
+ goto error;
+ }
- // 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;
- }
+ 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;