Skip to content

Commit

Permalink
Remove Thread::kill() and related unittest (#10317)
Browse files Browse the repository at this point in the history
Closes: #6065
  • Loading branch information
semarie authored Sep 10, 2020
1 parent 0683bea commit 3fb1f45
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 65 deletions.
54 changes: 22 additions & 32 deletions src/threading/thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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)
{
Expand Down
8 changes: 0 additions & 8 deletions src/threading/thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
25 changes: 0 additions & 25 deletions src/unittest/test_threading.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ class TestThreading : public TestBase {
void runTests(IGameDef *gamedef);

void testStartStopWait();
void testThreadKill();
void testAtomicSemaphoreThread();
};

Expand All @@ -40,7 +39,6 @@ static TestThreading g_test_instance;
void TestThreading::runTests(IGameDef *gamedef)
{
TEST(testStartStopWait);
TEST(testThreadKill);
TEST(testAtomicSemaphoreThread);
}

Expand Down Expand Up @@ -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:
Expand Down

0 comments on commit 3fb1f45

Please sign in to comment.