-
Notifications
You must be signed in to change notification settings - Fork 468
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
test sync_wait_tests.cpp /"multiple threads" hangs on mac osx #98
Comments
The test just hangs for ever.
|
I spent a few min to debug this. I added
output, and changed the number of times the loop runs: |
Actually, after running the same test multiple times and observing the printed value it became clear that the number of iterations is not directly relevant for the outcome. The test simply has some sort of a race condition. Some time it passes, some time is hangs after 7 iterations, some time after 89. Ex:
|
So the thread in the static thread pool and the main thread are clashing when accessing the task state somewhere. |
Thread #2:
|
Thread #1 (main):
|
I suspect there is a bug in the void cppcoro::detail::lightweight_manual_reset_event::set() noexcept
{
{
std::lock_guard<std::mutex> lock(m_mutex);
m_isSet = true;
}
m_cv.notify_all();
} I think by having the call to Can you try moving this call inside the scope of the lock_guard and see if that fixes the hang? |
I've made this change in 0a768bf. @yfinkelstein Let me know if you still see this issue with latest master. |
This should not matter. See this old discussion thread: https://groups.google.com/forum/?hl=ky#!msg/comp.programming.threads/wEUgPq541v8/ZByyyS8acqMJ |
I wrote:
Looking at your comment on commit 0a768bf again, I was wrong. Obviously, if setting m_isSet to true implies permission for another thread to destroy the condition variable, then it must be guaranteed that the new value of m_isSet is not observable yet when notify_all() is called. Sorry for the noise! |
@wilevers Your original comment was valid based on a response to my comment on this thread. Indeed, whether or not the call to notify_all() was done inside or outside of the critical section shouldn't cause a missed wake-up. But, yes, there was another problem which was that it's possible the This seems to be the most likely cause for the I'm marking this issue as closed but feel free to reopen if you are still seeing it. |
Excellent. Thank you, and again, sorry for the noise... |
output on the console after ctrl-c under lldb:
The text was updated successfully, but these errors were encountered: