diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index adc9502..3bf3e83 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -47,24 +47,18 @@ jobs: - name: Setup latest deno version uses: denoland/setup-deno@v2 + - name: Setup Python + uses: actions/setup-python@v5 + with: + python-version: "3.13" + - name: Setup Bun if: ${{ matrix.os != 'windows-latest' }} uses: oven-sh/setup-bun@v1 - - name: Setup Python (Windows) - uses: actions/setup-python@v2 - if: ${{ matrix.os == 'windows-latest' }} - with: - python-version: "3.13" - - name: Install NumPy - if: ${{ matrix.os != 'macos-latest' }} run: python3 -m pip install numpy - - name: Install NumPy on MacOs - if: ${{ matrix.os == 'macos-latest' }} - run: python3 -m pip install --user --break-system-packages numpy - - name: Run deno test run: deno task test diff --git a/deno.json b/deno.json index d8901e3..59b0faf 100644 --- a/deno.json +++ b/deno.json @@ -10,7 +10,7 @@ "check:mod": "deno check --unstable-ffi mod.ts", "check:ext": "deno check --unstable-ffi ext/*.ts", "check:examples": "deno check --unstable-ffi examples/*.ts", - "test": "deno test --unstable-ffi -A test/test.ts", + "test": "deno test --unstable-ffi -A test/test.ts && deno test -A --v8-flags=--expose-gc test/test_with_gc.ts", "example:hello_python": "deno run -A --unstable-ffi examples/hello_python.ts", "example:matplotlib": "deno run -A --unstable-ffi examples/matplotlib.ts", "example:pip_import": "deno run -A --unstable-ffi examples/pip_import.ts", diff --git a/ext/pip.ts b/ext/pip.ts index d06a2fc..8ba1ce4 100644 --- a/ext/pip.ts +++ b/ext/pip.ts @@ -1,3 +1,4 @@ +// deno-lint-ignore-file no-import-prefix import { kw, python, PythonError } from "../mod.ts"; import { join } from "jsr:@std/path@^1/join"; diff --git a/src/python.ts b/src/python.ts index 8054590..c8d7332 100644 --- a/src/python.ts +++ b/src/python.ts @@ -143,6 +143,14 @@ export class Callback { result: "pointer"; }>; + // Keep native-facing buffers alive for as long as this Callback exists + /** @private */ + _pyMethodDef?: Uint8Array; + /** @private */ + _nameBuf?: Uint8Array; + /** @private */ + _docBuf?: Uint8Array; + constructor(public callback: PythonJSCallback) { this.unsafe = new Deno.UnsafeCallback( { @@ -490,16 +498,20 @@ export class PyObject { } else if (v instanceof Callback) { // https://docs.python.org/3/c-api/structures.html#c.PyMethodDef // there are extra 4 bytes of padding after ml_flags field - const pyMethodDef = new Uint8Array(8 + 8 + 4 + 4 + 8); - const view = new DataView(pyMethodDef.buffer); + // Build and pin PyMethodDef + string buffers on the Callback instance + const methodDef = new Uint8Array(8 + 8 + 4 + 4 + 8); + v._pyMethodDef = methodDef; + const view = new DataView(methodDef.buffer); const LE = new Uint8Array(new Uint32Array([0x12345678]).buffer)[0] !== 0x7; const name = "JSCallback:" + (v.callback.name || "anonymous"); - const nameBuf = new TextEncoder().encode(`${name}\0`); + v._nameBuf = new TextEncoder().encode(`${name}\0`); view.setBigUint64( 0, - BigInt(Deno.UnsafePointer.value(Deno.UnsafePointer.of(nameBuf)!)), + BigInt( + Deno.UnsafePointer.value(Deno.UnsafePointer.of(v._nameBuf)!), + ), LE, ); view.setBigUint64( @@ -507,32 +519,35 @@ export class PyObject { BigInt(Deno.UnsafePointer.value(v.unsafe.pointer)), LE, ); + // METH_VARARGS | METH_KEYWORDS view.setInt32(16, 0x1 | 0x2, LE); + // https://github.com/python/cpython/blob/f27593a87c344f3774ca73644a11cbd5614007ef/Objects/typeobject.c#L688 const SIGNATURE_END_MARKER = ")\n--\n\n"; // We're not using the correct arguments name, but just using dummy ones (because they're not accessible in js) const fnArgs = [...Array(v.callback.length).keys()] .map((_, i) => String.fromCharCode(97 + i)).join(","); - const docBuf = `${name}(${fnArgs}${SIGNATURE_END_MARKER}\0`; + v._docBuf = new TextEncoder().encode( + `${name}(${fnArgs}${SIGNATURE_END_MARKER}\0`, + ); view.setBigUint64( 24, BigInt( - Deno.UnsafePointer.value( - Deno.UnsafePointer.of(new TextEncoder().encode(docBuf))!, - ), + Deno.UnsafePointer.value(Deno.UnsafePointer.of(v._docBuf)!), ), LE, ); const fn = py.PyCFunction_NewEx( - pyMethodDef, + v._pyMethodDef, PyObject.from(null).handle, 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 const pyObject = new PyObject(fn); - pyObject.#pyMethodDef = pyMethodDef; + pyObject.#pyMethodDef = methodDef; return pyObject; } else if (v instanceof PyObject) { return v; diff --git a/src/util.ts b/src/util.ts index e4566b6..cbb6902 100644 --- a/src/util.ts +++ b/src/util.ts @@ -42,7 +42,7 @@ export function postSetup(lib: string) { /** * Encodes a C string. */ -export function cstr(str: string): Uint8Array { +export function cstr(str: string): Uint8Array { const buf = new Uint8Array(str.length + 1); encoder.encodeInto(str, buf); return buf; diff --git a/test/test_with_gc.ts b/test/test_with_gc.ts new file mode 100644 index 0000000..d8724b9 --- /dev/null +++ b/test/test_with_gc.ts @@ -0,0 +1,48 @@ +import python, { Callback } from "../mod.ts"; +import { assertEquals } from "./asserts.ts"; + +Deno.test("callbacks are not gc'd while still needed by python", () => { + const pyModule = python.runModule( + ` +stored_callback = None + +def store_and_call_callback(cb): + global stored_callback + stored_callback = cb + return stored_callback() + +def call_stored_callback(): + global stored_callback + if stored_callback is None: + 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 + const callbackObj = new Callback(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(); + } + + // 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); + callbackObj.destroy(); +});