Scheduler data race #1758
Scheduler data race #1758
Conversation
core/base/Scheduler.cpp
Outdated
| { | ||
| _performMutex.lock(); | ||
| // fixed #4123: Save the callback functions, they must be invoked after '_performMutex.unlock()', otherwise if | ||
| // new functions are added in callback, it will cause thread deadlock. | ||
| auto temp = std::move(_actionsToPerform); | ||
| _performMutex.unlock(); | ||
| _actionsToPerformEmpty.store(false, std::memory_order_release); |
There was a problem hiding this comment.
Shouldn't this be set to true?
There was a problem hiding this comment.
Thank you fixed
|
The schedule actionsToPerform delay depends on timing add by writer thread, even through you use atomic(will add cpu lock when compiled as asm). |
I am so sorry I do not fully understand your comment. |
|
I think you're introducing another race. _performMutex.unlock();
// <--- another thread can add new actions here and set _actionsToPerformEmpty to false here.
_actionsToPerformEmpty.store(true, std::memory_order_release);should be _actionsToPerformEmpty.store(true, std::memory_order_release);
_performMutex.unlock();I would get rid of this atomic and just take the lock and check for emptiness inside. It's called once per frame, so it has close to 0 performance impact, and I wouldn't trade readability here for potential issues (like above). |
|
@smilediver you are right :) I will change it. the fix with lock was here #1753 |
Describe your changes
This is next attempt for
#1753
Issue ticket number and link
#1752
Checklist before requesting a review
For each PR
Add Copyright if it missed:
-
"Copyright (c) 2019-present Axmol Engine contributors (see AUTHORS.md)."I have performed a self-review of my code.
Optional:
For core/new feature PR
Comments:
Fix for data race: