Conversation
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks! Please also add a CHANGELOG and add to protocols.md in the guide.
Note that also on Python 3.7 we need to set Py_TPFLAGS_HAVE_FINALIZE, it's ignored on newer versions.
|
I found today that if we don't implement However the documentation in this area is confusing, as it implies that I think it may be possible that we use that default implementation to support this case, but we'll need to test carefully whether this also works for all Python versions, PyPy etc. |
|
I spoke to vstinner (thought no need to ping so no @) briefly about this, I understood that |
19cd088 to
107effb
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
ef04513 to
5086f38
Compare
Add the foundational pieces needed for __del__ support:
- Implement PyCallbackOutput for () to support void-returning callbacks
- Add finalizefunc trampoline for tp_finalize that:
- Saves/restores the current exception around the call
(matching CPython's slot_tp_finalize behavior)
- Routes errors to sys.unraisablehook via write_unraisable
- Handles both Python 3.12+ (PyErr_GetRaisedException) and
older versions (PyErr_Fetch/PyErr_Restore)
Register __del__ as a PyMethodProtoKind::Del variant with a custom impl_del_slot function (similar to __clear__'s impl_clear_slot). The generated code: - Creates a wrapper function with the finalizefunc trampoline - Maps to the Py_tp_finalize slot with ffi::destructor type - Validates that __del__ is an instance method with no arguments
Since PyO3 replaces CPython's subtype_dealloc with its own tp_dealloc, tp_finalize (which implements __del__) would never be called. This commit adds explicit calls to PyObject_CallFinalizerFromDealloc in both tp_dealloc (non-GC) and tp_dealloc_with_gc (GC) implementations. Key behaviors matching CPython's subtype_dealloc: - For non-GC types: call finalizer, abort dealloc if object is resurrected - For GC types: untrack, re-track, call finalizer (so the finalizer can make the object visible to the GC), then untrack again before proceeding This is currently gated behind #[cfg(not(Py_LIMITED_API))] since PyObject_CallFinalizerFromDealloc is not in the stable/limited API. For abi3 builds, the Py_tp_finalize slot is still set correctly, so: - Python code can call obj.__del__() explicitly - The cyclic GC will call tp_finalize for GC objects in reference cycles - Simple refcounted deallocation won't invoke __del__ on abi3 (known limitation)
Test cases: - test_del_called_explicitly: verify __del__ can be called as a method - test_del_called_on_dealloc: verify __del__ is called when object is deallocated (refcount drops to zero) - test_del_error_is_unraisable: verify exceptions in __del__ are routed to sys.unraisablehook (not propagated) - test_del_with_gc: verify __del__ works correctly with GC types (classes that also have __traverse__ and __clear__)
- Remove the 'not yet supported' note from protocols.md - Add __del__ to the basic customizations section with signature, description, and abi3 limitation note - Add changelog entry
Tests that rely on __del__ being called during deallocation depend on PyObject_CallFinalizerFromDealloc, which is not available in abi3 builds. Add #[cfg(not(Py_LIMITED_API))] guards to: - test_del_called_on_dealloc - test_del_error_is_unraisable - test_del_with_gc test_del_called_explicitly (which calls __del__ as a Python method) works on all builds and is left unguarded. Also clarify the finalizefunc trampoline comment to explain why we handle write_unraisable inside the body rather than letting trampoline_unraisable handle it: the saved exception must be restored before trampoline_unraisable runs its error handler, otherwise it would clobber the restored exception.
- test_del_via_cyclic_gc: creates an actual reference cycle (obj.cycle = obj), drops external references, then calls gc.collect(). Verifies __del__ is invoked by the cyclic GC's own tp_finalize call. This test is NOT gated behind cfg(not(Py_LIMITED_API)) since the cyclic GC calls tp_finalize directly (not through our tp_dealloc). - test_del_resurrection: a Python subclass __del__ stores self in a global variable, preventing deallocation. Verifies that PyObject_CallFinalizerFromDealloc correctly aborts dealloc (returns -1) when the object's refcount stays above zero, and the object remains usable afterwards.
Use a dedicated trampoline for tp_finalize instead of delegating to trampoline_unraisable. The exception save/restore now runs outside catch_unwind, ensuring that a panic in __del__ does not silently swallow a pre-existing Python exception. Add test_del_panic_preserves_active_exception to verify this behavior, along with test_del_with_py_arg for __del__ methods taking Python<'_>.
UnraisableCapture requires #[cfg(all(feature = "macros", Py_3_8))], so tests that use it must also be gated on Py_3_8 to compile on CPython 3.7 configs.
On Py_GIL_DISABLED builds the GC may run asynchronously in a separate thread and need multiple collection passes. Retry gc.collect() in a loop (with a short sleep on free-threaded builds), matching the pattern used in test_gc.rs.
- Gate test_del_called_on_dealloc, test_del_with_gc with Py_3_8 because Py_tp_finalize set via PyType_FromSpec is not properly applied on Python 3.7 (tp_finalize stays NULL on the type). - Gate test_del_via_cyclic_gc with Py_3_8 for the same reason — the GC also relies on tp_finalize being set. - Gate test_del_panic_preserves_active_exception with not(wasm32) because panics abort on wasm32-wasip1 (no unwinding support), so catch_unwind in the finalizefunc trampoline cannot catch the panic.
GraalPy does not properly support PyObject_CallFinalizerFromDealloc, causing crashes during deallocation. Gate the calls with not(GraalPy) alongside the existing not(Py_LIMITED_API) and not(PyPy) guards.
|
With the help from LLM, made some progress on this, tests are passing, review would be appreciated! I have also looked into removing our |
Adds support for
__del__in#[pymethods], mapped to CPython'stp_finalizeslot.Why a custom
impl_del_slotinstead ofSlotDef?The generic
SlotDefmachinery assumes the trampoline module name and the FFI type name are the same (e.g.,inquiryforPy_tp_clear). Fortp_finalize, the FFI type isdestructor(void-returning) but the trampoline needs to befinalizefunc— they don't match. A customimpl_del_slot(following the same pattern asimpl_clear_slot) avoids complicating the generic machinery for a single special case.Alternative considered: Adding a separate
ffi_typefield toSlotDefto decouple the trampoline name from the cast type. Rejected because it would add complexity to the generic path that only__del__would use.Why a dedicated trampoline instead of
trampoline_unraisable?CPython's
slot_tp_finalizesaves the active exception before calling__del__and restores it afterwards, since finalizers can run at arbitrary points during execution. We need the same save/restore semantics, but crucially, the restore must happen even if__del__panics. If we usedtrampoline_unraisable(which wraps the entire body incatch_unwind), a panic would unwind past the restore, silently swallowing the active exception. The dedicated trampoline putscatch_unwindaround only the user's__del__call, so the exception restore always runs.Alternative considered: Nesting the error handling inside
trampoline_unraisable's body — save exception, call__del__,write_unraisableon error, restore exception, returnOk(()). This was the initial implementation but was replaced because a panic in__del__would skip the restore, leaking the saved exceptionPyObject*pointers and losing the active exception state.Why call
PyObject_CallFinalizerFromDeallocfromtp_dealloc?PyO3 replaces CPython's
subtype_deallocwith its owntp_dealloc, sotp_finalizewould never be called during normal deallocation without explicit invocation. The GC track/untrack dance intp_dealloc_with_gc(untrack → re-track → finalize → untrack) mirrors CPython'ssubtype_deallocto correctly handle object resurrection and GC visibility during finalization.Alternative considered: Hooking into
subtype_deallocor usingtp_del(the deprecated pre-PEP 442 slot). Rejected because PyO3 already owns thetp_deallocslot, so callingPyObject_CallFinalizerFromDeallocdirectly is the most straightforward approach, andtp_delis deprecated since Python 3.4.abi3 limitation
PyObject_CallFinalizerFromDeallocis not in the stable ABI, so on abi3 builds__del__won't fire during normal refcount-based deallocation. ThePy_tp_finalizeslot is still set, so__del__is invoked by the cyclic garbage collector for GC-tracked types and can be called explicitly via Python code. This is documented in the guide.Alternative considered: Manually inlining the logic of
PyObject_CallFinalizerFromDealloc(temporarily resurrect the object, calltp_finalize, check refcount) using only stable-ABI functions. Rejected because it would duplicate subtle CPython internals and could diverge across Python versions.Closes #2479