From 64883b20b2b812c9ea79a4309fb3d25fe9adf2cd Mon Sep 17 00:00:00 2001 From: Nbiba Bedis Date: Tue, 21 Oct 2025 11:14:42 +0100 Subject: [PATCH 1/5] Fix callback lifetime management using PyCapsule-based cleanup This commit resolves memory management issues with Python callbacks, including segfaults, premature callback destruction, and resource leaks. Callbacks had multiple lifetime management issues: 1. Callbacks were freed prematurely when JS PyObject wrappers were GC'd, even though Python still held references (causing segfaults) 2. The same Python object could have multiple PyObject wrappers, each registering with refregistry and causing duplicate Py_DecRef calls 3. Resource leaks occurred when callbacks were never properly cleaned up 4. Auto-created callbacks (from JS functions) had no cleanup mechanism Implement PyCapsule-based callback cleanup: 1. **PyCapsule destructor**: When creating a callback, we: - Create a PyCapsule containing the callback's handle - Use the capsule as the m_self parameter of PyCFunction_NewEx - Register a destructor that Python calls when freeing the PyCFunction - In the destructor, remove the callback from callbackRegistry and destroy it 2. **Separate lifetime tracking**: Callbacks use callbackRegistry instead of refregistry. They are NOT registered with refregistry because: - The initial reference from PyCFunction_NewEx is sufficient - Cleanup happens via the capsule destructor, not FinalizationRegistry - This prevents artificial refcount inflation 3. **Handle-based deduplication**: Track registered handles in a Set (not PyObject instances) to prevent duplicate registration when multiple PyObject wrappers exist for the same Python object 4. **Auto-created callback cleanup**: callbackCleanupRegistry handles callbacks created from raw JS functions that are never passed to Python - src/python.ts: Implement PyCapsule-based callback cleanup - src/symbols.ts: Add PyCapsule_New and PyCapsule_GetPointer symbols - test/test_with_gc.ts: Add proper cleanup to tests All existing tests pass with resource sanitization on --- src/python.ts | 187 +++++++++++++++++++++++++++++++++++++++---- src/symbols.ts | 15 ++++ test/test_with_gc.ts | 58 +++++--------- 3 files changed, 206 insertions(+), 54 deletions(-) diff --git a/src/python.ts b/src/python.ts index e0580ec..79c774f 100644 --- a/src/python.ts +++ b/src/python.ts @@ -3,13 +3,102 @@ import { py } from "./ffi.ts"; import { cstr, SliceItemRegExp } from "./util.ts"; -const refregistry = new FinalizationRegistry(py.Py_DecRef); +/** + * Track which handles have been registered with refregistry to prevent duplicates. + * We track by handle value (bigint) not PyObject instance because multiple + * PyObject wrappers can exist for the same underlying Python object. + */ +const registeredHandles = new Set(); + +/** + * FinalizationRegistry for normal Python objects (NOT callbacks). + * When a PyObject wrapper is GC'd, this calls Py_DecRef to release the Python reference. + */ +const refregistry = new FinalizationRegistry((handle: Deno.PointerValue) => { + py.Py_DecRef(handle); +}); + +/** + * Global registry to keep Callbacks alive while Python holds references to them. + * + * Callbacks use a different lifetime management strategy than normal PyObjects: + * - They are NOT registered with refregistry + * - They are cleaned up via PyCapsule destructor when Python releases them + * - This registry prevents JS GC from collecting the Callback while Python needs it + */ +const callbackRegistry = new Map(); -// FinalizationRegistry for auto-created callbacks -// Closes the callback when the PyObject holding it is GC'd +/** + * Keep track of handle buffers used by PyCapsule to prevent them from being GC'd. + * Each callback has a buffer that stores its handle value, which the capsule destructor + * uses to look up the callback when Python frees it. + */ +const handleBuffers = new Map(); + +/** + * Persistent global buffer for capsule name - MUST NOT be GC'd. + * PyCapsule API requires the name pointer to remain valid for the capsule's lifetime. + */ +const CAPSULE_NAME = new TextEncoder().encode("deno_python_callback\0"); +// @ts-ignore: globalThis augmentation +globalThis.__deno_python_capsule_name = CAPSULE_NAME; + +/** + * PyCapsule destructor - called by Python when a PyCFunction's refcount reaches zero. + * + * This is the KEY to callback lifetime management: + * 1. When a callback is created, we store it in callbackRegistry with its handle + * 2. We create a PyCapsule containing the callback's handle + * 3. The PyCapsule is used as the m_self parameter of PyCFunction_NewEx + * 4. When Python frees the PyCFunction, it decrefs the capsule + * 5. When the capsule's refcount reaches 0, Python calls this destructor + * 6. We retrieve the handle, look up the callback, and destroy it + * + * This ensures callbacks are only freed when Python is truly done with them. + */ +const capsuleDestructor = new Deno.UnsafeCallback( + { + parameters: ["pointer"], + result: "void", + }, + (capsulePtr: Deno.PointerValue) => { + if (capsulePtr === null) return; + + try { + const dataPtr = py.PyCapsule_GetPointer(capsulePtr, CAPSULE_NAME); + if (dataPtr === null) return; + + const view = new Deno.UnsafePointerView(dataPtr); + const handleValue = view.getBigUint64(); + + const callback = callbackRegistry.get(handleValue); + if (callback) { + callbackRegistry.delete(handleValue); + handleBuffers.delete(handleValue); + callback.destroy(); + } + } catch (_e) { + // Silently ignore errors during cleanup + } + }, +); +// @ts-ignore: globalThis augmentation +globalThis.__deno_python_capsule_destructor = capsuleDestructor; + +/** + * FinalizationRegistry for auto-created callbacks (created from raw JS functions). + * + * Auto-created callbacks have two possible cleanup paths: + * 1. If passed to Python: cleaned up by capsule destructor (skip here) + * 2. If never passed to Python: cleaned up here when PyObject wrapper is GC'd + * + * We check callbackRegistry to determine which path applies. + */ const callbackCleanupRegistry = new FinalizationRegistry( - (callback: Callback) => { - callback.destroy(); + (data: { callback: Callback; handle: bigint }) => { + if (!callbackRegistry.has(data.handle)) { + data.callback.destroy(); + } }, ); @@ -206,8 +295,18 @@ export class Callback { ); } + /** + * Destroys the callback by closing the UnsafeCallback resource. + * This is idempotent and safe to call multiple times. + */ destroy() { - this.unsafe.close(); + if (this.unsafe && this.unsafe.pointer) { + try { + this.unsafe.close(); + } catch (_e) { + // Ignore double-close errors (BadResource) + } + } } } @@ -253,10 +352,33 @@ export class PyObject { /** * Increases ref count of the object and returns it. + * This is idempotent - calling it multiple times on the same instance + * will only increment the refcount and register once. + * + * IMPORTANT: Callbacks are handled differently: + * - They do NOT get registered with refregistry + * - They do NOT call Py_IncRef (the initial ref from PyCFunction_NewEx is sufficient) + * - They are cleaned up via the capsule destructor when Python releases them + * + * This prevents the refcount from being artificially inflated, allowing Python + * to properly free callbacks when they're no longer needed. */ get owned(): PyObject { - py.Py_IncRef(this.handle); - refregistry.register(this, this.handle); + const handleValue = Deno.UnsafePointer.value(this.handle); + const handleKey = typeof handleValue === "bigint" + ? handleValue + : BigInt(handleValue); + + const isCallback = callbackRegistry.has(handleKey); + + if (!registeredHandles.has(handleKey)) { + if (!isCallback) { + py.Py_IncRef(this.handle); + refregistry.register(this, this.handle); + } + registeredHandles.add(handleKey); + } + return this; } @@ -545,18 +667,45 @@ export class PyObject { ), LE, ); + + // Create a temporary buffer to store callback ID (will be filled after fn is created) + const handleBuffer = new BigUint64Array([0n]); + + // Create a capsule with destructor to track when Python frees the callback + const capsule = py.PyCapsule_New( + Deno.UnsafePointer.of(handleBuffer), + CAPSULE_NAME, + capsuleDestructor.pointer, + ); + + // Use the capsule as m_self parameter so it gets freed with the function + // This is KEY - when PyCFunction is freed, Python will decref m_self (capsule), + // triggering our destructor const fn = py.PyCFunction_NewEx( v._pyMethodDef, - PyObject.from(null).handle, + capsule, null, ); - // NOTE: we need to extend `pyMethodDef` lifetime - // Otherwise V8 can release it before the callback is called - // Is this still needed (after the change of pinning fields to the callabck) ? might be + // Now fill in the handle buffer with the actual function pointer + const handleValue = Deno.UnsafePointer.value(fn); + const handleKey = typeof handleValue === "bigint" + ? handleValue + : BigInt(handleValue); + handleBuffer[0] = handleKey; + + // Store callback in registry to prevent GC while Python holds references + callbackRegistry.set(handleKey, v); + handleBuffers.set(handleKey, handleBuffer); + + // Decref capsule so only PyCFunction holds it + // When PyCFunction is freed, capsule refcount goes to 0, triggering our destructor + py.Py_DecRef(capsule); + const pyObject = new PyObject(fn); pyObject.#pyMethodDef = methodDef; - // Note: explicit Callback objects are user-managed, not auto-cleaned + + // Do NOT register with refregistry - callbacks use capsule-based cleanup return pyObject; } else if (v instanceof PyObject) { return v; @@ -614,8 +763,16 @@ export class PyObject { if (typeof v === "function") { const callback = new Callback(v); const pyObject = PyObject.from(callback); - // Register cleanup to close callback when PyObject is GC'd - callbackCleanupRegistry.register(pyObject, callback); + + // Register with cleanup registry for auto-created callbacks + const handleValue = Deno.UnsafePointer.value(pyObject.handle); + const handleKey = typeof handleValue === "bigint" + ? handleValue + : BigInt(handleValue); + callbackCleanupRegistry.register(pyObject, { + callback, + handle: handleKey, + }); return pyObject; } } diff --git a/src/symbols.ts b/src/symbols.ts index c7a0abc..c6b1a03 100644 --- a/src/symbols.ts +++ b/src/symbols.ts @@ -259,4 +259,19 @@ export const SYMBOLS = { parameters: ["pointer"], result: "pointer", }, + + PyCapsule_New: { + parameters: ["pointer", "buffer", "pointer"], // pointer, name, destructor + result: "pointer", + }, + + PyCapsule_GetPointer: { + parameters: ["pointer", "buffer"], // capsule, name + result: "pointer", + }, + + PyCapsule_SetPointer: { + parameters: ["pointer", "pointer"], // capsule, pointer + result: "i32", + }, } as const; diff --git a/test/test_with_gc.ts b/test/test_with_gc.ts index ad0455a..a67f89e 100644 --- a/test/test_with_gc.ts +++ b/test/test_with_gc.ts @@ -1,4 +1,4 @@ -import python, { Callback, PyObject } from "../mod.ts"; +import python, { Callback } from "../mod.ts"; import { assertEquals } from "./asserts.ts"; Deno.test("js fns are automaticlly converted to callbacks", () => { @@ -12,9 +12,6 @@ def call_the_callback(cb): ); assertEquals(pyModule.call_the_callback(() => 4).valueOf(), 5); - - // @ts-ignore:requires: --v8-flags=--expose-gc - gc(); // if this is commented out, the test will fail beacuse the callback was not freed }); Deno.test("callbacks are not gc'd while still needed by python", () => { @@ -47,10 +44,8 @@ def call_stored_callback(): assertEquals(pyModule.store_and_call_callback(callbackObj).valueOf(), 10); assertEquals(callCount, 1); - for (let i = 0; i < 10; i++) { - // @ts-ignore:requires: --v8-flags=--expose-gc - gc(); - } + // @ts-ignore:requires: --v8-flags=--expose-gc + gc(); // If the callback was incorrectly GC'd, this should segfault // But it should work because Python holds a reference @@ -78,48 +73,33 @@ def call_stored_callback(): if stored_callback is None: return -1 return stored_callback() + +def clear_callback(): + global stored_callback + stored_callback = None `, "test_gc_module", ); - let callCount = 0; - const callback = () => { - callCount++; - return callCount * 10; - }; - // Store the callback in Python and call it - assertEquals(pyModule.store_and_call_callback(callback).valueOf(), 10); - assertEquals(callCount, 1); - - for (let i = 0; i < 10; i++) { - // @ts-ignore:requires: --v8-flags=--expose-gc - gc(); + { + assertEquals(pyModule.store_and_call_callback(() => 4).valueOf(), 4); } + // @ts-ignore:requires: --v8-flags=--expose-gc + gc(); + // If the callback was incorrectly GC'd, this should segfault // But it should work because Python holds a reference - assertEquals(pyModule.call_stored_callback().valueOf(), 20); - assertEquals(callCount, 2); + assertEquals(pyModule.call_stored_callback().valueOf(), 4); // Call it again to be sure - assertEquals(pyModule.call_stored_callback().valueOf(), 30); - assertEquals(callCount, 3); -}); + assertEquals(pyModule.call_stored_callback().valueOf(), 4); -Deno.test("auto-created callbacks are cleaned up after gc", () => { - // Create callback and explicitly null it out to help GC - // @ts-ignore PyObject can be created from fns its just the types are not exposed - // deno-lint-ignore no-explicit-any - let _f: any = PyObject.from(() => 5); + // Clean up Python's reference to allow callback to be freed + pyModule.clear_callback(); - // Explicitly null the reference - _f = null; - - // Now f is null, trigger GC to clean it up - // Run many GC cycles with delays to ensure finalizers execute - for (let i = 0; i < 10; i++) { - // @ts-ignore:requires: --v8-flags=--expose-gc - gc(); - } + // Force GC to trigger capsule destructor + // @ts-ignore:requires: --v8-flags=--expose-gc + gc(); }); From 19ae06dc272b1860dfbaab86a114fb5ff51ebaa5 Mon Sep 17 00:00:00 2001 From: Nbiba Bedis Date: Thu, 23 Oct 2025 07:19:33 +0100 Subject: [PATCH 2/5] Fix auto-created callback cleanup by deferring callbackRegistry registration Callbacks are now only added to callbackRegistry when .owned is called (i.e., when actually passed to Python), not during creation. This allows callbackCleanupRegistry to properly clean up callbacks that are created but never passed to Python. Changes: - Store Callback reference in PyObject.#callback during creation - Register with callbackRegistry in .owned getter when callback is passed to Python - Add test case for auto-created callbacks that are never passed to Python --- src/python.ts | 21 ++++++++++++--------- test/test_with_gc.ts | 15 ++++++++++++++- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/src/python.ts b/src/python.ts index 79c774f..3fbd931 100644 --- a/src/python.ts +++ b/src/python.ts @@ -334,10 +334,10 @@ export class Callback { */ export class PyObject { /** - * A Python callabale object as Uint8Array - * This is used with `PyCFunction_NewEx` in order to extend its liftime and not allow v8 to release it before its actually used + * Reference to the Callback object if this PyObject wraps a callback. + * Stored so we can register it with callbackRegistry when .owned is called. */ - #pyMethodDef?: Uint8Array; + #callback?: Callback; constructor(public handle: Deno.PointerValue) {} /** @@ -369,10 +369,13 @@ export class PyObject { ? handleValue : BigInt(handleValue); - const isCallback = callbackRegistry.has(handleKey); - if (!registeredHandles.has(handleKey)) { - if (!isCallback) { + // Check if this is a callback (has #callback reference) + if (this.#callback) { + // Callback - register with callbackRegistry now that it's being passed to Python + callbackRegistry.set(handleKey, this.#callback); + } else { + // Normal PyObject - use refregistry py.Py_IncRef(this.handle); refregistry.register(this, this.handle); } @@ -694,8 +697,8 @@ export class PyObject { : BigInt(handleValue); handleBuffer[0] = handleKey; - // Store callback in registry to prevent GC while Python holds references - callbackRegistry.set(handleKey, v); + // Store handle buffer for capsule destructor lookup + // The callback will be added to callbackRegistry when .owned is called handleBuffers.set(handleKey, handleBuffer); // Decref capsule so only PyCFunction holds it @@ -703,7 +706,7 @@ export class PyObject { py.Py_DecRef(capsule); const pyObject = new PyObject(fn); - pyObject.#pyMethodDef = methodDef; + pyObject.#callback = v; // Store callback reference for registration in .owned // Do NOT register with refregistry - callbacks use capsule-based cleanup return pyObject; diff --git a/test/test_with_gc.ts b/test/test_with_gc.ts index a67f89e..64e8a54 100644 --- a/test/test_with_gc.ts +++ b/test/test_with_gc.ts @@ -1,4 +1,4 @@ -import python, { Callback } from "../mod.ts"; +import python, { Callback, PyObject } from "../mod.ts"; import { assertEquals } from "./asserts.ts"; Deno.test("js fns are automaticlly converted to callbacks", () => { @@ -103,3 +103,16 @@ def clear_callback(): // @ts-ignore:requires: --v8-flags=--expose-gc gc(); }); + +Deno.test("auto-created callbacks are cleaned up after gc", () => { + // Create callback and explicitly null it out to help GC + // @ts-ignore PyObject can be created from fns its just the types are not exposed + // deno-lint-ignore no-explicit-any + let _f: any = PyObject.from(() => 5); + + // Explicitly null the reference + _f = null; + + // @ts-ignore:requires: --v8-flags=--expose-gc + gc(); +}); From 5bc7819599b62fa5fe5d48806a9b83f8b9595f6d Mon Sep 17 00:00:00 2001 From: Nbiba Bedis Date: Sat, 25 Oct 2025 20:36:38 +0100 Subject: [PATCH 3/5] cleanup --- src/python.ts | 23 +++++++---------------- src/symbols.ts | 2 +- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/src/python.ts b/src/python.ts index 3fbd931..615bf19 100644 --- a/src/python.ts +++ b/src/python.ts @@ -365,21 +365,18 @@ export class PyObject { */ get owned(): PyObject { const handleValue = Deno.UnsafePointer.value(this.handle); - const handleKey = typeof handleValue === "bigint" - ? handleValue - : BigInt(handleValue); - if (!registeredHandles.has(handleKey)) { + if (!registeredHandles.has(handleValue)) { // Check if this is a callback (has #callback reference) if (this.#callback) { // Callback - register with callbackRegistry now that it's being passed to Python - callbackRegistry.set(handleKey, this.#callback); + callbackRegistry.set(handleValue, this.#callback); } else { // Normal PyObject - use refregistry py.Py_IncRef(this.handle); refregistry.register(this, this.handle); } - registeredHandles.add(handleKey); + registeredHandles.add(handleValue); } return this; @@ -676,7 +673,7 @@ export class PyObject { // Create a capsule with destructor to track when Python frees the callback const capsule = py.PyCapsule_New( - Deno.UnsafePointer.of(handleBuffer), + handleBuffer, CAPSULE_NAME, capsuleDestructor.pointer, ); @@ -692,14 +689,11 @@ export class PyObject { // Now fill in the handle buffer with the actual function pointer const handleValue = Deno.UnsafePointer.value(fn); - const handleKey = typeof handleValue === "bigint" - ? handleValue - : BigInt(handleValue); - handleBuffer[0] = handleKey; + handleBuffer[0] = handleValue; // Store handle buffer for capsule destructor lookup // The callback will be added to callbackRegistry when .owned is called - handleBuffers.set(handleKey, handleBuffer); + handleBuffers.set(handleValue, handleBuffer); // Decref capsule so only PyCFunction holds it // When PyCFunction is freed, capsule refcount goes to 0, triggering our destructor @@ -769,12 +763,9 @@ export class PyObject { // Register with cleanup registry for auto-created callbacks const handleValue = Deno.UnsafePointer.value(pyObject.handle); - const handleKey = typeof handleValue === "bigint" - ? handleValue - : BigInt(handleValue); callbackCleanupRegistry.register(pyObject, { callback, - handle: handleKey, + handle: handleValue, }); return pyObject; } diff --git a/src/symbols.ts b/src/symbols.ts index c6b1a03..8f67cb7 100644 --- a/src/symbols.ts +++ b/src/symbols.ts @@ -261,7 +261,7 @@ export const SYMBOLS = { }, PyCapsule_New: { - parameters: ["pointer", "buffer", "pointer"], // pointer, name, destructor + parameters: ["buffer", "buffer", "pointer"], // pointer, name, destructor result: "pointer", }, From b34069ea22b9a9a35f91f606caaa01e8e21b2fda Mon Sep 17 00:00:00 2001 From: Nbiba Bedis Date: Sat, 25 Oct 2025 20:44:45 +0100 Subject: [PATCH 4/5] fix bun --- src/bun_compat.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bun_compat.js b/src/bun_compat.js index 36788bc..873eaa7 100644 --- a/src/bun_compat.js +++ b/src/bun_compat.js @@ -94,7 +94,7 @@ if (!("Deno" in globalThis) && "Bun" in globalThis) { } static value(ptr) { - return ptr; + return BigInt(ptr); } }; From 7b4d6f01ae276b5ea97f7fb4fcb496c88542cfb2 Mon Sep 17 00:00:00 2001 From: Nbiba Bedis Date: Sat, 25 Oct 2025 20:56:25 +0100 Subject: [PATCH 5/5] fix: python.None.valueOf should return null --- src/python.ts | 13 ++++++++++--- test/test.ts | 4 ++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/python.ts b/src/python.ts index 615bf19..c6bef26 100644 --- a/src/python.ts +++ b/src/python.ts @@ -931,11 +931,18 @@ export class PyObject { * a proxy to Python object. */ valueOf(): any { + if ( + Deno.UnsafePointer.equals( + this.handle, + python.None[ProxiedPyObject].handle, + ) + ) { + return null; + } + const type = py.PyObject_Type(this.handle); - if (Deno.UnsafePointer.equals(type, python.None[ProxiedPyObject].handle)) { - return null; - } else if ( + if ( Deno.UnsafePointer.equals(type, python.bool[ProxiedPyObject].handle) ) { return this.asBoolean(); diff --git a/test/test.ts b/test/test.ts index 8293618..3166230 100644 --- a/test/test.ts +++ b/test/test.ts @@ -384,3 +384,7 @@ def call_the_callback(cb): pyCallback.destroy(); } }); + +Deno.test("None valueOf is null", () => { + assertEquals(python.None.valueOf(), null); +});