Thread creation and destruction is now handled by the main loop.

Some modifications to the initialization code were also necessary
to keep the system from deadlocking itself immediately; only subthreads
may now call CreateThread or AssignTask.

All threads are guaranteed to get a WaitThread () on them when they're
finished now, which hopefully will stop thread ID leaking (bug #561).


git-svn-id: svn://svn.code.sf.net/p/sc2/code/trunk@1311 8092fc87-c524-0410-9efc-e669fe64eaf9

sc2/ChangeLog

@@ -1,4 +1,7 @@
 Changes towards version 0.4:
+- Thread code refactoring: only the main thread will actually spawn 
+  threads, and thread IDs are properly recycled with SDL_WaitThead () 
+  once they're done.  (With luck, this will fix bug #561) -Michael
 - Sound code refactoring: core api is now virtualized,
   MixSDL is divided to generic mixer and driver entities - Mika
 - Optimized MixSDL mixing and resampling routines

sc2/src/sc2code/libs/threadlib.h

@@ -77,6 +77,11 @@ enum
 	SYNC_CLASS_RESOURCE = (1 << 3)     /* Involves system resources (_MemoryLock) */
 };
 
+/* Note.  NEVER call CreateThread from the main thread, or deadlocks
+   are guaranteed.  Use StartThread instead (which doesn't wait around
+   for the main thread to actually create the thread and return
+   it). */
+
 #ifdef NAMED_SYNCHRO
 /* Logical OR of all classes we want to track. */
 #define TRACK_CONTENTION_CLASSES (SYNC_CLASS_TOPLEVEL)
@@ -84,6 +89,7 @@ enum
 /* Prototypes with the "name" field */
 
 Thread CreateThread_Core (ThreadFunction func, void *data, SDWORD stackSize, const char *name);
+void StartThread_Core (ThreadFunction func, void *data, SDWORD stackSize, const char *name);
 Semaphore CreateSemaphore_Core (DWORD initial, const char *name, DWORD syncClass);
 Mutex CreateMutex_Core (const char *name, DWORD syncClass);
 RecursiveMutex CreateRecursiveMutex_Core (const char *name, DWORD syncClass);
@@ -93,6 +99,8 @@ CondVar CreateCondVar_Core (const char *name, DWORD syncClass);
 
 #define CreateThread(func, data, stackSize, name) \
 	CreateThread_Core ((func), (data), (stackSize), (name))
+#define StartThread(func, data, stackSize, name) \
+	StartThread_Core ((func), (data), (stackSize), (name))
 #define CreateSemaphore(initial, name, syncClass) \
 	CreateSemaphore_Core ((initial), (name), (syncClass))
 #define CreateMutex(name, syncClass) \
@@ -106,6 +114,7 @@ CondVar CreateCondVar_Core (const char *name, DWORD syncClass);
 
 /* Prototypes without the "name" field. */
 Thread CreateThread_Core (ThreadFunction func, void *data, SDWORD stackSize);
+void StartThread_Core (ThreadFunction func, void *data, SDWORD stackSize);
 Semaphore CreateSemaphore_Core (DWORD initial);
 Mutex CreateMutex_Core (void);
 RecursiveMutex CreateRecursiveMutex_Core (void);
@@ -117,6 +126,8 @@ CondVar CreateCondVar_Core (void);
 
 #define CreateThread(func, data, stackSize, name) \
 	CreateThread_Core ((func), (data), (stackSize))
+#define StartThread(func, data, stackSize, name) \
+	StartThread_Core ((func), (data), (stackSize))
 #define CreateSemaphore(initial, name, syncClass) \
 	CreateSemaphore_Core ((initial))
 #define CreateMutex(name, syncClass) \
@@ -134,9 +145,13 @@ ThreadLocal *GetMyThreadLocal (void);
 
 void SleepThread (TimePeriod timePeriod);
 void SleepThreadUntil (TimeCount wakeTime);
