COMMON: Use boost::atomic to signal thread running and killing

Otherwise, this might be prone to race conditions. volatile isn't the
correct way to deal with this, either.

We can't really force a thread to die, either. SDL1 had a function for
that, but it was not portable and unreliable, so they removed it.

We can, however, detect that the thread is still running and issue a
warning.

src/common/thread.cpp

@@ -30,72 +30,86 @@ START_IGNORE_IMPLICIT_FALLTHROUGH
 STOP_IGNORE_IMPLICIT_FALLTHROUGH
 
 #include "src/common/thread.h"
+#include "src/common/util.h"
 
 namespace Common {
 
-Thread::Thread() {
-	_thread = 0;
-
-	_killThread    = false;
-	_threadRunning = false;
+Thread::Thread() : _killThread(false), _thread(0), _threadRunning(false) {
 }
 
 Thread::~Thread() {
 	destroyThread();
 }
 
 bool Thread::createThread(const UString &name) {
-	if (_threadRunning)
-		// Already running, nothing to do
-		return true;
+	if (_threadRunning.load(boost::memory_order_relaxed)) {
+		if (_name == name) {
+			warning("Thread::createThread(): Thread \"%s\" already running", _name.c_str());
+			return true;
+		}
+
+		warning("Thread::createThread(): Thread \"%s\" already running and trying to rename to \"%s\"", _name.c_str(), name.c_str());
+		return false;
+	}
+
+	_name = name;
 
 	// Try to create the thread
-	if (!(_thread = SDL_CreateThread(threadHelper, name.empty() ? 0 : name.c_str(), static_cast<void *>(this))))
+	if (!(_thread = SDL_CreateThread(threadHelper, _name.empty() ? 0 : _name.c_str(), static_cast<void *>(this))))
 		return false;
 
 	return true;
 }
 
 bool Thread::destroyThread() {
-	if (!_threadRunning)
+	if (!_threadRunning.load(boost::memory_order_seq_cst))
 		return true;
 
 	// Signal the thread that it should die
-	_killThread = true;
+	_killThread.store(true, boost::memory_order_seq_cst);
 
 	// Wait a whole second for the thread to finish on its own
-	for (int i = 0; _threadRunning && (i < 100); i++)
+	for (int i = 0; _threadRunning.load(boost::memory_order_seq_cst) && (i < 100); i++)
 		SDL_Delay(10);
 
-	if (!_threadRunning) {
-		// Wait for everything to settle
-		SDL_WaitThread(_thread, 0);
+	_killThread.store(false, boost::memory_order_seq_cst);
 
-		_killThread    = false;
-		_threadRunning = false;
+	const bool stillRunning = _threadRunning.load(boost::memory_order_seq_cst);
 
-		return true;
-	}
+	/* Clean up the thread if it's not still running. If the thread is still running,
+	 * this would block, potentially indefinitely, so we leak instead in that case.
+	 *
+	 * We could use SDL_DetachThread() instead, but:
+	 * - This only means that thread resources will be released once the thread
+	 *   ends. We waited for a whole second, so it might just not.
+	 * - SDL_DetachThread() was added in SDL 2.0.2, so we'd need to bump the
+	 *   minimum SDL version from 2.0.0 to 2.0.2. Not worth it for this case, IMHO. */
+	if (!stillRunning)
+		SDL_WaitThread(_thread, 0);
 
-	/// FIXME: not sure if the thread is really killed
+	_thread = 0;
 
-	_killThread    = false;
-	_threadRunning = false;
+	/* TODO:: If we get threads that we start and stop multiple times within the runtime
+	 *        of xoreos, we might need to do something more aggressive here, like throw. */
+	if (stillRunning) {
+		warning("Thread::destroyThread(): Thread \"%s\" still running", _name.c_str());
+		return false;
+	}
 
-	return false;
+	return true;
 }
 
 int Thread::threadHelper(void *obj) {
 	Thread *thread = static_cast<Thread *>(obj);
 
 	// The thread is running.
-	thread->_threadRunning = true;
+	thread->_threadRunning.store(true, boost::memory_order_relaxed);
 
 	// Run the thread
 	thread->threadMethod();
 
 	// Thread thread is not running.
-	thread->_threadRunning = false;
+	thread->_threadRunning.store(false, boost::memory_order_relaxed);
 
 	return 0;
 }

src/common/thread.h

@@ -27,6 +27,8 @@
 #ifndef COMMON_THREAD_H
 #define COMMON_THREAD_H
 
+#include "src/common/atomic.h"
+
 #include "src/common/fallthrough.h"
 START_IGNORE_IMPLICIT_FALLTHROUGH
 #include <SDL_thread.h>
@@ -48,12 +50,13 @@ class Thread : boost::noncopyable {
 	bool destroyThread();
 
 protected:
-	volatile bool _killThread;
+	boost::atomic<bool> _killThread;
 
 private:
 	SDL_Thread *_thread;
+	Common::UString _name;
 
-	bool _threadRunning;
+	boost::atomic<bool> _threadRunning;
 
 	virtual void threadMethod() = 0;
 

src/events/requests.cpp

@@ -47,8 +47,7 @@ void RequestManager::init() {
 }
 
 void RequestManager::deinit() {
-	if (!destroyThread())
-		warning("RequestManager::deinit(): Requests thread had to be killed");
+	destroyThread();
 
 	clearList();
 }
@@ -175,7 +174,7 @@ void RequestManager::collectGarbage() {
 }
 
 void RequestManager::threadMethod() {
-	while (!_killThread) {
+	while (!_killThread.load(boost::memory_order_relaxed)) {
 		collectGarbage();
 		EventMan.delay(100);
 	}

src/graphics/aurora/animationthread.cpp

@@ -95,7 +95,7 @@ void AnimationThread::flush() {
 }
 
 void AnimationThread::threadMethod() {
-	while (!_killThread) {
+	while (!_killThread.load(boost::memory_order_relaxed)) {
 		if (EventMan.quitRequested())
 			break;
 

src/sound/sound.cpp

@@ -135,8 +135,7 @@ void SoundManager::deinit() {
 	if (!_ready)
 		return;
 
-	if (!destroyThread())
-		warning("SoundManager::deinit(): Sound thread had to be killed");
+	destroyThread();
 
 	for (size_t i = 0; i < kChannelCount; i++)
 		freeChannel(i);
@@ -852,7 +851,7 @@ void SoundManager::freeChannel(size_t channel) {
 }
 
 void SoundManager::threadMethod() {
-	while (!_killThread) {
+	while (!_killThread.load(boost::memory_order_relaxed)) {
 		update();
 		_needUpdate.wait(100);
 	}