From 27965e2667064242c2613fb991b28ede15ce1ee6 Mon Sep 17 00:00:00 2001 From: Tim Felgentreff Date: Fri, 8 May 2026 15:04:42 +0200 Subject: [PATCH] [GR-50212] Free native weakrefs during shutdown --- .../src/tests/cpyext/test_shutdown.py | 123 +++++++++++++++++- .../cext/PythonCextWeakrefBuiltins.java | 5 +- .../capi/transitions/CApiTransitions.java | 19 ++- 3 files changed, 140 insertions(+), 7 deletions(-) diff --git a/graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_shutdown.py b/graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_shutdown.py index c3f7e6cae4..090d3a89fd 100644 --- a/graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_shutdown.py +++ b/graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_shutdown.py @@ -40,16 +40,23 @@ import os import signal import subprocess +import tempfile +import textwrap from pathlib import Path import sys +from unittest import skipIf +from tests import compile_module_from_string +from tests.util import skip_if_sandboxed + +GRAALPY = sys.implementation.name == 'graalpy' DIR = Path(__file__).parent MODULE_PATH = DIR / 'module_with_native_destructor.py' ENV = dict(os.environ) ENV['PYTHONPATH'] = str(DIR.parent.parent) ARGS = [] -if sys.implementation.name == 'graalpy': +if GRAALPY: ARGS = ['--experimental-options', f'--log.file={os.devnull}', '--python.EnableDebuggingBuiltins'] if not __graalpython__.is_native: ARGS += [f'--vm.Djdk.graal.LogFile={os.devnull}'] @@ -68,3 +75,117 @@ def test_sigterm(): assert proc.stdout.read(len(expected)) == expected proc.terminate() assert proc.wait() in [-signal.SIGTERM, 128 + signal.SIGTERM] + + +@skip_if_sandboxed("Needs native extension support in sandboxed runs") +@skipIf(not GRAALPY, "GraalPy-only native weakref shutdown test") +def test_native_weakref_shutdown_with_c_retained_object(): + module = compile_module_from_string(textwrap.dedent(""" + #include "Python.h" + #include "structmember.h" + #include + #include + #include + + typedef struct { + PyObject_HEAD + PyObject *weakreflist; + } NativeWeakRefObject; + + static PyObject *kept_alive; + + static void write_marker(const char *contents) { + const char *marker_path = getenv("GR50212_DEALLOC_MARKER"); + if (marker_path != NULL) { + FILE *marker = fopen(marker_path, "w"); + if (marker != NULL) { + fputs(contents, marker); + fclose(marker); + } + } + } + + static void NativeWeakRef_dealloc(NativeWeakRefObject *self) { + write_marker("deallocated\\n"); + if (self->weakreflist != NULL) { + PyObject_ClearWeakRefs((PyObject *)self); + } + Py_TYPE(self)->tp_free((PyObject *)self); + } + + static PyTypeObject NativeWeakRefType = { + PyVarObject_HEAD_INIT(NULL, 0) + .tp_name = "native_weakref_shutdown_reproducer_gr50212.NativeWeakRef", + .tp_basicsize = sizeof(NativeWeakRefObject), + .tp_dealloc = (destructor)NativeWeakRef_dealloc, + .tp_flags = Py_TPFLAGS_DEFAULT, + .tp_new = PyType_GenericNew, + .tp_weaklistoffset = offsetof(NativeWeakRefObject, weakreflist), + }; + + static PyObject *hold(PyObject *self, PyObject *Py_UNUSED(ignored)) { + Py_CLEAR(kept_alive); + kept_alive = PyObject_CallNoArgs((PyObject *)&NativeWeakRefType); + if (kept_alive == NULL) { + return NULL; + } + write_marker("held\\n"); + return Py_NewRef(kept_alive); + } + + static PyMethodDef methods[] = { + {"hold", hold, METH_NOARGS, ""}, + {NULL, NULL, 0, NULL}, + }; + + static PyModuleDef module = { + PyModuleDef_HEAD_INIT, + "native_weakref_shutdown_reproducer_gr50212", + "", + -1, + methods, + NULL, NULL, NULL, NULL + }; + + PyMODINIT_FUNC PyInit_native_weakref_shutdown_reproducer_gr50212(void) { + if (PyType_Ready(&NativeWeakRefType) < 0) { + return NULL; + } + PyObject *m = PyModule_Create(&module); + if (m == NULL) { + return NULL; + } + Py_INCREF(&NativeWeakRefType); + if (PyModule_AddObject(m, "NativeWeakRef", (PyObject *)&NativeWeakRefType) < 0) { + Py_DECREF(&NativeWeakRefType); + Py_DECREF(m); + return NULL; + } + return m; + } + """), "native_weakref_shutdown_reproducer_gr50212") + module_dir = str(Path(module.__file__).parent) + args = [ + sys.executable, + '--experimental-options', + '--python.EnableDebuggingBuiltins', + ] + if not __graalpython__.is_native: + args += [f'--vm.Dpython.EnableBytecodeDSLInterpreter={str(__graalpython__.is_bytecode_dsl_interpreter).lower()}'] + code = textwrap.dedent(""" + import weakref + import native_weakref_shutdown_reproducer_gr50212 + + obj = native_weakref_shutdown_reproducer_gr50212.hold() + wr = weakref.ref(obj) + type_wr = weakref.ref(native_weakref_shutdown_reproducer_gr50212.NativeWeakRef) + print(type(obj).__name__, flush=True) + """) + env = dict(ENV) + env["PYTHONPATH"] = os.pathsep.join([module_dir, env["PYTHONPATH"]]) + with tempfile.TemporaryDirectory() as tmpdir: + marker = Path(tmpdir) / "deallocated" + env["GR50212_DEALLOC_MARKER"] = str(marker) + proc = subprocess.run([*args, '-c', code], env=env, capture_output=True, text=True, check=True) + assert proc.stdout.strip() == "NativeWeakRef" + assert marker.read_text() == "deallocated\n" diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextWeakrefBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextWeakrefBuiltins.java index 5baaf836c6..ad3b013c86 100644 --- a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextWeakrefBuiltins.java +++ b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextWeakrefBuiltins.java @@ -56,6 +56,7 @@ import com.oracle.graal.python.builtins.modules.weakref.PProxyType; import com.oracle.graal.python.builtins.objects.PNone; import com.oracle.graal.python.builtins.objects.cext.capi.ExternalFunctionNodes.ToNativeBorrowedNode; +import com.oracle.graal.python.builtins.objects.cext.capi.transitions.CApiTransitions; import com.oracle.graal.python.builtins.objects.cext.capi.transitions.CApiTransitions.NativeToPythonNode; import com.oracle.graal.python.builtins.objects.cext.capi.transitions.CApiTransitions.PythonToNativeNewRefNode; import com.oracle.graal.python.builtins.objects.module.PythonModule; @@ -69,8 +70,8 @@ public final class PythonCextWeakrefBuiltins { @CApiBuiltin(ret = Void, args = {PyObjectRawPointer}, call = Direct, acquireGil = false, canRaise = false) - public static void PyObject_ClearWeakRefs(@SuppressWarnings("unused") long pyObject) { - // TODO: Implement + public static void PyObject_ClearWeakRefs(long pyObject) { + CApiTransitions.removeNativeWeakRef(PythonContext.get(null), pyObject); } @CApiBuiltin(ret = PyObjectTransfer, args = {PyObject, PyObject}, call = Direct, acquireGil = false, canRaise = true) diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/CApiTransitions.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/CApiTransitions.java index 871903525f..bceeb866c0 100644 --- a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/CApiTransitions.java +++ b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/CApiTransitions.java @@ -935,12 +935,23 @@ public static void deallocateNativeWeakRefs(PythonContext pythonContext) { assert pythonContext.ownsGil(); HandleContext context = pythonContext.handleContext; int idx = -1; - Object[] list = context.nativeWeakRef.values().toArray(); + Long[] list = context.nativeWeakRef.keySet().toArray(new Long[0]); context.nativeWeakRef.clear(); long[] ptrArray = new long[list.length]; - for (Object ptr : list) { - if (context.nativeLookup.containsKey(ptr)) { - ptrArray[++idx] = (Long) ptr; + for (Long ptr : list) { + if (ptr != NULLPTR) { + IdReference ref = nativeLookupGet(context, ptr); + Object object = ref != null ? ref.get() : null; + long refCount = ref != null ? readNativeRefCount(ptr) : 0; + /* + * Only native weakrefs with extra C references need shutdown deallocation here. + * Objects with just MANAGED_REFCNT are still owned by the managed wrapper. + */ + if (refCount > MANAGED_REFCNT && refCount != IMMORTAL_REFCNT && + (object == null || !TypeNodes.IsTypeNode.executeUncached(object))) { + nativeLookupRemove(context, ptr); + ptrArray[++idx] = ptr; + } } } if (idx != -1) {