From f8b08004d9e8b3f91590a0ec73add2a4abc8489e Mon Sep 17 00:00:00 2001 From: Nbiba Bedis Date: Sun, 2 Nov 2025 06:44:23 +0100 Subject: [PATCH] fix callbacks lifetime callbacks now are preserved in a glonbal map, and they're removed from it when callback.destory is called, this is because JS GC can't reason about them correctly autocallbacks now just leak the callback, they're considerd now a convience function, if the user wants to control memory they can use python.callback add gtk.ts test becasue it just a nice stress test, I want add it to the ci but its just for manual testing --- src/python.ts | 26 +++------ test/gtk.ts | 72 ++++++++++++++++++++++++ test/test_with_gc.ts | 127 ++++++++++++++++++++++++------------------- 3 files changed, 151 insertions(+), 74 deletions(-) create mode 100644 test/gtk.ts diff --git a/src/python.ts b/src/python.ts index e0580ec..e7577ea 100644 --- a/src/python.ts +++ b/src/python.ts @@ -4,14 +4,9 @@ import { py } from "./ffi.ts"; import { cstr, SliceItemRegExp } from "./util.ts"; const refregistry = new FinalizationRegistry(py.Py_DecRef); - -// FinalizationRegistry for auto-created callbacks -// Closes the callback when the PyObject holding it is GC'd -const callbackCleanupRegistry = new FinalizationRegistry( - (callback: Callback) => { - callback.destroy(); - }, -); +// keep tracks of all callbacks, because JS can incorrectly GC them +// using callback.destroy will remove the callback from this map +const callbacks: { [id: number]: Callback } = {}; /** * Symbol used on proxied Python objects to point to the original PyObject object. @@ -151,6 +146,9 @@ export class Callback { result: "pointer"; }>; + static #nextId = 0; + #id: number; + // Keep native-facing buffers alive for as long as this Callback exists /** @private */ _pyMethodDef?: Uint8Array; @@ -160,6 +158,8 @@ export class Callback { _docBuf?: Uint8Array; constructor(public callback: PythonJSCallback) { + this.#id = Callback.#nextId++; + callbacks[this.#id] = this; this.unsafe = new Deno.UnsafeCallback( { parameters: ["pointer", "pointer", "pointer"], @@ -208,6 +208,7 @@ export class Callback { destroy() { this.unsafe.close(); + delete callbacks[this.#id]; } } @@ -234,11 +235,6 @@ export class Callback { * C PyObject. */ 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 - */ - #pyMethodDef?: Uint8Array; constructor(public handle: Deno.PointerValue) {} /** @@ -555,8 +551,6 @@ export class PyObject { // 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 const pyObject = new PyObject(fn); - pyObject.#pyMethodDef = methodDef; - // Note: explicit Callback objects are user-managed, not auto-cleaned return pyObject; } else if (v instanceof PyObject) { return v; @@ -614,8 +608,6 @@ 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); return pyObject; } } diff --git a/test/gtk.ts b/test/gtk.ts new file mode 100644 index 0000000..f614002 --- /dev/null +++ b/test/gtk.ts @@ -0,0 +1,72 @@ +import { kw, NamedArgument, type PyObject, python } from "../mod.ts"; + +import { + type Adw1_ as Adw_, + DenoGLibEventLoop, + type Gtk4_ as Gtk_, + // deno-lint-ignore no-import-prefix +} from "jsr:@sigma/gtk-py@0.7.0"; + +const gi = python.import("gi"); +gi.require_version("Gtk", "4.0"); +gi.require_version("Adw", "1"); +const Gtk: Gtk_.Gtk = python.import("gi.repository.Gtk"); +const Adw: Adw_.Adw = python.import("gi.repository.Adw"); +const GLib = python.import("gi.repository.GLib"); +const gcp = python.import("gc"); +const el = new DenoGLibEventLoop(GLib); // this is important so setInterval works (by unblockig deno async event loop) + +const gcInterval = setInterval(() => { + gcp.collect(); + // @ts-ignore: requirse --v8-flags=--expose-gc + gc(); +}, 100); + +class MainWindow extends Gtk.ApplicationWindow { + #state = false; + #f?: PyObject; + constructor(kwArg: NamedArgument) { + // deno-lint-ignore no-explicit-any + super(kwArg as any); + this.set_default_size(300, 150); + this.set_title("Awaker"); + this.connect("close-request", () => { + el.stop(); + clearInterval(gcInterval); + return false; + }); + + const button = Gtk.ToggleButton( + new NamedArgument("label", "OFF"), + ); + const f = python.callback(this.onClick); + button.connect("clicked", f); + const vbox = Gtk.Box( + new NamedArgument("orientation", Gtk.Orientation.VERTICAL), + ); + vbox.append(button); + this.set_child(vbox); + } + + // deno-lint-ignore no-explicit-any + onClick = (_: any, button: Gtk_.ToggleButton) => { + this.#state = !this.#state; + (this.#state) ? button.set_label("ON") : button.set_label("OFF"); + }; +} + +class App extends Adw.Application { + #win: MainWindow | undefined; + constructor(kwArg: NamedArgument) { + super(kwArg); + this.connect("activate", this.onActivate); + } + onActivate = python.callback((_kwarg, app: Gtk_.Application) => { + new MainWindow(new NamedArgument("application", app)).present(); + }); +} + +const app = new App(kw`application_id=${"com.example.com"}`); +app.register(); +app.activate(); +el.start(); diff --git a/test/test_with_gc.ts b/test/test_with_gc.ts index ad0455a..93a6f7b 100644 --- a/test/test_with_gc.ts +++ b/test/test_with_gc.ts @@ -1,21 +1,27 @@ -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", () => { - const pyModule = python.runModule( - ` +Deno.test( + "js fns are automaticlly converted to callbacks", + // auto callbacks are just a convience api, but they leak their resources + // the user can use python.callback if they want to control the memory + { sanitizeResources: false }, + () => { + const pyModule = python.runModule( + ` def call_the_callback(cb): result = cb() return result + 1 `, - "test_module", - ); + "test_module", + ); - assertEquals(pyModule.call_the_callback(() => 4).valueOf(), 5); + 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 -}); + // @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", () => { const pyModule = python.runModule( @@ -63,9 +69,13 @@ def call_stored_callback(): callbackObj.destroy(); }); -Deno.test("callbacks are not gc'd while still needed by python (function version)", () => { - const pyModule = python.runModule( - ` +Deno.test( + "callbacks are not gc'd while still needed by python (autocallback version)", + // auto callbacks leak + { sanitizeResources: false }, + () => { + const pyModule = python.runModule( + ` stored_callback = None def store_and_call_callback(cb): @@ -79,47 +89,50 @@ def call_stored_callback(): return -1 return stored_callback() `, - "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(); - } - - // 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); - - // Call it again to be sure - assertEquals(pyModule.call_stored_callback().valueOf(), 30); - assertEquals(callCount, 3); -}); - -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; - - // 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(); - } -}); + "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(); + } + + // 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); + + // Call it again to be sure + assertEquals(pyModule.call_stored_callback().valueOf(), 30); + assertEquals(callCount, 3); + }, +); + +// Disabled for now, maybe in this feature someone can figure this out +// https://github.com/denosaurs/deno_python/pull/87 +// 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; + +// // 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(); +// } +// });