fix: Refcount mess in mvAddCallback() and mvRunCallback() #2282
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Something was wrong with the vector init in InsertParser_Block0(), so that's chopped out too.
Rename mvAddCallback() -> mvAddCallbackJob(), mvRunCallback() -> mvRunCallbackJob(). Moved the burden of the overloading to mvCallbackWithData, it's cleaner. (cherry picked from commit 16b4bfe)
|
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 |
(cherry picked from commit 5069963)
Drag and drop still crashes in the demo. (cherry picked from commit b3d6ad0)
…bject. This may be a bit slower, but it makes resource management much easier. Drag and drop doesn't crash anymore. (cherry picked from commit 3e789c7)
(cherry picked from commit 05e0c7e)
(cherry picked from commit afad663)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
mvCallbackJobis now an RAII struct that handles references to Python objects.Rename
mvAddCallback()->mvAddCallbackJob(),mvRunCallback()->mvRunCallbackJob(). Both of those functions only takemvCallbackJob&&.mvCallbackJobhas 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()againstPyDict_SetItem()etc.mvAppItemis nowenable_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 parentmvAppItem.Concerning Areas:
I hope I caught the ones which actually need to have
appDataborrowed, 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, everymvAppItemdestruction has taken place in a thread which already has the GIL.