Fix some threading things and add additional thread unittests

- Fix thread name reset on start()
- Fully reset thread state on kill()
- Add unittests to check for correct object states under various circumstances

diff --git a/src/threading/thread.cpp b/src/threading/thread.cpp
index cca7e636e13e5ea95eda29ff4c8f7108b50fc1af..0996a9e34bbd421cd89107dd51b0608e209edf99 100644 (file)
--- a/src/threading/thread.cpp
+++ b/src/threading/thread.cpp
@@ -88,16 +88,13 @@ DEALINGS IN THE SOFTWARE.
 Thread::Thread(const std::string &name) :
        m_name(name),
        m_retval(NULL),
+       m_joinable(false),
        m_request_stop(false),
        m_running(false)
 {
 #ifdef _AIX
        m_kernel_thread_id = -1;
 #endif
-
-#if USE_CPP11_THREADS
-       m_thread_obj = NULL;
-#endif
 }
 
 
@@ -109,12 +106,12 @@ Thread::~Thread()
 
 bool Thread::start()
 {
-       MutexAutoLock lock(m_continue_mutex);
+       MutexAutoLock lock(m_mutex);
 
        if (m_running)
                return false;
 
-       cleanup();
+       m_request_stop = false;
 
 #if USE_CPP11_THREADS
 
@@ -145,6 +142,8 @@ bool Thread::start()
        while (!m_running)
                sleep_ms(1);
 
+       m_joinable = true;
+
        return true;
 }
 
@@ -156,21 +155,30 @@ bool Thread::stop()
 }
 
 
-void Thread::wait()
+bool Thread::wait()
 {
-       if (!m_running)
-               return;
+       MutexAutoLock lock(m_mutex);
+
+       if (!m_joinable)
+               return false;
 
 #if USE_CPP11_THREADS
 
        m_thread_obj->join();
 
+       delete m_thread_obj;
+       m_thread_obj = NULL;
+
 #elif USE_WIN_THREADS
 
        int ret = WaitForSingleObject(m_thread_handle, INFINITE);
        assert(ret == WAIT_OBJECT_0);
        UNUSED(ret);
 
+       CloseHandle(m_thread_handle);
+       m_thread_handle = NULL;
+       m_thread_id = -1;
+
 #elif USE_POSIX_THREADS
 
        int ret = pthread_join(m_thread_handle, NULL);
@@ -180,8 +188,8 @@ void Thread::wait()
 #endif
 
        assert(m_running == false);
-
-       return;
+       m_joinable = false;
+       return true;
 }
 
 
@@ -192,10 +200,12 @@ bool Thread::kill()
                return false;
        }
 
+       m_running = false;
+
 #ifdef _WIN32
        TerminateThread(m_thread_handle, 0);
+       CloseHandle(m_thread_handle);
 #else
-
        // We need to pthread_kill instead on Android since NDKv5's pthread
        // implementation is incomplete.
 # ifdef __ANDROID__
@@ -203,39 +213,14 @@ bool Thread::kill()
 # else
        pthread_cancel(m_thread_handle);
 # endif
-
        wait();
 #endif
 
-       cleanup();
-
-       return true;
-}
-
-
-void Thread::cleanup()
-{
-#if USE_CPP11_THREADS
-
-       delete m_thread_obj;
-       m_thread_obj = NULL;
-
-#elif USE_WIN_THREADS
-
-       CloseHandle(m_thread_handle);
-       m_thread_handle = NULL;
-       m_thread_id = -1;
-
-#elif USE_POSIX_THREADS
-
-       // Can't do any cleanup for pthreads
-
-#endif
-
-       m_name         = "";
        m_retval       = NULL;
-       m_running      = false;
+       m_joinable     = false;
        m_request_stop = false;
+
+       return true;
 }
 
 
@@ -256,11 +241,11 @@ bool Thread::isCurrentThread()
 
 
 #if USE_CPP11_THREADS || USE_POSIX_THREADS
-       void *(Thread::threadProc)(void *param)
+void *Thread::threadProc(void *param)
 #elif defined(_WIN32_WCE)
-       DWORD (Thread::threadProc)(LPVOID param)
+DWORD Thread::threadProc(LPVOID param)
 #elif defined(_WIN32)
