forked from Mirrorlandia_minetest/minetest
Fix synchronization issue at thread start
If a newly spawned thread called getThreadId or getThreadHandle before the spawning thread finished saving the thread handle, then the handle/id would be used uninitialized. This would cause the threading tests to fail since isCurrentThread would return false, and if Minetest is built with C++11 support the std::thread object pointer would be dereferenced while ininitialized, causing a segmentation fault. This fixes the issue by using a mutex to force the spawned thread to wait for the spawning thread to finish initializing the thread object. An alternative way to handle this would be to also set the thread handle/id in the started thread but this wouldn't work for C++11 builds because there's no way to get the partially constructed object.
This commit is contained in:
parent
79d752ba4f
commit
c93f7f5cea
@ -88,6 +88,15 @@ void Mutex::lock()
|
|||||||
#endif
|
#endif
|
||||||
}
|
}
|
||||||
|
|
||||||
|
bool Mutex::try_lock()
|
||||||
|
{
|
||||||
|
#if USE_WIN_MUTEX
|
||||||
|
return TryEnterCriticalSection(&mutex) != 0;
|
||||||
|
#else
|
||||||
|
return pthread_mutex_trylock(&mutex) == 0;
|
||||||
|
#endif
|
||||||
|
}
|
||||||
|
|
||||||
void Mutex::unlock()
|
void Mutex::unlock()
|
||||||
{
|
{
|
||||||
#if USE_WIN_MUTEX
|
#if USE_WIN_MUTEX
|
||||||
|
@ -56,6 +56,8 @@ public:
|
|||||||
void lock();
|
void lock();
|
||||||
void unlock();
|
void unlock();
|
||||||
|
|
||||||
|
bool try_lock();
|
||||||
|
|
||||||
protected:
|
protected:
|
||||||
Mutex(bool recursive);
|
Mutex(bool recursive);
|
||||||
void init_mutex(bool recursive);
|
void init_mutex(bool recursive);
|
||||||
|
@ -101,6 +101,11 @@ Thread::Thread(const std::string &name) :
|
|||||||
Thread::~Thread()
|
Thread::~Thread()
|
||||||
{
|
{
|
||||||
kill();
|
kill();
|
||||||
|
|
||||||
|
// Make sure start finished mutex is unlocked before it's destroyed
|
||||||
|
m_start_finished_mutex.try_lock();
|
||||||
|
m_start_finished_mutex.unlock();
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
@ -113,6 +118,9 @@ bool Thread::start()
|
|||||||
|
|
||||||
m_request_stop = false;
|
m_request_stop = false;
|
||||||
|
|
||||||
|
// The mutex may already be locked if the thread is being restarted
|
||||||
|
m_start_finished_mutex.try_lock();
|
||||||
|
|
||||||
#if USE_CPP11_THREADS
|
#if USE_CPP11_THREADS
|
||||||
|
|
||||||
try {
|
try {
|
||||||
@ -135,6 +143,9 @@ bool Thread::start()
|
|||||||
|
|
||||||
#endif
|
#endif
|
||||||
|
|
||||||
|
// Allow spawned thread to continue
|
||||||
|
m_start_finished_mutex.unlock();
|
||||||
|
|
||||||
while (!m_running)
|
while (!m_running)
|
||||||
sleep_ms(1);
|
sleep_ms(1);
|
||||||
|
|
||||||
@ -249,6 +260,10 @@ DWORD WINAPI Thread::threadProc(LPVOID param)
|
|||||||
g_logger.registerThread(thr->m_name);
|
g_logger.registerThread(thr->m_name);
|
||||||
thr->m_running = true;
|
thr->m_running = true;
|
||||||
|
|
||||||
|
// Wait for the thread that started this one to finish initializing the
|
||||||
|
// thread handle so that getThreadId/getThreadHandle will work.
|
||||||
|
thr->m_start_finished_mutex.lock();
|
||||||
|
|
||||||
thr->m_retval = thr->run();
|
thr->m_retval = thr->run();
|
||||||
|
|
||||||
thr->m_running = false;
|
thr->m_running = false;
|
||||||
|
@ -153,6 +153,7 @@ private:
|
|||||||
Atomic<bool> m_request_stop;
|
Atomic<bool> m_request_stop;
|
||||||
Atomic<bool> m_running;
|
Atomic<bool> m_running;
|
||||||
Mutex m_mutex;
|
Mutex m_mutex;
|
||||||
|
Mutex m_start_finished_mutex;
|
||||||
|
|
||||||
#if USE_CPP11_THREADS
|
#if USE_CPP11_THREADS
|
||||||
std::thread *m_thread_obj;
|
std::thread *m_thread_obj;
|
||||||
|
@ -39,9 +39,7 @@ static TestThreading g_test_instance;
|
|||||||
|
|
||||||
void TestThreading::runTests(IGameDef *gamedef)
|
void TestThreading::runTests(IGameDef *gamedef)
|
||||||
{
|
{
|
||||||
#if !(defined(__MACH__) && defined(__APPLE__))
|
|
||||||
TEST(testStartStopWait);
|
TEST(testStartStopWait);
|
||||||
#endif
|
|
||||||
TEST(testThreadKill);
|
TEST(testThreadKill);
|
||||||
TEST(testAtomicSemaphoreThread);
|
TEST(testAtomicSemaphoreThread);
|
||||||
}
|
}
|
||||||
|
Loading…
Reference in New Issue
Block a user