Synchronization between renderer and handlers doesn't work

[v-ein]

Version of Dear PyGui

Version: 1.8.0
Operating System: Windows 10

My Issue/Question

I'm creating a lot of UI widgets from within a handler, and I've noticed that my application crashes or hangs up from time to time when the UI is being created. After setting up a series of experiments, I've found that the mutex used for synchronization between the renderer thread and the handlers thread doesn't work the intended way. In particular, I was getting random crashes in RenderItemRegistry() when it was going through themeRegistryRoots and at the same time the other thread was adding one more theme to themeRegistryRoots (which of course invalidated the iterator in RenderItemRegistry()). These two things are not supposed to happen simultaneously; the mutex should have "serialized" them.

The following piece of code is used in most API functions and is supposed to lock the mutex:

if (!GContext->manualMutexControl) std::lock_guard<std::mutex> lk(GContext->mutex);

However, it also unlocks the mutex immediately after acquiring it. It becomes easier to see it if one adds curly braces like this:

if (!GContext->manualMutexControl)
{
    std::lock_guard<std::mutex> lk(GContext->mutex);
}

lock_guard's scope is the if statement, so it locks the mutex and unlocks it right away.

To Reproduce

I've no idea how to reproduce it other than re-running the same application many times. With a relatively simple example (below), I could not get a reliable crash even with 100,000 themes being added in a loop. However, once in a while it does crash, so I'd suggest to run it in a loop from a script. Or maybe if the app also deletes themes it has just created, and repeat this in an infinite loop, it will eventually crash - I haven't tried that.

Expected behavior

No random crashes. Access to common data must be synchronized properly.

Screenshots/Video

Standalone, minimal, complete and verifiable example

import dearpygui.dearpygui as dpg

dpg.create_context()
dpg.create_viewport(title="Test", width=500, height=400)

window_id = None
quit = False
 
def on_show():
    # We only want to run this once - unbind the handler
    global window_id
    dpg.bind_item_handler_registry(window_id, 0)

     # This code runs in the handlers thread
    with dpg.window(pos=(100, 100), width=300, height=100):
        # Keep changing the themes array until it clashes with the renderer thread
        for _ in range(0, 100000):
            with dpg.theme():
                pass
    # Quitting the program so it can be re-run in a loop
    global quit
    quit = True 


dpg.setup_dearpygui()
dpg.show_viewport()

with dpg.item_handler_registry() as event_handlers:
    dpg.add_item_visible_handler(callback=on_show)

window_id = dpg.add_window()
dpg.bind_item_handler_registry(window_id, event_handlers)

while dpg.is_dearpygui_running() and not quit:
    jobs = dpg.get_callback_queue()     # Retrieves and clears queue
    dpg.run_callbacks(jobs)
    dpg.render_dearpygui_frame()
 
dpg.destroy_context()

[v-ein]

I've fixed this in my local copy by making mvContext::mutex a recursive mutex, and getting rid of mvContext::manualMutexControl altogether. Each API function then just locks the mutex with std::lock_guard without extra preconditions. I know some people hate recursive mutexes; however, taking into account that one can do with dpg.mutex(): in Python code (and potentially call another function that invokes dpg.mutex()), a recursive mutex looks to me the lesser evil here.

Unfortunately not all code paths are protected by the mutex. Even with the manualMutexControl issue fixed, there's still a chance to get unsynchronized access, which leads to a crash or to some other unexpected behavior. A good example is configure_viewport(), which can be used to move the viewport or to change its title - with a slight chance that the requested changes will not be made because mvPrerender() happened to reset a flag right after configure_viewport() had set it.

To be on the safe side, I've also added (locally) sync via the same mutex:

• to some mvPrerender() paths,
• to mvHandleMsg() (I'm testing on Windows at the moment),
• to mvToolWindow::draw(),
• and to mvLayoutWindow::drawWidgets().

There might be other areas that still need proper synchronization.

[bandit-masked]

Since you submitted a PR for this -- thanks again for all the PR's -- I'm closing the bug report. You are really putting DPG to the test! What are you building?

[v-ein]

Like I said in one of the tickets, I cannot tell much about the project I'm using DPG for. But yeah, its GUI is quite complicated, and sometimes reveals unexpected things in DPG. I'm used to analyzing code written by other people, and often try to sort it out myself.

[DataExplorerUser]

Alright, well, if you can share at any point in future that would be great. Thanks for the contributions!