DearPyGui
DearPyGui copied to clipboard
Refcount mess in mvAddCallback() and mvRunCallback()
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
anduser_data
are typically passed in as borrowed references:-
callable
is usually obtained frommvAppItem::getCallback()
, which returns a reference without INCREF'ing it. -
user_data
usually comes fromitem.config.user_data
- again withoutPy_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 ifmvAddCallback()
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) ifmanualCallbacks
is true; moreover, ifmvAddCallback()
gets fixed to properly own the callable, all such callbacks will be leaking it even withoutmanualCallbacks
. - If any of
app_data
oruser_data
are null, they are reasonably replaced withPy_None
; however, thosePy_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
anduser_data
as borrowed refs, which is inconsistent with howmvAddCallback()
should be handling them (currently inconsistent with themanualCallbacks
branch only). That is, it might be leaking these objects.- However, if
callable
is not a real callable (see thePyCallable_Check
condition), it treats these same arguments as owned refs! Well, might be fine as soon asmvAddCallback()
gets fixed; just wanted to point out an inconsistency.
- However, if
- 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 lastelse
branches. As a result,app_data
anduser_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 :heart_eyes:
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 :laughing:.
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:
- Run the code sample below from a console (you'll need to see stdout)
- Hover the 'Hover me' button.
- Look at stdout. You'll see how refcounts grow for the None object and for the sender (button) UUID.
- Remove
user_data
from lambda parameters and try again - you'll see how None refcount grows faster (2 INCREFs per frame) - this is becausemvRunCallback
goes into thecount==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()
For reference: decrementAppData
was added in:
- commit 74cc095 in the master branch
- PR #1711
- commit cd4c44d in that PR
The only call that uses this parameter is mvAddCallback
within output_frame_buffer()
in dearpygui_commands.h
. The purpose was probably to prevent framebuffer leaks - by defaul app_data gets an INCREF, and in this call mvAddCallback
is supposed to take ownership (no DECREF in the caller), so it's one INCREF too many.
I bet this parameter can be removed if we fix mvAddCallback so that it always treats all parameters as borrowed; the framebuffer object then can be wrapped into mvPyObject to prevent leaking.
Update: I've tried to fix this in my local copy, and it seems to work, but the fix still needs quite a bit of testing. I'm going to give it some time and then will open a PR.
Unfortunately I don't have time and data to test all the areas the fix touches, like plots, or like frame callbacks; my primary testing will be done on my own project. Maybe somebody else can run more extensive tests in future.
There is indeed a memory leak for handler callbacks. @Atlamillias suggest one temp workaround within Python itself 👍
import ctypes
Py_DECREF = ctypes.pythonapi.Py_DecRef
Py_DECREF.argtypes = (ctypes.py_object,)
Py_DECREF.restype = None
call Py_DECREF(app_data)
within callback function after app_data
become unused will release the stacked memory.
Reference to discord: https://discord.com/channels/736279277242417272/1147177649874161713/1147359978114515045