-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Thread: fix a crash on Windows due to data race condition on Thread::m_start_finished_mutex #6515
Conversation
…m_start_finished_mutex
// Unlock m_start_finished_mutex to prevent data race condition on Windows. | ||
// On Windows with VS2017 build TerminateThread is called and this mutex is not | ||
// released. We try to unlock it from caller thread and it's refused by system. | ||
thr->m_start_finished_mutex.unlock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be inside an #ifdef _WIN32
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on android it's also undefined behaviour, we use pthread_kill, and it's better to do it properly
Approved by @sfan5 on IRC: http://irc.minetest.net/minetest-dev/2017-10-10#i_5105037 |
…m_start_finished_mutex (#6515)
…m_start_finished_mutex (minetest#6515)
…m_start_finished_mutex (#6515)
…m_start_finished_mutex (#6515)
…m_start_finished_mutex (#6515)
…m_start_finished_mutex (minetest#6515)
…m_start_finished_mutex (minetest#6515)
m_start_finished_mutex is not unlocked by thread when exiting the thread::run function.
On windows TerminateHandle kill the thread but doesn't release mutexes owned by it, it's a dangerous function.
On VS2017 when calling Thread destructor and trying to unlock, it's refused by system because mutex is not owned by this thread.