From 964be640cb1072b122e5047ddfed19907c6b9dab Mon Sep 17 00:00:00 2001 From: kwolekr Date: Sat, 17 Oct 2015 22:42:48 -0400 Subject: [PATCH] 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 --- src/threading/thread.cpp | 71 +++++++++------------ src/threading/thread.h | 10 +-- src/threads.h | 6 +- src/unittest/test_threading.cpp | 105 ++++++++++++++++++++++++++++++-- 4 files changed, 137 insertions(+), 55 deletions(-) diff --git a/src/threading/thread.cpp b/src/threading/thread.cpp index cca7e636..0996a9e3 100644 --- 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,42 +213,17 @@ bool Thread::kill() # else pthread_cancel(m_thread_handle); # endif - wait(); #endif - cleanup(); + m_retval = NULL; + m_joinable = false; + m_request_stop = false; 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_request_stop = false; -} - - bool Thread::getReturnValue(void **ret) { if (m_running) @@ -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 cd46856e..3d85e0eb 100644 --- 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 m_request_stop; Atomic 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 176b69c2..d4306f63 100644 --- 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 a5d98f0a..f0df85b2 100644 --- 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) + { + } -class AtomicTestThread : public Thread +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; +} + + +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 &v, Semaphore &trigger) : Thread("AtomicTest"), val(v), trigger(trigger) - {} + { + } + private: void *run() { @@ -56,6 +152,7 @@ private: ++val; return NULL; } + Atomic &val; Semaphore &trigger; };