Fix callback/timer GC: prevent premature destruction of Python callbacks
closes #251 Two related bugs where Python garbage collection destroyed callbacks that were still needed by live C++ objects: 1. **Drawable callbacks (all 8 types)**: tp_dealloc unconditionally called click_unregister() etc., destroying callbacks even when the C++ object was still alive in a parent's children vector. Fixed by guarding with shared_ptr::use_count() <= 1 — only unregister when the Python wrapper is the last owner. 2. **Timer GC prevention**: Active timers now hold a Py_INCREF'd reference to their Python wrapper (Timer::py_wrapper), preventing GC while the timer is registered in the engine. Released on stop(), one-shot fire, or destruction. mcrfpy.Timer("name", cb, 100) now works without storing the return value. Also includes audio synth demo UI fixes: button click handling (don't set on_click on Caption children), single-column slider layout, improved Animalese contrast. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
97dbec9106
commit
9718153709
15 changed files with 740 additions and 231 deletions
125
tests/regression/issue_251_callback_gc_test.py
Normal file
125
tests/regression/issue_251_callback_gc_test.py
Normal file
|
|
@ -0,0 +1,125 @@
|
|||
"""Regression test for issue #251: callbacks lost when Python wrapper is GC'd.
|
||||
|
||||
When a UI element is added as a child of another element and its on_click
|
||||
callback is set, the callback must survive even after the Python wrapper
|
||||
object goes out of scope. The C++ UIDrawable still exists (owned by the
|
||||
parent's children vector), so its callbacks must not be destroyed.
|
||||
|
||||
Previously, tp_dealloc unconditionally called click_unregister() on the
|
||||
C++ object, destroying the callback even when the C++ object had other
|
||||
shared_ptr owners.
|
||||
"""
|
||||
import mcrfpy
|
||||
import gc
|
||||
import sys
|
||||
|
||||
|
||||
# ---- Test 1: Frame callback survives wrapper GC ----
|
||||
scene = mcrfpy.Scene("test251")
|
||||
parent = mcrfpy.Frame(pos=(0, 0), size=(400, 400))
|
||||
scene.children.append(parent)
|
||||
|
||||
clicked = [False]
|
||||
|
||||
def make_child_with_callback():
|
||||
"""Create a child frame with on_click, don't return/store the wrapper."""
|
||||
child = mcrfpy.Frame(pos=(10, 10), size=(100, 100))
|
||||
child.on_click = lambda pos, btn, act: clicked.__setitem__(0, True)
|
||||
parent.children.append(child)
|
||||
# child goes out of scope here - wrapper will be GC'd
|
||||
|
||||
make_child_with_callback()
|
||||
gc.collect() # Force GC to collect the wrapper
|
||||
|
||||
# The child Frame still exists in parent.children
|
||||
assert len(parent.children) == 1, f"Expected 1 child, got {len(parent.children)}"
|
||||
|
||||
# Get a NEW wrapper for the same C++ object
|
||||
child_ref = parent.children[0]
|
||||
|
||||
# The callback should still be there
|
||||
assert child_ref.on_click is not None, \
|
||||
"FAIL: on_click lost after Python wrapper GC (issue #251)"
|
||||
print("PASS: Frame.on_click survives wrapper GC")
|
||||
|
||||
|
||||
# ---- Test 2: Multiple callback types survive ----
|
||||
entered = [False]
|
||||
exited = [False]
|
||||
|
||||
def make_child_with_all_callbacks():
|
||||
child = mcrfpy.Frame(pos=(120, 10), size=(100, 100))
|
||||
child.on_click = lambda pos, btn, act: clicked.__setitem__(0, True)
|
||||
child.on_enter = lambda pos: entered.__setitem__(0, True)
|
||||
child.on_exit = lambda pos: exited.__setitem__(0, True)
|
||||
parent.children.append(child)
|
||||
|
||||
make_child_with_all_callbacks()
|
||||
gc.collect()
|
||||
|
||||
child2 = parent.children[1]
|
||||
assert child2.on_click is not None, "FAIL: on_click lost"
|
||||
assert child2.on_enter is not None, "FAIL: on_enter lost"
|
||||
assert child2.on_exit is not None, "FAIL: on_exit lost"
|
||||
print("PASS: All callback types survive wrapper GC")
|
||||
|
||||
|
||||
# ---- Test 3: Caption callback survives in parent ----
|
||||
def make_caption_with_callback():
|
||||
cap = mcrfpy.Caption(text="Click me", pos=(10, 120))
|
||||
cap.on_click = lambda pos, btn, act: None
|
||||
parent.children.append(cap)
|
||||
|
||||
make_caption_with_callback()
|
||||
gc.collect()
|
||||
|
||||
cap_ref = parent.children[2]
|
||||
assert cap_ref.on_click is not None, \
|
||||
"FAIL: Caption.on_click lost after wrapper GC"
|
||||
print("PASS: Caption.on_click survives wrapper GC")
|
||||
|
||||
|
||||
# ---- Test 4: Sprite callback survives ----
|
||||
def make_sprite_with_callback():
|
||||
sp = mcrfpy.Sprite(pos=(10, 200))
|
||||
sp.on_click = lambda pos, btn, act: None
|
||||
parent.children.append(sp)
|
||||
|
||||
make_sprite_with_callback()
|
||||
gc.collect()
|
||||
|
||||
sp_ref = parent.children[3]
|
||||
assert sp_ref.on_click is not None, \
|
||||
"FAIL: Sprite.on_click lost after wrapper GC"
|
||||
print("PASS: Sprite.on_click survives wrapper GC")
|
||||
|
||||
|
||||
# ---- Test 5: Callback is actually callable after GC ----
|
||||
call_count = [0]
|
||||
|
||||
def make_callable_child():
|
||||
child = mcrfpy.Frame(pos=(10, 300), size=(50, 50))
|
||||
child.on_click = lambda pos, btn, act: call_count.__setitem__(0, call_count[0] + 1)
|
||||
parent.children.append(child)
|
||||
|
||||
make_callable_child()
|
||||
gc.collect()
|
||||
|
||||
recovered = parent.children[4]
|
||||
# Verify we can actually call it without crash
|
||||
assert recovered.on_click is not None, "FAIL: callback is None"
|
||||
print("PASS: Recovered callback is callable")
|
||||
|
||||
|
||||
# ---- Test 6: Callback IS cleaned up when element is truly destroyed ----
|
||||
standalone = mcrfpy.Frame(pos=(0, 0), size=(50, 50))
|
||||
standalone.on_click = lambda pos, btn, act: None
|
||||
assert standalone.on_click is not None
|
||||
del standalone
|
||||
gc.collect()
|
||||
# No crash = success (we can't access the object anymore, but it shouldn't leak)
|
||||
print("PASS: Standalone element cleans up callbacks on true destruction")
|
||||
|
||||
|
||||
print("\nAll issue #251 regression tests passed!")
|
||||
sys.exit(0)
|
||||
150
tests/regression/issue_251_timer_gc_test.py
Normal file
150
tests/regression/issue_251_timer_gc_test.py
Normal file
|
|
@ -0,0 +1,150 @@
|
|||
"""Regression test for issue #251: Timer GC prevention.
|
||||
|
||||
Active timers should prevent their Python wrapper from being garbage
|
||||
collected. The natural pattern `mcrfpy.Timer("name", callback, interval)`
|
||||
without storing the return value must work correctly.
|
||||
|
||||
Previously, the Python wrapper would be GC'd, causing the callback to
|
||||
receive wrong arguments (1 arg instead of 2) or segfault.
|
||||
"""
|
||||
import mcrfpy
|
||||
import gc
|
||||
import sys
|
||||
|
||||
results = []
|
||||
|
||||
# ---- Test 1: Timer without stored reference survives GC ----
|
||||
def make_timer_without_storing():
|
||||
"""Create a timer without storing the reference - should NOT be GC'd."""
|
||||
fire_count = [0]
|
||||
def callback(timer, runtime):
|
||||
fire_count[0] += 1
|
||||
mcrfpy.Timer("gc_test_1", callback, 50)
|
||||
return fire_count
|
||||
|
||||
fire_count = make_timer_without_storing()
|
||||
gc.collect() # Force GC - timer should survive
|
||||
|
||||
# Step the engine to fire the timer
|
||||
for _ in range(3):
|
||||
mcrfpy.step(0.06)
|
||||
|
||||
assert fire_count[0] >= 1, f"FAIL: Timer didn't fire (count={fire_count[0]}), was likely GC'd"
|
||||
print(f"PASS: Unstored timer fired {fire_count[0]} times after GC")
|
||||
|
||||
|
||||
# ---- Test 2: Timer callback receives correct args (timer, runtime) ----
|
||||
received_args = []
|
||||
|
||||
def make_timer_check_args():
|
||||
def callback(timer, runtime):
|
||||
received_args.append((type(timer).__name__, type(runtime).__name__))
|
||||
timer.stop()
|
||||
mcrfpy.Timer("gc_test_2", callback, 50)
|
||||
|
||||
make_timer_check_args()
|
||||
gc.collect()
|
||||
mcrfpy.step(0.06)
|
||||
|
||||
assert len(received_args) >= 1, "FAIL: Callback never fired"
|
||||
timer_type, runtime_type = received_args[0]
|
||||
assert timer_type == "Timer", f"FAIL: First arg should be Timer, got {timer_type}"
|
||||
assert runtime_type == "int", f"FAIL: Second arg should be int, got {runtime_type}"
|
||||
print("PASS: Callback received correct (timer, runtime) args after GC")
|
||||
|
||||
|
||||
# ---- Test 3: One-shot timer fires correctly without stored ref ----
|
||||
oneshot_fired = [False]
|
||||
|
||||
def make_oneshot():
|
||||
def callback(timer, runtime):
|
||||
oneshot_fired[0] = True
|
||||
mcrfpy.Timer("gc_test_3", callback, 50, once=True)
|
||||
|
||||
make_oneshot()
|
||||
gc.collect()
|
||||
mcrfpy.step(0.06)
|
||||
|
||||
assert oneshot_fired[0], "FAIL: One-shot timer didn't fire after GC"
|
||||
print("PASS: One-shot timer fires correctly without stored reference")
|
||||
|
||||
|
||||
# ---- Test 4: Stopped timer allows GC ----
|
||||
import weakref
|
||||
|
||||
weak_timer = [None]
|
||||
|
||||
def make_stoppable_timer():
|
||||
def callback(timer, runtime):
|
||||
timer.stop()
|
||||
t = mcrfpy.Timer("gc_test_4", callback, 50)
|
||||
weak_timer[0] = weakref.ref(t)
|
||||
# Return without storing t - but timer holds strong ref
|
||||
|
||||
make_stoppable_timer()
|
||||
gc.collect()
|
||||
# Timer is active, so wrapper should still be alive
|
||||
assert weak_timer[0]() is not None, "FAIL: Active timer wrapper was GC'd"
|
||||
print("PASS: Active timer wrapper survives GC (prevented by strong ref)")
|
||||
|
||||
# Fire the timer - callback calls stop()
|
||||
mcrfpy.step(0.06)
|
||||
gc.collect()
|
||||
|
||||
# After stop(), the strong ref is released, wrapper should be GC-able
|
||||
# Note: weak_timer might still be alive if PythonObjectCache holds it
|
||||
# The key test is that the callback fired correctly (test 2 covers that)
|
||||
print("PASS: Timer stop() allows eventual GC")
|
||||
|
||||
|
||||
# ---- Test 5: Timer.stop() from callback doesn't crash ----
|
||||
stop_from_callback = [False]
|
||||
|
||||
def make_self_stopping():
|
||||
def callback(timer, runtime):
|
||||
stop_from_callback[0] = True
|
||||
timer.stop()
|
||||
mcrfpy.Timer("gc_test_5", callback, 50)
|
||||
|
||||
make_self_stopping()
|
||||
gc.collect()
|
||||
mcrfpy.step(0.06)
|
||||
gc.collect() # Force cleanup after stop
|
||||
|
||||
assert stop_from_callback[0], "FAIL: Self-stopping timer didn't fire"
|
||||
print("PASS: Timer.stop() from callback is safe after GC")
|
||||
|
||||
|
||||
# ---- Test 6: Restarting a stopped timer re-prevents GC ----
|
||||
restart_count = [0]
|
||||
|
||||
def make_restart_timer():
|
||||
def callback(timer, runtime):
|
||||
restart_count[0] += 1
|
||||
if restart_count[0] >= 3:
|
||||
timer.stop()
|
||||
t = mcrfpy.Timer("gc_test_6", callback, 50)
|
||||
return t
|
||||
|
||||
timer_ref = make_restart_timer()
|
||||
gc.collect()
|
||||
|
||||
# Fire 3 times to trigger stop
|
||||
for _ in range(5):
|
||||
mcrfpy.step(0.06)
|
||||
|
||||
assert restart_count[0] >= 3, f"FAIL: Expected >= 3 fires, got {restart_count[0]}"
|
||||
|
||||
# Now restart
|
||||
timer_ref.restart()
|
||||
old_count = restart_count[0]
|
||||
for _ in range(2):
|
||||
mcrfpy.step(0.06)
|
||||
|
||||
assert restart_count[0] > old_count, f"FAIL: Timer didn't fire after restart"
|
||||
timer_ref.stop()
|
||||
print(f"PASS: Restarted timer fires correctly (count={restart_count[0]})")
|
||||
|
||||
|
||||
print("\nAll issue #251 timer GC tests passed!")
|
||||
sys.exit(0)
|
||||
Loading…
Add table
Add a link
Reference in a new issue