+void DestroyThread (Thread);
 void TaskSwitch (void);
 void WaitThread (Thread thread, int *status);
 
+void FinishThread (Thread);
+void ProcessThreadLifecycles (void);
+
 #ifdef PROFILE_THREADS
 void PrintThreadsStats (void);
 #endif  /* PROFILE_THREADS */

sc2/src/sc2code/libs/threads/sdl/sdlthreads.c

@@ -220,10 +220,17 @@ ThreadHelper (void *startInfo) {
 
 	UnQueueThread (thread);
 	DestroyThreadLocal (thread->localData);
-	HFree (thread);
+	FinishThread (thread);
+	/* Destroying the thread is the responsibility of ProcessThreadLifecycles() */
 	return result;
 }
 
+void
+DestroyThread_SDL (Thread t)
+{
+	HFree (t);
+}	
+
 Thread
 CreateThread_SDL (ThreadFunction func, void *data, SDWORD stackSize
 #ifdef NAMED_SYNCHRO

sc2/src/sc2code/libs/threads/sdl/sdlthreads.h

@@ -50,6 +50,7 @@ void SleepThread_SDL (TimeCount sleepTime);
 void SleepThreadUntil_SDL (TimeCount wakeTime);
 void TaskSwitch_SDL (void);
 void WaitThread_SDL (Thread thread, int *status);
+void DestroyThread_SDL (Thread thread);
 
 void DestroyMutex_SDL (Mutex m);
 void LockMutex_SDL (Mutex m);
@@ -79,6 +80,7 @@ int  GetRecursiveMutexDepth_SDL (RecursiveMutex m);
 #define NativeSleepThreadUntil SleepThreadUntil_SDL
 #define NativeTaskSwitch TaskSwitch_SDL
 #define NativeWaitThread WaitThread_SDL
+#define NativeDestroyThread DestroyThread_SDL
 
 #define NativeCreateMutex CreateMutex_SDL
 #define NativeDestroyMutex DestroyMutex_SDL

sc2/src/sc2code/libs/threads/thrcommon.c

@@ -18,23 +18,140 @@
  */
 
 #include <stdio.h>
+#include <stdlib.h>
 #include "libs/threadlib.h"
 #include "libs/timelib.h"
 #include "libs/misc.h"
 #include "thrcommon.h"
 
+#define LIFECYCLE_SIZE 8
+typedef struct {
+	ThreadFunction func;
+	void *data;
+	SDWORD stackSize;
+	Semaphore sem;
+	Thread value;
+#ifdef NAMED_SYNCHRO
+	const char *name;
+#endif
+} SpawnRequest_struct;
+
+typedef SpawnRequest_struct *SpawnRequest;
+
+static Mutex        lifecycleMutex;
+static SpawnRequest pendingBirth[LIFECYCLE_SIZE];
+static Thread       pendingDeath[LIFECYCLE_SIZE];
+
 void
 InitThreadSystem (void)
 {
+	int i;
 	NativeInitThreadSystem ();
+	for (i = 0; i < LIFECYCLE_SIZE; i++)
+	{
+		pendingBirth[i] = NULL;
+		pendingDeath[i] = NULL;
+	}
+	lifecycleMutex = CreateMutex ("Thread Lifecycle Mutex", SYNC_CLASS_RESOURCE);
 }
 
 void
 UnInitThreadSystem (void)
 {
 	NativeUnInitThreadSystem ();
+	DestroyMutex (lifecycleMutex);
 }
 
+static Thread
+FlagStartThread (SpawnRequest s)
+{
+	int i;
+	LockMutex (lifecycleMutex);
+	for (i = 0; i < LIFECYCLE_SIZE; i++)
+	{
+		if (pendingBirth[i] == NULL)
+		{
+			pendingBirth[i] = s;
+			UnlockMutex (lifecycleMutex);
+			if (s->sem)
+			{
+				Thread result;
+				SetSemaphore (s->sem);
+				DestroySemaphore (s->sem);
+				result = s->value;
+				HFree (s);
+				return result;
+			}
+			return NULL;
+		}
+	}
+	fprintf (stderr, "Thread Lifecycle array filled.  This is a fatal error!  Make LIFECYCLE_SIZE something larger than %d.\n", LIFECYCLE_SIZE);
+	exit (-1);
+}
+
+void
+FinishThread (Thread thread)
+{
+	int i;
+	LockMutex (lifecycleMutex);
+	for (i = 0; i < LIFECYCLE_SIZE; i++)
+	{
+		if (pendingDeath[i] == NULL)
+		{
+			pendingDeath[i] = thread;
+			UnlockMutex (lifecycleMutex);
+			return;
+		}
+	}
+	fprintf (stderr, "Thread Lifecycle array filled.  This is a fatal error!  Make LIFECYCLE_SIZE something larger than %d.\n", LIFECYCLE_SIZE);
+	exit (-1);
+}
+
+/* Only call from main thread! */
+void
+ProcessThreadLifecycles (void)
+{
+	int i;
+	LockMutex (lifecycleMutex);
+	for (i = 0; i < LIFECYCLE_SIZE; i++)
+	{
+		SpawnRequest s = pendingBirth[i];
+		if (s != NULL)
+		{
+#ifdef NAMED_SYNCHRO
+			s->value = NativeCreateThread (s->func, s->data, s->stackSize, s->name);
+#else
+			s->value = NativeCreateThread (s->func, s->data, s->stackSize);
+#endif
+			if (s->sem)
+			{
+				ClearSemaphore (s->sem);
+				/* The spawning thread's FlagStartThread will clean up s */
+			}
+			else
+			{
+				/* The thread value has been lost to the game logic.  We must
+				   clean up s ourself. */
+				HFree (s);
+			}
+			pendingBirth[i] = NULL;
+		}
+	}
+
+	for (i = 0; i < LIFECYCLE_SIZE; i++)
+	{
+		Thread t = pendingDeath[i];
+		if (t != NULL)
+		{
+			WaitThread (t, NULL);
+			pendingDeath[i] = NULL;	
+			DestroyThread (t);
+		}
+	}
+	UnlockMutex (lifecycleMutex);
+}
+
+
 /* The Create routines look different based on whether NAMED_SYNCHRO
    is defined or not. */
 
@@ -42,7 +159,25 @@ UnInitThreadSystem (void)
 Thread
 CreateThread_Core (ThreadFunction func, void *data, SDWORD stackSize, const char *name)
 {
-	return NativeCreateThread (func, data, stackSize, name);
+	SpawnRequest s = HMalloc(sizeof (SpawnRequest_struct));
+	s->func = func;
+	s->data = data;
+	s->stackSize = stackSize;
+	s->name = name;
+	s->sem = CreateSemaphore (0, "SpawnRequest semaphore", SYNC_CLASS_RESOURCE);
+	return FlagStartThread (s);
+}
+
+void
+StartThread_Core (ThreadFunction func, void *data, SDWORD stackSize, const char *name)
+{
+	SpawnRequest s = HMalloc(sizeof (SpawnRequest_struct));
+	s->func = func;
+	s->data = data;
+	s->stackSize = stackSize;
+	s->name = name;
+	s->sem = NULL;
+	FlagStartThread (s);
 }
 
 Mutex
@@ -74,7 +209,23 @@ CreateCondVar_Core (const char *name, DWORD syncClass)
 Thread
 CreateThread_Core (ThreadFunction func, void *data, SDWORD stackSize)
 {
-	return NativeCreateThread (func, data, stackSize);
+	SpawnRequest s = HMalloc(sizeof (SpawnRequest_struct));
+	s->func = func;
+	s->data = data;
+	s->stackSize = stackSize;
+	s->sem = CreateSemaphore (0, "SpawnRequest semaphore", SYNC_CLASS_RESOURCE);
+	return FlagStartThread (s);
+}
+
+void
+StartThread_Core (ThreadFunction func, void *data, SDWORD stackSize)
+{
+	SpawnRequest s = HMalloc(sizeof (SpawnRequest_struct));
+	s->func = func;
+	s->data = data;
+	s->stackSize = stackSize;
+	s->sem = NULL;
+	FlagStartThread (s);
 }
 
 Mutex
@@ -102,6 +253,12 @@ CreateCondVar_Core (void)
 }
 #endif
 
