Repository navigation
CPython: callback trampolines hold their callback for the call and release their arguments - #35
Merged
Merged
Conversation
…leases its arguments The trampoline looked the callback up as a borrowed reference from the callback dict, so a callback that replaced its own registration while it ran (timer.set_cb(other) inside the timer callback) was freed mid-call. It now takes its own reference for the call. It also never released the converters' references to the arguments after packing them, so every call leaked one reference to each argument. A failed conversion now clears its error instead of passing NULL on.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Two reference bugs in every generated CPython callback trampoline (
binding/emit_cpython_native.py), on every Python build.The callback could be freed while it ran. The trampoline looked the Python callback up in the callback dict and used that borrowed reference for the whole call. A callback that replaces its own registration while it runs, such as
timer.set_cb(other)from inside the timer callback, dropped the dict's reference, and with it the only one. The trampoline now takes its own reference before the call and lets go of it right after, before LVGL's lock is taken back.Every call leaked its arguments. Each argument converter returns a new reference and
PyTuple_Packtakes another, but the trampoline never released the converter's. A widget passed to atree_walkcallback gained a reference per call and was never freed. The trampoline now releases them, and a failed conversion clears its error and returns instead of handingPyTuple_Packa NULL.generated/lvgl_python.cis regenerated (./regenerate_all.sh --target cpython;--checkmatches).tests/test_cpython_callback_references.pyreads every trampoline in the generated C and checks both; with the template change reverted on this branch (a planted fault, since undone) both tests fail, and the full generator suite passes with it (106 tests).Built into lvgl-python, a behavioural test (in lvgl-python's sync PR) fails on the current bindings, with the callback dead after replacing itself and a widget's count up by 10 after 10 tree walks, and passes with this change. lvgl-python's suite and
tools/test_lvgl_smoke.pypass on that build. The sync should land with or after lvgl-python#35, whose runtime fix makes every converter return a new reference.