[GR-50212] Free native weakrefs during shutdown

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 <stddef.h>
+        #include <stdio.h>
+        #include <stdlib.h>
+
+        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"

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)

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) {