search

hoffstadt / DearPyGui

Dear PyGui: A fast and powerful Graphical User Interface Toolkit for Python with minimal dependencies
https://dearpygui.readthedocs.io/en/latest/
MIT License
13.31k stars 691 forks source link

fix: Refcount mess in mvAddCallback() and mvRunCallback() #2282

Open bzczb opened 9 months ago

bzczb commented 9 months ago

This PR ought to be ignored until #2275 is merged and I've rebased this, there are a few complicated conflicts

name: Pull Request about: Create a pull request to help us improve title: 'Refcount mess in mvAddCallback() and mvRunCallback()' assignees: '@hoffstadt @Pcothren @v-ein'

closes #2036

Description:

mvCallbackJob is now an RAII struct that handles references to Python objects.

Rename mvAddCallback() -> mvAddCallbackJob(), mvRunCallback() -> mvRunCallbackJob(). Both of those functions only take mvCallbackJob&&.

mvCallbackJob has a lot of overloads to make constructing it concise in different situations, and takes explicit flags for the borrow semantics.

Using RAII structs, all the XINCREF and XDECREF can be worry-free, though you still have to watch out for PyList_SetItem() against PyDict_SetItem() etc.

mvAppItem is now enable_shared_from_this, which makes managing lifecycle of callbacks and callback data a lot easier. Make smart pointers to callbacks using the lifecycle of their parent mvAppItem.

Concerning Areas:

I hope I caught the ones which actually need to have appData borrowed, I did a broad search for those functions, but it's best to have that double checked.

One must verify that neither manual callback management nor automatic callback management are broken.

Removed GIL locking from the destructor of mvAppItem. So far in my testing since those changes, every mvAppItem destruction has taken place in a thread which already has the GIL.

v-ein commented 9 months ago

Ouch. That's what happens when you don't have time to publish your fix: another person duplicates the job you've already done :(.

Will try to review this one, but it will have to wait for quite long (let's say a month).

bzczb commented 9 months ago

Ouch. That's what happens when you don't have time to publish your fix: another person duplicates the job you've already done :(.

Will try to review this one, but it will have to wait for quite long (let's say a month).

Sorry to bogart in... but this is a tricky bug, so it's good to fix it as many times as possible