Synchronization between renderer and handlers doesn't work #2004
Conversation
|
A few small breaking changes were added on our part, once these are corrected, I can merge this finally! |
| @@ -2125,7 +2127,7 @@ split_frame(PyObject* self, PyObject* args, PyObject* kwargs) | |||
| &delay)) | |||
| return GetPyNone(); | |||
|
|
|||
| if (!GContext->manualMutexControl) std::lock_guard<std::mutex> lk(GContext->mutex); | |||
| std::lock_guard<std::recursive_mutex> lk(GContext->mutex); | |||
There was a problem hiding this comment.
I've just found a problem with this fix: it breaks split_frame(). Since it now properly locks the mutex, there's very little chance that waitOneFrame goes back from true to false. To fix that, it's sufficient to remove the line 2130 (locking the mutex) altogether: split_frame doesn't need the mutex.
There was a problem hiding this comment.
Also, while split_frame can be fixed by removing the mutex lock in it, it could also be made more efficient using a condition variable instead of an atomic bool. This way it returns as soon as rendering of the new frame has ended, instead of polling the variable every 32 ms (and potentially waiting up to 32 ms longer than needed).
I've implemented this in my local copy and can share as a separate PR.
There was a problem hiding this comment.
You can submit with this PR if you'd like.
|
Manually copied your code over. Let me know what email to use so I can co-author yet in the commit! |
|
Thanks Jonathan! I've had a busy week or two and completely missed these comments...
You can use |
| } | ||
| else | ||
| { | ||
| GContext->viewport->clientHeight = cheight + 39; |
There was a problem hiding this comment.
Doesn't this break #1130 ? Not sure where these lines came from, I didn't have them in the original fix...
There was a problem hiding this comment.
Ooh. I believe so. Let me check (this was several fixes in a single commit).
There was a problem hiding this comment.
Have you had a chance to look at this? Any plans on how to handle it? Since this PR is closed and seems to have been included into 1.9, we probably need to open another PR to change the lines back, right?
I'm asking because I've started updating to 1.9 and would like to have a clean base upon which to cherry-pick my commits...
name: Pull Request
about: Create a pull request to help us improve
title: Synchronization between renderer and handlers doesn't work
assignees: @hoffstadt
Description:
This fix replaces
mvContext::mutexwith arecursive_mutexand removes themanualMutexControlvariable since it's not needed for a recursive mutex. This allows the handlers thread to properly lock the mutex while making changes to the widgets tree.It also adds synchronization to some other code paths where I bumped into a crash or just noticed potential issues.
Concerning Areas:
There might be other areas that still need proper synchronization. I'm especially unsure about different messages in mvHandleMsg(). This should probably be researched in a separate issue and a separate PR, since the current PR does address the main concern of #1985 that manualMutexControl actually prevented mutex locking.
Another concern is that I didn't try to fix Linux and Mac implementations of mvViewport - I don't have a test system for that.