+void
+DestroyThread (Thread t)
+{
+	NativeDestroyThread (t);
+}
+
 ThreadLocal *
 CreateThreadLocal (void)
 {

sc2/src/sc2code/starcon.c

@@ -849,6 +849,9 @@ EventHandler (BYTE selector)
 
 #define DEBUG_PSYTRON 0
 
+/* TODO: Remove these declarations once threading is gone. */
+extern int snddriver, soundflags;
+
 int Starcon2Main(void* blah)
 {
 #if DEBUG_PSYTRON || CREATE_JOURNAL
@@ -882,7 +885,10 @@ while (--ac > 0)
 }
 }
 #endif //DEBUG_PSYTRON || CREATE_JOURNAL
-				
+
+/* TODO: Put initAudio back in main where it belongs once threading is gone */
+initAudio (snddriver, soundflags);
+
 	if (LoadKernel (0,0))
 	{
 		extern BOOLEAN StartGame (void);

sc2/src/starcon2.c

@@ -44,6 +44,10 @@
 			// Including this is actually necessary on OSX.
 #endif
 
+/* TODO: Remove these (making them local to main() ) once threading is
+   gone. */
+
+int snddriver, soundflags;
 
 static int
 Check_PC_3DO_opt (const char *value, DWORD mask, const char *opt)
@@ -66,9 +70,7 @@ int
 main (int argc, char *argv[])
 {
 	int gfxdriver = TFB_GFXDRIVER_SDL_PURE;
-	int snddriver = audio_DRIVER_MIXSDL;
 	int gfxflags = 0;
-	int soundflags = audio_QUALITY_MEDIUM;
 	int width = 640, height = 480, bpp = 16;
 	int vol;
 	int val;
@@ -120,6 +122,14 @@ main (int argc, char *argv[])
 		{0, 0, 0, 0}
 	};
 
+	/* TODO: Once threading is gone, these become local variables
+	   again.  In the meantime, they must be global so that
+	   initAudio (in StarCon2Main) can see them.  initAudio needed
+	   to be moved there because calling AssignTask in the main
+	   thread doesn't work */
+	snddriver = audio_DRIVER_MIXSDL;
+	soundflags = audio_QUALITY_MEDIUM;
+
 	fprintf (stderr, "The Ur-Quan Masters v%d.%d%s (compiled %s %s)\n"
 	        "This software comes with ABSOLUTELY NO WARRANTY;\n"
 			"for details see the included 'COPYING' file.\n\n",
@@ -357,14 +367,18 @@ main (int argc, char *argv[])
 	init_communication ();
 	if (gammaset) 
 		TFB_SetGamma (gamma);
-	initAudio (snddriver, soundflags);
+	/* TODO: Once threading is gone, restore initAudio here.
+	   initAudio calls AssignTask, which currently blocks on
+	   ProcessThreadLifecycles... */
+	// initAudio (snddriver, soundflags);
 	TFB_InitInput (TFB_INPUTDRIVER_SDL, 0);
 
-	AssignTask (Starcon2Main, 1024, "Starcon2Main");
+	StartThread (Starcon2Main, NULL, 1024, "Starcon2Main");
 
 	for (;;)
 	{
 		TFB_ProcessEvents ();
+		ProcessThreadLifecycles ();
 		TFB_FlushGraphics ();
 	}