fix callbacks lifetime

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<ArrayBuffer>;
@@ -160,6 +158,8 @@ export class Callback {
   _docBuf?: Uint8Array<ArrayBuffer>;
 
   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;
         }
       }

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();

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();
+//   }
+// });