Skip to content

Commit ef26aff

Browse files
sigmaSdsigmaSd
authored
fix: callbacks lifetime (#89)
1 parent 4a64342 commit ef26aff

File tree

3 files changed

+151
-74
lines changed

3 files changed

+151
-74
lines changed

‎src/python.ts‎

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,9 @@ import { py } from "./ffi.ts";
44
import { cstr, SliceItemRegExp } from "./util.ts";
55

66
const refregistry = new FinalizationRegistry(py.Py_DecRef);
7-
8-
// FinalizationRegistry for auto-created callbacks
9-
// Closes the callback when the PyObject holding it is GC'd
10-
const callbackCleanupRegistry = new FinalizationRegistry(
11-
(callback: Callback) => {
12-
callback.destroy();
13-
},
14-
);
7+
// keep tracks of all callbacks, because JS can incorrectly GC them
8+
// using callback.destroy will remove the callback from this map
9+
const callbacks: { [id: number]: Callback } = {};
1510

1611
/**
1712
* Symbol used on proxied Python objects to point to the original PyObject object.
@@ -151,6 +146,9 @@ export class Callback {
151146
result: "pointer";
152147
}>;
153148

149+
static #nextId = 0;
150+
#id: number;
151+
154152
// Keep native-facing buffers alive for as long as this Callback exists
155153
/** @private */
156154
_pyMethodDef?: Uint8Array<ArrayBuffer>;
@@ -160,6 +158,8 @@ export class Callback {
160158
_docBuf?: Uint8Array<ArrayBuffer>;
161159

162160
constructor(public callback: PythonJSCallback) {
161+
this.#id = Callback.#nextId++;
162+
callbacks[this.#id] = this;
163163
this.unsafe = new Deno.UnsafeCallback(
164164
{
165165
parameters: ["pointer", "pointer", "pointer"],
@@ -208,6 +208,7 @@ export class Callback {
208208

209209
destroy() {
210210
this.unsafe.close();
211+
delete callbacks[this.#id];
211212
}
212213
}
213214

@@ -234,11 +235,6 @@ export class Callback {
234235
* C PyObject.
235236
*/
236237
export class PyObject {
237-
/**
238-
* A Python callabale object as Uint8Array
239-
* This is used with `PyCFunction_NewEx` in order to extend its liftime and not allow v8 to release it before its actually used
240-
*/
241-
#pyMethodDef?: Uint8Array;
242238
constructor(public handle: Deno.PointerValue) {}
243239

244240
/**
@@ -555,8 +551,6 @@ export class PyObject {
555551
// Otherwise V8 can release it before the callback is called
556552
// Is this still needed (after the change of pinning fields to the callabck) ? might be
557553
const pyObject = new PyObject(fn);
558-
pyObject.#pyMethodDef = methodDef;
559-
// Note: explicit Callback objects are user-managed, not auto-cleaned
560554
return pyObject;
561555
} else if (v instanceof PyObject) {
562556
return v;
@@ -614,8 +608,6 @@ export class PyObject {
614608
if (typeof v === "function") {
615609
const callback = new Callback(v);
616610
const pyObject = PyObject.from(callback);
617-
// Register cleanup to close callback when PyObject is GC'd
618-
callbackCleanupRegistry.register(pyObject, callback);
619611
return pyObject;
620612
}
621613
}

‎test/gtk.ts‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
import { kw, NamedArgument, type PyObject, python } from "../mod.ts";
2+
3+
import {
4+
type Adw1_ as Adw_,
5+
DenoGLibEventLoop,
6+
type Gtk4_ as Gtk_,
7+
// deno-lint-ignore no-import-prefix
8+
} from "jsr:@sigma/gtk-py@0.7.0";
9+
10+
const gi = python.import("gi");
11+
gi.require_version("Gtk", "4.0");
12+
gi.require_version("Adw", "1");
13+
const Gtk: Gtk_.Gtk = python.import("gi.repository.Gtk");
14+
const Adw: Adw_.Adw = python.import("gi.repository.Adw");
15+
const GLib = python.import("gi.repository.GLib");
16+
const gcp = python.import("gc");
17+
const el = new DenoGLibEventLoop(GLib); // this is important so setInterval works (by unblockig deno async event loop)
18+
19+
const gcInterval = setInterval(() => {
20+
gcp.collect();
21+
// @ts-ignore: requirse --v8-flags=--expose-gc
22+
gc();
23+
}, 100);
24+
25+
class MainWindow extends Gtk.ApplicationWindow {
26+
#state = false;
27+
#f?: PyObject;
28+
constructor(kwArg: NamedArgument) {
29+
// deno-lint-ignore no-explicit-any
30+
super(kwArg as any);
31+
this.set_default_size(300, 150);
32+
this.set_title("Awaker");
33+
this.connect("close-request", () => {
34+
el.stop();
35+
clearInterval(gcInterval);
36+
return false;
37+
});
38+
39+
const button = Gtk.ToggleButton(
40+
new NamedArgument("label", "OFF"),
41+
);
42+
const f = python.callback(this.onClick);
43+
button.connect("clicked", f);
44+
const vbox = Gtk.Box(
45+
new NamedArgument("orientation", Gtk.Orientation.VERTICAL),
46+
);
47+
vbox.append(button);
48+
this.set_child(vbox);
49+
}
50+
51+
// deno-lint-ignore no-explicit-any
52+
onClick = (_: any, button: Gtk_.ToggleButton) => {
53+
this.#state = !this.#state;
54+
(this.#state) ? button.set_label("ON") : button.set_label("OFF");
55+
};
56+
}
57+
58+
class App extends Adw.Application {
59+
#win: MainWindow | undefined;
60+
constructor(kwArg: NamedArgument) {
61+
super(kwArg);
62+
this.connect("activate", this.onActivate);
63+
}
64+
onActivate = python.callback((_kwarg, app: Gtk_.Application) => {
65+
new MainWindow(new NamedArgument("application", app)).present();
66+
});
67+
}
68+
69+
const app = new App(kw`application_id=${"com.example.com"}`);
70+
app.register();
71+
app.activate();
72+
el.start();

‎test/test_with_gc.ts‎

Lines changed: 70 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,27 @@
1-
import python, { Callback, PyObject } from "../mod.ts";
1+
import python, { Callback } from "../mod.ts";
22
import { assertEquals } from "./asserts.ts";
33

4-
Deno.test("js fns are automaticlly converted to callbacks", () => {
5-
const pyModule = python.runModule(
6-
`
4+
Deno.test(
5+
"js fns are automaticlly converted to callbacks",
6+
// auto callbacks are just a convience api, but they leak their resources
7+
// the user can use python.callback if they want to control the memory
8+
{ sanitizeResources: false },
9+
() => {
10+
const pyModule = python.runModule(
11+
`
712
def call_the_callback(cb):
813
result = cb()
914
return result + 1
1015
`,
11-
"test_module",
12-
);
16+
"test_module",
17+
);
1318

14-
assertEquals(pyModule.call_the_callback(() => 4).valueOf(), 5);
19+
assertEquals(pyModule.call_the_callback(() => 4).valueOf(), 5);
1520

16-
// @ts-ignore:requires: --v8-flags=--expose-gc
17-
gc(); // if this is commented out, the test will fail beacuse the callback was not freed
18-
});
21+
// @ts-ignore:requires: --v8-flags=--expose-gc
22+
gc(); // if this is commented out, the test will fail beacuse the callback was not freed
23+
},
24+
);
1925

2026
Deno.test("callbacks are not gc'd while still needed by python", () => {
2127
const pyModule = python.runModule(
@@ -63,9 +69,13 @@ def call_stored_callback():
6369
callbackObj.destroy();
6470
});
6571

66-
Deno.test("callbacks are not gc'd while still needed by python (function version)", () => {
67-
const pyModule = python.runModule(
68-
`
72+
Deno.test(
73+
"callbacks are not gc'd while still needed by python (autocallback version)",
74+
// auto callbacks leak
75+
{ sanitizeResources: false },
76+
() => {
77+
const pyModule = python.runModule(
78+
`
6979
stored_callback = None
7080
7181
def store_and_call_callback(cb):
@@ -79,47 +89,50 @@ def call_stored_callback():
7989
return -1
8090
return stored_callback()
8191
`,
82-
"test_gc_module",
83-
);
84-
85-
let callCount = 0;
86-
const callback = () => {
87-
callCount++;
88-
return callCount * 10;
89-
};
90-
91-
// Store the callback in Python and call it
92-
assertEquals(pyModule.store_and_call_callback(callback).valueOf(), 10);
93-
assertEquals(callCount, 1);
94-
95-
for (let i = 0; i < 10; i++) {
96-
// @ts-ignore:requires: --v8-flags=--expose-gc
97-
gc();
98-
}
99-
100-
// If the callback was incorrectly GC'd, this should segfault
101-
// But it should work because Python holds a reference
102-
assertEquals(pyModule.call_stored_callback().valueOf(), 20);
103-
assertEquals(callCount, 2);
104-
105-
// Call it again to be sure
106-
assertEquals(pyModule.call_stored_callback().valueOf(), 30);
107-
assertEquals(callCount, 3);
108-
});
109-
110-
Deno.test("auto-created callbacks are cleaned up after gc", () => {
111-
// Create callback and explicitly null it out to help GC
112-
// @ts-ignore PyObject can be created from fns its just the types are not exposed
113-
// deno-lint-ignore no-explicit-any
114-
let _f: any = PyObject.from(() => 5);
115-
116-
// Explicitly null the reference
117-
_f = null;
118-
119-
// Now f is null, trigger GC to clean it up
120-
// Run many GC cycles with delays to ensure finalizers execute
121-
for (let i = 0; i < 10; i++) {
122-
// @ts-ignore:requires: --v8-flags=--expose-gc
123-
gc();
124-
}
125-
});
92+
"test_gc_module",
93+
);
94+
95+
let callCount = 0;
96+
const callback = () => {
97+
callCount++;
98+
return callCount * 10;
99+
};
100+
101+
// Store the callback in Python and call it
102+
assertEquals(pyModule.store_and_call_callback(callback).valueOf(), 10);
103+
assertEquals(callCount, 1);
104+
105+
for (let i = 0; i < 10; i++) {
106+
// @ts-ignore:requires: --v8-flags=--expose-gc
107+
gc();
108+
}
109+
110+
// If the callback was incorrectly GC'd, this should segfault
111+
// But it should work because Python holds a reference
112+
assertEquals(pyModule.call_stored_callback().valueOf(), 20);
113+
assertEquals(callCount, 2);
114+
115+
// Call it again to be sure
116+
assertEquals(pyModule.call_stored_callback().valueOf(), 30);
117+
assertEquals(callCount, 3);
118+
},
119+
);
120+
121+
// Disabled for now, maybe in this feature someone can figure this out
122+
// https://github.com/denosaurs/deno_python/pull/87
123+
// Deno.test("auto-created callbacks are cleaned up after gc", () => {
124+
// // Create callback and explicitly null it out to help GC
125+
// // @ts-ignore PyObject can be created from fns its just the types are not exposed
126+
// // deno-lint-ignore no-explicit-any
127+
// let _f: any = PyObject.from(() => 5);
128+
129+
// // Explicitly null the reference
130+
// _f = null;
131+
132+
// // Now f is null, trigger GC to clean it up
133+
// // Run many GC cycles with delays to ensure finalizers execute
134+
// for (let i = 0; i < 10; i++) {
135+
// // @ts-ignore:requires: --v8-flags=--expose-gc
136+
// gc();
137+
// }
138+
// });

0 commit comments

Comments
 (0)