From eb1be89e70106aa45fb2c81192fd078b285e2f52 Mon Sep 17 00:00:00 2001
From: Sam Lantinga <[EMAIL REDACTED]>
Date: Thu, 26 Dec 2024 09:28:01 -0800
Subject: [PATCH] Cleaned up thread state handling and added thread object
validation
---
include/SDL3/SDL_thread.h | 25 +++++++--------
src/SDL_utils.c | 3 ++
src/SDL_utils_c.h | 1 +
src/thread/SDL_thread.c | 55 ++++++++++++++++++++++-----------
src/thread/SDL_thread_c.h | 2 +-
src/thread/n3ds/SDL_systhread.c | 2 +-
6 files changed, 54 insertions(+), 34 deletions(-)
diff --git a/include/SDL3/SDL_thread.h b/include/SDL3/SDL_thread.h
index 6bac1e402196f..7b7e6d5465699 100644
--- a/include/SDL3/SDL_thread.h
+++ b/include/SDL3/SDL_thread.h
@@ -105,8 +105,7 @@ typedef enum SDL_ThreadPriority {
/**
* The SDL thread state.
*
- * SDL stores the current state of a thread in an atomic int. The current
- * state of a thread can be checked by calling SDL_GetThreadState.
+ * The current state of a thread can be checked by calling SDL_GetThreadState.
*
* \since This enum is available since SDL 3.1.3.
*
@@ -114,10 +113,10 @@ typedef enum SDL_ThreadPriority {
*/
typedef enum SDL_ThreadState
{
- SDL_THREAD_STATE_ALIVE,
- SDL_THREAD_STATE_DETACHED,
- SDL_THREAD_STATE_ZOMBIE,
- SDL_THREAD_STATE_CLEANED,
+ SDL_THREAD_UNKNOWN, /**< The thread is not valid */
+ SDL_THREAD_ALIVE, /**< The thread is currently running */
+ SDL_THREAD_DETACHED, /**< The thread is detached and can't be waited on */
+ SDL_THREAD_COMPLETE, /**< The thread has finished and should be cleaned up with SDL_WaitThread() */
} SDL_ThreadState;
/**
@@ -408,14 +407,14 @@ extern SDL_DECLSPEC bool SDLCALL SDL_SetCurrentThreadPriority(SDL_ThreadPriority
/**
* Wait for a thread to finish.
*
- * Threads that haven't been detached will remain (as a "zombie") until this
+ * Threads that haven't been detached will remain until this
* function cleans them up. Not doing so is a resource leak.
*
* Once a thread has been cleaned up through this function, the SDL_Thread
* that references it becomes invalid and should not be referenced again. As
* such, only one thread may call SDL_WaitThread() on another.
*
- * The return code for the thread function is placed in the area pointed to by
+ * The return code from the thread function is placed in the area pointed to by
* `status`, if `status` is not NULL.
*
* You may not wait on a thread that has been used in a call to
@@ -429,9 +428,8 @@ extern SDL_DECLSPEC bool SDLCALL SDL_SetCurrentThreadPriority(SDL_ThreadPriority
*
* \param thread the SDL_Thread pointer that was returned from the
* SDL_CreateThread() call that started this thread.
- * \param status pointer to an integer that will receive the value returned
- * from the thread function by its 'return', or NULL to not
- * receive such value back.
+ * \param status a pointer filled in with the value returned
+ * from the thread function by its 'return', or -1 if the thread has been detached or isn't valid, may be NULL.
*
* \since This function is available since SDL 3.1.3.
*
@@ -443,9 +441,8 @@ extern SDL_DECLSPEC void SDLCALL SDL_WaitThread(SDL_Thread *thread, int *status)
/**
* Get the current state of a thread.
*
- * \param thread the thread whose status you want to check.
- * \returns the current state of a thread as defined in the SDL_ThreadState
- * enum.
+ * \param thread the thread to query.
+ * \returns the current state of a thread, or SDL_THREAD_UNKNOWN if the thread isn't valid.
*
* \since This function is available since SDL 3.2.0.
*
diff --git a/src/SDL_utils.c b/src/SDL_utils.c
index 5487b9d567ee9..d128e495fbc55 100644
--- a/src/SDL_utils.c
+++ b/src/SDL_utils.c
@@ -214,6 +214,9 @@ void SDL_SetObjectsInvalid(void)
case SDL_OBJECT_TYPE_HIDAPI_JOYSTICK:
type = "hidapi joystick";
break;
+ case SDL_OBJECT_TYPE_THREAD:
+ type = "thread";
+ break;
default:
type = "unknown object";
break;
diff --git a/src/SDL_utils_c.h b/src/SDL_utils_c.h
index adb150a92271d..65f2c6216898a 100644
--- a/src/SDL_utils_c.h
+++ b/src/SDL_utils_c.h
@@ -60,6 +60,7 @@ typedef enum
SDL_OBJECT_TYPE_SENSOR,
SDL_OBJECT_TYPE_HIDAPI_DEVICE,
SDL_OBJECT_TYPE_HIDAPI_JOYSTICK,
+ SDL_OBJECT_TYPE_THREAD,
} SDL_ObjectType;
diff --git a/src/thread/SDL_thread.c b/src/thread/SDL_thread.c
index fcb11019c14fc..67183ac3baa09 100644
--- a/src/thread/SDL_thread.c
+++ b/src/thread/SDL_thread.c
@@ -306,6 +306,11 @@ SDL_error *SDL_GetErrBuf(bool create)
#endif // SDL_THREADS_DISABLED
}
+static bool ThreadValid(SDL_Thread *thread)
+{
+ return SDL_ObjectValid(thread, SDL_OBJECT_TYPE_THREAD);
+}
+
void SDL_RunThread(SDL_Thread *thread)
{
void *userdata = thread->userdata;
@@ -326,9 +331,10 @@ void SDL_RunThread(SDL_Thread *thread)
SDL_CleanupTLS();
// Mark us as ready to be joined (or detached)
- if (!SDL_CompareAndSwapAtomicInt(&thread->state, SDL_THREAD_STATE_ALIVE, SDL_THREAD_STATE_ZOMBIE)) {
+ if (!SDL_CompareAndSwapAtomicInt(&thread->state, SDL_THREAD_ALIVE, SDL_THREAD_COMPLETE)) {
// Clean up if something already detached us.
- if (SDL_CompareAndSwapAtomicInt(&thread->state, SDL_THREAD_STATE_DETACHED, SDL_THREAD_STATE_CLEANED)) {
+ if (SDL_GetThreadState(thread) == SDL_THREAD_DETACHED) {
+ SDL_SetObjectValid(thread, SDL_OBJECT_TYPE_THREAD, false);
SDL_free(thread->name); // Can't free later, we've already cleaned up TLS
SDL_free(thread);
}
@@ -364,7 +370,7 @@ SDL_Thread *SDL_CreateThreadWithPropertiesRuntime(SDL_PropertiesID props,
return NULL;
}
thread->status = -1;
- SDL_SetAtomicInt(&thread->state, SDL_THREAD_STATE_ALIVE);
+ SDL_SetAtomicInt(&thread->state, SDL_THREAD_ALIVE);
// Set up the arguments for the thread
if (name) {
@@ -379,9 +385,12 @@ SDL_Thread *SDL_CreateThreadWithPropertiesRuntime(SDL_PropertiesID props,
thread->userdata = userdata;
thread->stacksize = stacksize;
+ SDL_SetObjectValid(thread, SDL_OBJECT_TYPE_THREAD, true);
+
// Create the thread and go!
if (!SDL_SYS_CreateThread(thread, pfnBeginThread, pfnEndThread)) {
// Oops, failed. Gotta free everything
+ SDL_SetObjectValid(thread, SDL_OBJECT_TYPE_THREAD, false);
SDL_free(thread->name);
SDL_free(thread);
thread = NULL;
@@ -420,10 +429,12 @@ SDL_Thread *SDL_CreateThreadWithStackSize(SDL_ThreadFunction fn, const char *nam
SDL_ThreadID SDL_GetThreadID(SDL_Thread *thread)
{
- SDL_ThreadID id;
+ SDL_ThreadID id = 0;
if (thread) {
- id = thread->threadid;
+ if (ThreadValid(thread)) {
+ id = thread->threadid;
+ }
} else {
id = SDL_GetCurrentThreadID();
}
@@ -432,7 +443,7 @@ SDL_ThreadID SDL_GetThreadID(SDL_Thread *thread)
const char *SDL_GetThreadName(SDL_Thread *thread)
{
- if (thread) {
+ if (ThreadValid(thread)) {
return SDL_GetPersistentString(thread->name);
} else {
return NULL;
@@ -446,39 +457,47 @@ bool SDL_SetCurrentThreadPriority(SDL_ThreadPriority priority)
void SDL_WaitThread(SDL_Thread *thread, int *status)
{
- if (thread) {
- SDL_SYS_WaitThread(thread);
+ if (!ThreadValid(thread) || SDL_GetThreadState(thread) == SDL_THREAD_DETACHED) {
if (status) {
- *status = thread->status;
+ *status = -1;
}
- SDL_free(thread->name);
- SDL_free(thread);
+ return;
+ }
+
+ SDL_SYS_WaitThread(thread);
+ if (status) {
+ *status = thread->status;
}
+ SDL_SetObjectValid(thread, SDL_OBJECT_TYPE_THREAD, false);
+ SDL_free(thread->name);
+ SDL_free(thread);
}
SDL_ThreadState SDL_GetThreadState(SDL_Thread *thread)
{
+ if (!ThreadValid(thread)) {
+ return SDL_THREAD_UNKNOWN;
+ }
+
return (SDL_ThreadState)SDL_GetAtomicInt(&thread->state);
}
void SDL_DetachThread(SDL_Thread *thread)
{
- if (!thread) {
+ if (!ThreadValid(thread)) {
return;
}
// Grab dibs if the state is alive+joinable.
- if (SDL_CompareAndSwapAtomicInt(&thread->state, SDL_THREAD_STATE_ALIVE, SDL_THREAD_STATE_DETACHED)) {
+ if (SDL_CompareAndSwapAtomicInt(&thread->state, SDL_THREAD_ALIVE, SDL_THREAD_DETACHED)) {
SDL_SYS_DetachThread(thread);
} else {
// all other states are pretty final, see where we landed.
- const int thread_state = SDL_GetAtomicInt(&thread->state);
- if ((thread_state == SDL_THREAD_STATE_DETACHED) || (thread_state == SDL_THREAD_STATE_CLEANED)) {
+ SDL_ThreadState thread_state = SDL_GetThreadState(thread);
+ if (thread_state == SDL_THREAD_DETACHED) {
return; // already detached (you shouldn't call this twice!)
- } else if (thread_state == SDL_THREAD_STATE_ZOMBIE) {
+ } else if (thread_state == SDL_THREAD_COMPLETE) {
SDL_WaitThread(thread, NULL); // already done, clean it up.
- } else {
- SDL_assert(0 && "Unexpected thread state");
}
}
}
diff --git a/src/thread/SDL_thread_c.h b/src/thread/SDL_thread_c.h
index f8b3fb8953f5e..5620e7558b72f 100644
--- a/src/thread/SDL_thread_c.h
+++ b/src/thread/SDL_thread_c.h
@@ -50,7 +50,7 @@ struct SDL_Thread
SDL_ThreadID threadid;
SYS_ThreadHandle handle;
int status;
- SDL_AtomicInt state; /* SDL_THREAD_STATE_* */
+ SDL_AtomicInt state; /* SDL_ThreadState */
SDL_error errbuf;
char *name;
size_t stacksize; // 0 for default, >0 for user-specified stack size.
diff --git a/src/thread/n3ds/SDL_systhread.c b/src/thread/n3ds/SDL_systhread.c
index a535cfd0951cf..a0aa3f6c742c7 100644
--- a/src/thread/n3ds/SDL_systhread.c
+++ b/src/thread/n3ds/SDL_systhread.c
@@ -126,7 +126,7 @@ void SDL_SYS_WaitThread(SDL_Thread *thread)
Detached threads can be waited on, but should NOT be cleaned manually
as it would result in a fatal error.
*/
- if (R_SUCCEEDED(res) && SDL_GetAtomicInt(&thread->state) != SDL_THREAD_STATE_DETACHED) {
+ if (R_SUCCEEDED(res) && SDL_GetThreadState(thread) != SDL_THREAD_DETACHED) {
threadFree(thread->handle);
}
}