-       DWORD WINAPI (Thread::threadProc)(LPVOID param)
+DWORD WINAPI Thread::threadProc(LPVOID param)
 #endif
 {
        Thread *thr = (Thread *)param;

diff --git a/src/threading/thread.h b/src/threading/thread.h
index cd46856e9342bb8abae54833d9dbd4ae6ccc1925..3d85e0eb9b6a43ce7983a6e12828d6318d6a89d0 100644 (file)
--- a/src/threading/thread.h
+++ b/src/threading/thread.h
@@ -81,9 +81,10 @@ public:
        /*
         * Waits for thread to finish.
         * Note:  This does not stop a thread, you have to do this on your own.
-        * Returns immediately if the thread is not started.
+        * Returns false immediately if the thread is not started or has been waited
+        * on before.
         */
-       void wait();
+       bool wait();
 
        /*
         * Returns true if the calling thread is this Thread object.
@@ -140,15 +141,14 @@ protected:
 
 private:
        void *m_retval;
+       bool m_joinable;
        Atomic<bool> m_request_stop;
        Atomic<bool> m_running;
-       Mutex m_continue_mutex;
+       Mutex m_mutex;
 
        threadid_t m_thread_id;
        threadhandle_t m_thread_handle;
 
-       void cleanup();
-
        static ThreadStartFunc threadProc;
 
 #ifdef _AIX

diff --git a/src/threads.h b/src/threads.h
index 176b69c2ee657c0680467df3fbd24e842be2ca3f..d4306f631a966132f926ed56f7602b25a66ea8dc 100644 (file)
--- a/src/threads.h
+++ b/src/threads.h
@@ -58,11 +58,11 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 // ThreadStartFunc
 //
 #if USE_CPP11_THREADS || USE_POSIX_THREADS
-       typedef void *(ThreadStartFunc)(void *param);
+       typedef void *ThreadStartFunc(void *param);
 #elif defined(_WIN32_WCE)
-       typedef DWORD (ThreadStartFunc)(LPVOID param);
+       typedef DWORD ThreadStartFunc(LPVOID param);
 #elif defined(_WIN32)
-       typedef DWORD WINAPI (ThreadStartFunc)(LPVOID param);
+       typedef DWORD WINAPI ThreadStartFunc(LPVOID param);
 #endif
 
 

diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp
index a5d98f0a6f2d627c22636ea8c83b055601de4ff5..f0df85b2d85da73d675dba175cbd75a9253fa9b5 100644 (file)
--- a/src/unittest/test_threading.cpp
+++ b/src/unittest/test_threading.cpp
@@ -28,26 +28,122 @@ class TestThreading : public TestBase {
 public:
        TestThreading() { TestManager::registerTestModule(this); }
        const char *getName() { return "TestThreading"; }
-       void runTests(IGameDef *);
+       void runTests(IGameDef *gamedef);
+
+       void testStartStopWait();
+       void testThreadKill();
        void testAtomicSemaphoreThread();
 };
 
 static TestThreading g_test_instance;
 
-void TestThreading::runTests(IGameDef *)
+void TestThreading::runTests(IGameDef *gamedef)
 {
+       TEST(testStartStopWait);
+       TEST(testThreadKill);
        TEST(testAtomicSemaphoreThread);
 }
 
+class SimpleTestThread : public Thread {
+public:
+       SimpleTestThread(unsigned int interval) :
+               Thread("SimpleTest"),
+               m_interval(interval)
+       {
+       }
+
+private:
+       void *run()
+       {
+               void *retval = this;
+
+               if (isCurrentThread() == false)
+                       retval = (void *)0xBAD;
+
+               while (!stopRequested())
+                       sleep_ms(m_interval);
+
+               return retval;
+       }
+
+       unsigned int m_interval;
+};
+
+void TestThreading::testStartStopWait()
+{
+       void *thread_retval;
+       SimpleTestThread *thread = new SimpleTestThread(25);
+
+       // Try this a couple times, since a Thread should be reusable after waiting
+       for (size_t i = 0; i != 5; i++) {
+               // Can't wait() on a joined, stopped thread
+               UASSERT(thread->wait() == false);
+
+               // start() should work the first time, but not the second.
+               UASSERT(thread->start() == true);
+               UASSERT(thread->start() == false);
+
+               UASSERT(thread->isRunning() == true);
+               UASSERT(thread->isCurrentThread() == false);
+
+               // Let it loop a few times...
+               sleep_ms(70);
+
+               // It's still running, so the return value shouldn't be available to us.
+               UASSERT(thread->getReturnValue(&thread_retval) == false);
+
+               // stop() should always succeed
+               UASSERT(thread->stop() == true);
+
+               // wait() only needs to wait the first time - the other two are no-ops.
+               UASSERT(thread->wait() == true);
+               UASSERT(thread->wait() == false);
+               UASSERT(thread->wait() == false);
+
+               // Now that the thread is stopped, we should be able to get the
+               // return value, and it should be the object itself.
+               thread_retval = NULL;
+               UASSERT(thread->getReturnValue(&thread_retval) == true);
+               UASSERT(thread_retval == thread);
+       }
+
+       delete thread;
+}
+
 
-class AtomicTestThread : public Thread
+void TestThreading::testThreadKill()
 {
+       SimpleTestThread *thread = new SimpleTestThread(300);
+
+       UASSERT(thread->start() == true);
+
+       // kill()ing is quite violent, so let's make sure our victim is sleeping
+       // before we do this... so we don't corrupt the rest of the program's state
+       sleep_ms(100);
+       UASSERT(thread->kill() == true);
+
+       // The state of the thread object should be reset if all went well
+       UASSERT(thread->isRunning() == false);
+       UASSERT(thread->start() == true);
+       UASSERT(thread->stop() == true);
+       UASSERT(thread->wait() == true);
+
+       // kill() after already waiting should fail.
+       UASSERT(thread->kill() == false);
+
+       delete thread;
+}
+
+
+class AtomicTestThread : public Thread {
 public:
        AtomicTestThread(Atomic<u32> &v, Semaphore &trigger) :
                Thread("AtomicTest"),
                val(v),
                trigger(trigger)
-       {}
+       {
+       }
+
 private:
        void *run()
        {
@@ -56,6 +152,7 @@ private:
                        ++val;
                return NULL;
        }
+
        Atomic<u32> &val;
        Semaphore &trigger;
 };