From 67d5b3eee15970714a446acbd9163954ba7cb43b Mon Sep 17 00:00:00 2001 From: Tim Felgentreff Date: Wed, 13 May 2026 17:28:28 +0200 Subject: [PATCH] [GR-50212] Avoid shutdown dealloc of native-owned weakrefs --- .../src/tests/cpyext/test_shutdown.py | 48 +++++++++++++++---- .../capi/transitions/CApiTransitions.java | 8 ++-- 2 files changed, 42 insertions(+), 14 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 090d3a89fd..9270af5914 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 @@ -79,7 +79,7 @@ def test_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(): +def test_native_weakref_shutdown_skips_c_retained_object(): module = compile_module_from_string(textwrap.dedent(""" #include "Python.h" #include "structmember.h" @@ -90,14 +90,16 @@ def test_native_weakref_shutdown_with_c_retained_object(): typedef struct { PyObject_HEAD PyObject *weakreflist; + int id; } NativeWeakRefObject; static PyObject *kept_alive; + static PyTypeObject NativeWeakRefType; 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"); + FILE *marker = fopen(marker_path, "a"); if (marker != NULL) { fputs(contents, marker); fclose(marker); @@ -105,8 +107,17 @@ def test_native_weakref_shutdown_with_c_retained_object(): } } + static PyObject *new_native_weakref(int id) { + NativeWeakRefObject *obj = (NativeWeakRefObject *)PyObject_CallNoArgs((PyObject *)&NativeWeakRefType); + if (obj == NULL) { + return NULL; + } + obj->id = id; + return (PyObject *)obj; + } + static void NativeWeakRef_dealloc(NativeWeakRefObject *self) { - write_marker("deallocated\\n"); + write_marker(self->id == 1 ? "deallocated:held\\n" : "deallocated:free\\n"); if (self->weakreflist != NULL) { PyObject_ClearWeakRefs((PyObject *)self); } @@ -125,16 +136,26 @@ def test_native_weakref_shutdown_with_c_retained_object(): static PyObject *hold(PyObject *self, PyObject *Py_UNUSED(ignored)) { Py_CLEAR(kept_alive); - kept_alive = PyObject_CallNoArgs((PyObject *)&NativeWeakRefType); + kept_alive = new_native_weakref(1); if (kept_alive == NULL) { return NULL; } - write_marker("held\\n"); + write_marker("created:held\\n"); return Py_NewRef(kept_alive); } + static PyObject *make(PyObject *self, PyObject *Py_UNUSED(ignored)) { + PyObject *obj = new_native_weakref(2); + if (obj == NULL) { + return NULL; + } + write_marker("created:free\\n"); + return obj; + } + static PyMethodDef methods[] = { {"hold", hold, METH_NOARGS, ""}, + {"make", make, METH_NOARGS, ""}, {NULL, NULL, 0, NULL}, }; @@ -173,13 +194,20 @@ def test_native_weakref_shutdown_with_c_retained_object(): if not __graalpython__.is_native: args += [f'--vm.Dpython.EnableBytecodeDSLInterpreter={str(__graalpython__.is_bytecode_dsl_interpreter).lower()}'] code = textwrap.dedent(""" + import gc import weakref import native_weakref_shutdown_reproducer_gr50212 - obj = native_weakref_shutdown_reproducer_gr50212.hold() - wr = weakref.ref(obj) + held = native_weakref_shutdown_reproducer_gr50212.hold() + free = native_weakref_shutdown_reproducer_gr50212.make() + held_wr = weakref.ref(held) + free_wr = weakref.ref(free) type_wr = weakref.ref(native_weakref_shutdown_reproducer_gr50212.NativeWeakRef) - print(type(obj).__name__, flush=True) + print(type(held).__name__, type(free).__name__, flush=True) + del free + for _ in range(3): + gc.collect() + print(held_wr() is not None, free_wr() is None, flush=True) """) env = dict(ENV) env["PYTHONPATH"] = os.pathsep.join([module_dir, env["PYTHONPATH"]]) @@ -187,5 +215,5 @@ def test_native_weakref_shutdown_with_c_retained_object(): 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" + assert proc.stdout.splitlines() == ["NativeWeakRef NativeWeakRef", "True True"] + assert marker.read_text().splitlines() == ["created:held", "created:free", "deallocated:free"] 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 72efc9f4e9..07676396c5 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 @@ -1025,11 +1025,11 @@ public static void deallocateNativeWeakRefs(PythonContext pythonContext) { 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. + * Only deallocate weakref referents whose managed wrapper has already been + * collected and that have no additional native owners. Forcing deallocation while + * native owners remain can leave dangling references for later shutdown DECREFs. */ - if (refCount > MANAGED_REFCNT && refCount != IMMORTAL_REFCNT && - (object == null || !TypeNodes.IsTypeNode.executeUncached(object))) { + if (object == null && refCount == MANAGED_REFCNT) { nativeLookupRemove(context, ptr); ptrArray[++idx] = ptr; }