Remove Thread::kill() and related unittest (#10317)

semarie · web-flow · commit 3fb1f4530188 · 2020-09-10T12:19:18.000+02:00
Closes: #6065
diff --git a/src/threading/thread.cpp b/src/threading/thread.cpp
@@ -73,7 +73,28 @@ Thread::Thread(const std::string &name) :
 
 Thread::~Thread()
 {
-	kill();
+	// kill the thread if running
+	if (!m_running) {
+		wait();
+	} else {
+
+		m_running = false;
+
+#if defined(_WIN32)
+		// See https://msdn.microsoft.com/en-us/library/hh920601.aspx#thread__native_handle_method
+		TerminateThread((HANDLE) m_thread_obj->native_handle(), 0);
+		CloseHandle((HANDLE) m_thread_obj->native_handle());
+#else
+		// We need to pthread_kill instead on Android since NDKv5's pthread
+		// implementation is incomplete.
+# ifdef __ANDROID__
+		pthread_kill(getThreadHandle(), SIGKILL);
+# else
+		pthread_cancel(getThreadHandle());
+# endif
+		wait();
+#endif
+	}
 
 	// Make sure start finished mutex is unlocked before it's destroyed
 	if (m_start_finished_mutex.try_lock())
@@ -138,37 +159,6 @@ bool Thread::wait()
 }
 
 
-bool Thread::kill()
-{
-	if (!m_running) {
-		wait();
-		return false;
-	}
-
-	m_running = false;
-
-#if defined(_WIN32)
-	// See https://msdn.microsoft.com/en-us/library/hh920601.aspx#thread__native_handle_method
-	TerminateThread((HANDLE) m_thread_obj->native_handle(), 0);
-	CloseHandle((HANDLE) m_thread_obj->native_handle());
-#else
-	// We need to pthread_kill instead on Android since NDKv5's pthread
-	// implementation is incomplete.
-# ifdef __ANDROID__
-	pthread_kill(getThreadHandle(), SIGKILL);
-# else
-	pthread_cancel(getThreadHandle());
-# endif
-	wait();
-#endif
-
-	m_retval       = nullptr;
-	m_joinable     = false;
-	m_request_stop = false;
-
-	return true;
-}
-
 
 bool Thread::getReturnValue(void **ret)
 {
diff --git a/src/threading/thread.h b/src/threading/thread.h
@@ -74,14 +74,6 @@ class Thread {
 	 */
 	bool stop();
 
-	/*
-	 * Immediately terminates the thread.
-	 * This should be used with extreme caution, as the thread will not have
-	 * any opportunity to release resources it may be holding (such as memory
-	 * or locks).
-	 */
-	bool kill();
-
 	/*
 	 * Waits for thread to finish.
 	 * Note:  This does not stop a thread, you have to do this on your own.
diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp
@@ -31,7 +31,6 @@ class TestThreading : public TestBase {
 	void runTests(IGameDef *gamedef);
 
 	void testStartStopWait();
-	void testThreadKill();
 	void testAtomicSemaphoreThread();
 };
 
@@ -40,7 +39,6 @@ static TestThreading g_test_instance;
 void TestThreading::runTests(IGameDef *gamedef)
 {
 	TEST(testStartStopWait);
-	TEST(testThreadKill);
 	TEST(testAtomicSemaphoreThread);
 }
 
@@ -111,29 +109,6 @@ void TestThreading::testStartStopWait()
 }
 
 
-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:
