Order our deallocations during shutdown

Author: timfel

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/CApiContext.java

@@ -1114,14 +1114,16 @@ public void finalizeCApi() {
         try {
             // TODO(fa): remove GIL acquisition (GR-51314)
             try (GilNode.UncachedAcquire ignored = GilNode.uncachedAcquire()) {
+                // First we want to free all replacements for which we have to call tp_dealloc,
+                // while all our stubs are still available for the tp_dealloc code to run.
+                CApiTransitions.deallocNativeReplacements(context, handleContext);
+                // The singletons can be freed now
                 freeSingletonNativeWrappers(handleContext);
-                /*
-                 * Clear all remaining native object stubs. This must be done after the small int
-                 * and the singleton wrappers were cleared because they might also end up in the
-                 * lookup table and may otherwise be double-freed.
-                 */
-                CApiTransitions.freeNativeReplacementStructs(context, handleContext);
+                // Now we can clear all native memory that was simply allocated from Java. This
+                // must be done after the the singleton wrappers were cleared because they might
+                // also end up in the lookup table and may otherwise be double-freed.
                 CApiTransitions.freeNativeObjectStubs(handleContext);
+                CApiTransitions.freeNativeReplacementStructs(context, handleContext);
                 CApiTransitions.freeNativeStorages(handleContext);
             }
             if (pyDateTimeCAPICapsule != null) {

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/CApiTransitions.java

@@ -49,6 +49,8 @@
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map.Entry;
 import java.util.Set;
 import java.util.WeakHashMap;
 import java.util.concurrent.ConcurrentHashMap;
@@ -643,26 +645,34 @@ private static boolean tableEntryRemoved(HandleContext context, PythonNativeWrap
         return true;
     }
 
-    public static void freeNativeReplacementStructs(PythonContext context, HandleContext handleContext) {
+    public static void deallocNativeReplacements(PythonContext context, HandleContext handleContext) {
         assert context.ownsGil();
         ArrayList<Long> referencesToBeFreed = new ArrayList<>();
+        Iterator<Entry<Long, IdReference<?>>> iterator = handleContext.nativeLookup.entrySet().iterator();
+        while (iterator.hasNext()) {
+            IdReference<?> ref = iterator.next().getValue();
+            if (ref instanceof PythonObjectReference reference) {
+                if (!reference.isAllocatedFromJava()) {
+                    // This memory must be free from C
+                    referencesToBeFreed.add(reference.pointer);
+                    iterator.remove();
+                }
+            }
+        }
+        releaseNativeObjects(context, referencesToBeFreed);
+    }
+
+    public static void freeNativeReplacementStructs(PythonContext context, HandleContext handleContext) {
+        assert context.ownsGil();
         handleContext.nativeLookup.forEach((l, ref) -> {
             if (ref instanceof PythonObjectReference reference) {
                 // We don't expect references to wrappers that would have a native object stub.
                 assert reference.handleTableIndex == -1;
-                /*
-                 * The ref may denote: (a) class wrappers, where some of them are backed by static
-                 * native memory and some of them were allocated in heap, and (b) struct wrappers,
-                 * which may be freed manually in a separate step.
-                 */
-                if (reference.isAllocatedFromJava()) {
-                    freeNativeStruct(reference);
-                } else {
-                    referencesToBeFreed.add(reference.pointer);
-                }
+                // We expect at this point that there are only references allocated from Java
+                assert reference.isAllocatedFromJava();
+                freeNativeStruct(reference);
             }
         });
-        releaseNativeObjects(context, referencesToBeFreed);
         handleContext.nativeLookup.clear();
     }