Version of Dear PyGui

Version: 1.8.0
Operating System: Windows 10

My Issue/Question

I'm adding a custom widget to the C++ part of DPG, in particular trying to schedule a callback from C++ code. It turns out the corresponding DPG functions don't handle reference counters properly for PyObject's passed into them.

Python docs say there are two ways to handle a PyObject reference: either own it (i.e. take away ownership from the caller) or borrow it (the caller holds ownership). mvAddCallback() is not very consistent in this sense: the manualCallbacks == true branch creates its own references (i.e. treats all parameters as borrowed refs) whereas the default (mvSubmitCallback) branch wants to take ownership.

To make matters worse, the code that actually calls mvAddCallback() doesn't seem to follow either of the two patterns above. Instead there's a mix of ownership transfer methods:

• Both callable and user_data are typically passed in as borrowed references:
  • callable is usually obtained from mvAppItem::getCallback(), which returns a reference without INCREF'ing it.
  • user_data usually comes from item.config.user_data - again without Py_INCREF.
• On the other hand, app_data is usually (but not always!) constructed anew on each call, and the reference is not decremented afterwards - as if mvAddCallback() was supposed to take ownership.

With the current implementation, there's a chance, however small it is, of callable or user_data getting destroyed while the callback task is waiting in the queue (i.e. between returning from mvAddCallback() and actual invocation of the callable). This only applies to manualCallbacks == false.

Also, the maxNumberOfCalls branch of mvAddCallback() is a different mix of ownership strategies. Luckily that branch is not supposed to run under normal conditions (it even has an assert(false) in it).

Okay, going further, the objects passed to mvAddCallback() will eventually end up in mvRunCallback(), which has a number of its own issues:

• callable is a borrowed ref, which means memory leaks (probably on a small scale) if manualCallbacks is true; moreover, if mvAddCallback() gets fixed to properly own the callable, all such callbacks will be leaking it even without manualCallbacks.
• If any of app_data or user_data are null, they are reasonably replaced with Py_None; however, those Py_None get two INCREFs, and at least one of these INCREFs won't ever be decreased.

	if (app_data == nullptr)
	{
		app_data = Py_None;
		Py_XINCREF(app_data);
	}
	Py_XINCREF(app_data);    // <--- second INCREF for Py_None

	if (user_data == nullptr)
	{
		user_data = Py_None;
		Py_XINCREF(user_data);
	}
	Py_XINCREF(user_data);    // <--- second INCREF for Py_None

• It treats both app_data and user_data as borrowed refs, which is inconsistent with how mvAddCallback() should be handling them (currently inconsistent with the manualCallbacks branch only). That is, it might be leaking these objects.
  • However, if callable is not a real callable (see the PyCallable_Check condition), it treats these same arguments as owned refs! Well, might be fine as soon as mvAddCallback() gets fixed; just wanted to point out an inconsistency.
• When it builds the parameter list, it does not DECREF parameters that do not make it to the list - see the count == 2, count == 1 and the last else branches. As a result, app_data and user_data might be leaking depending on how many arguments the callback function accepts.

That is mostly it, except for one little piece of code that has taken a special place in my heart 😍

	if(decrementAppData)
		Py_XINCREF(app_data);

Not only does decrementAppData exist in only one of two mvRunCallback overloads (and the same for mvAddCallback), it also increments the refcount 😆.

To Reproduce

Even with the most frequently running callbacks, like the hover callback, leaks are very small. Also, Python caches frequently used integer values, and when a callback leaks the same UUID every frame, Python just increases refcount for the corresponding (static) integer object.

Leaks themselves are difficult to discover; I can only demonstrate how refcounts grow on certain objects. To see it:

1. Run the code sample below from a console (you'll need to see stdout)
2. Hover the 'Hover me' button.
3. Look at stdout. You'll see how refcounts grow for the None object and for the sender (button) UUID.
4. Remove user_data from lambda parameters and try again - you'll see how None refcount grows faster (2 INCREFs per frame) - this is because mvRunCallback goes into the count==2 branch.

Expected behavior

No leaking.

Screenshots/Video

Standalone, minimal, complete and verifiable example

import sys
import dearpygui.dearpygui as dpg

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

dpg.setup_dearpygui()
with dpg.window(pos=(100, 100), width=300, height=200, no_title_bar=True):
    refs = [None]
    def show_ref_counts():
        print([ sys.getrefcount(ref) for ref in refs])

    with dpg.item_handler_registry() as handlers:
        dpg.add_item_hover_handler(callback=lambda sender, app_data, user_data: show_ref_counts())

    btn = dpg.add_button(label="Hover me!")
    refs.append(btn)
    print(f"refs = {refs}")

    dpg.bind_item_handler_registry(dpg.last_item(), handlers)

dpg.show_viewport()
dpg.start_dearpygui()
dpg.destroy_context()