Skip to content
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

Fix crash caused by destruction of non detached threads #418

Merged

Conversation

agorgl
Copy link
Contributor

@agorgl agorgl commented Feb 1, 2023

When an async_lua_task is erase()'d from the task_manager's m_map, the accompanying std::thread is destroyed.
In the event that the task_manager::on_idle is called before async_lua_task::proc has finished it's do_work() in order to detach() the task thread, a crash is caused in the async_lua_task's destructor by the m_thread's destructor as that sees that the thread is not marked as joined or detached.

The behavior has been noticed on machines with weak compute cores (virtual machines specifically) and on heavily overloaded hosts, such as development machines during heavy compilation duty.

Problem has been traced with debug builds and WinDbg and fixes are tested and work on the affected machines above.

When an async_lua_task is erase()'d from the task_manager's m_map,
the accompanying std::thread is destroyed. In the event that the
task_manager::on_idle is called before async_lua_task::proc has
finished it's do_work() in order to detach() the task thread,
a crash is caused in the async_lua_task's destructor by the m_thread's
destructor as that sees that the thread is not marked as joined or detached
As per std::unordered_map::erase documentation, return value is
the iterator following the last removed element, so any intermediate
next iterators are not necessary
Copy link
Owner

@chrisant996 chrisant996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, great find, thank you for analyzing and fixing it! And sorry for the inconvenience and crashes.

@chrisant996 chrisant996 merged commit dec9a6e into chrisant996:master Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants