Changes based on review.

Author: aardvark179

lib/truffle/truffle/cext.rb

@@ -1368,6 +1368,7 @@ def data_finalizer(free, data_holder)
   end
 
   def data_marker(mark, data_holder)
+    # In a separate method to avoid capturing the object
     raise unless mark.respond_to?(:call)
     proc { |obj|
       create_mark_list

src/main/java/org/truffleruby/cext/CExtNodes.java

@@ -1349,9 +1349,9 @@ protected UnwrapNode createUnwrapNode() {
     public abstract static class SetMarkList extends CoreMethodArrayArgumentsNode {
 
         @Specialization
-        public DynamicObject setMarkList(VirtualFrame frame, DynamicObject structOnwer,
+        public DynamicObject setMarkList(VirtualFrame frame, DynamicObject structOwwer,
                 @Cached("createWriter()") WriteObjectFieldNode writeMarkedNode) {
-            writeMarkedNode.write(structOnwer, getArray());
+            writeMarkedNode.write(structOwwer, getArray());
             return nil();
         }
 

src/main/java/org/truffleruby/cext/ValueWrapperManager.java

@@ -92,6 +92,7 @@ public synchronized long createNativeHandle(DynamicObject wrapper) {
         }
         Layouts.VALUE_WRAPPER.setHandle(wrapper, handleAddress);
         addToHandleMap(handleAddress, wrapper);
+        context.getMarkingService().keepObject(wrapper);
         addFinalizer(wrapper, handlePointer);
         return handleAddress;
     }

src/main/java/org/truffleruby/core/FinalizationService.java

@@ -157,27 +157,20 @@ protected String getThreadName() {
     protected void processReference(FinalizerReference finalizerReference) {
         super.processReference(finalizerReference);
 
-        try {
-            while (!context.isFinalizing()) {
-                final Finalizer finalizer;
-                synchronized (this) {
-                    finalizer = finalizerReference.getFirstFinalizer();
-                }
-                if (finalizer == null) {
-                    break;
-                }
-                final Runnable action = finalizer.getAction();
-                action.run();
+        runCatchingErrors(this::processReferenceInternal, finalizerReference);
+    }
+
+    protected void processReferenceInternal(FinalizerReference finalizerReference) {
+        while (!context.isFinalizing()) {
+            final Finalizer finalizer;
+            synchronized (this) {
+                finalizer = finalizerReference.getFirstFinalizer();
             }
-        } catch (TerminationException e) {
-            throw e;
-        } catch (RaiseException e) {
-            context.getCoreExceptions().showExceptionIfDebug(e.getException());
-        } catch (Exception e) {
-            // Do nothing, the finalizer thread must continue to process objects.
-            if (context.getCoreLibrary().getDebug() == Boolean.TRUE) {
-                e.printStackTrace();
+            if (finalizer == null) {
+                break;
             }
+            final Runnable action = finalizer.getAction();
+            action.run();
         }
     }
 

src/main/java/org/truffleruby/core/MarkingService.java

@@ -16,28 +16,27 @@
 import java.util.Deque;
 
 import org.truffleruby.RubyContext;
-import org.truffleruby.language.control.RaiseException;
-import org.truffleruby.language.control.TerminationException;
 
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.object.DynamicObject;
 
 /**
  * Class to provide GC marking and other facilities to keep objects alive for native extensions.
  *
- * Native extensions expect object on the stack to be kept alive even when they have been stored in
- * native structures on the stack. They also expect structs in objects with custom mark functions
- * keep marked objects alive.
+ * Native extensions expect objects on the stack to be kept alive even when they have been stored in
+ * native structures on the stack (e.g. pg keeps the VALUE of a ruby array in a structure on the
+ * stack, and places other objects in that array to keep them alive). They also expect structs in
+ * objects with custom mark functions to keep marked objects alive.
  *
- * Since we are not running on a VM that allows us to add custom ark functions to our garbage
+ * Since we are not running on a VM that allows us to add custom mark functions to our garbage
  * collector we keep objects alive in 2 ways. Any object converted to a native handle can be kept
- * alive by calling keepAlive(). This will add the object to two lists, a list of all objects
- * converted to native during in this call to a C extension which will be popped when the we return
- * to Ruby code, and a fixed sized list of objects converted to native handles. When the latter of
- * these two lists is full all mark functions will be run.
+ * alive by calling {@link #keepObject(Object)}. This will add the object to two lists, a list of
+ * all objects converted to native during this call to a C extension function which will be popped
+ * when the we return to Ruby code, and a fixed sized list of objects converted to native handles.
+ * When the latter of these two lists is full all mark functions will be run.
  *
- * Markers only keep a week reference to their owning object to ensure they don't themselves stop
- * the object from being garbage collected.
+ * Marker references only keep a week reference to their owning object to ensure they don't
+ * themselves stop the object from being garbage collected.
  *
  */
 public class MarkingService extends ReferenceProcessingService<MarkingService.MarkerReference> {
@@ -125,7 +124,7 @@ private synchronized void runAllMarkers() {
             nextMarker = currentMarker.next;
             runMarker(currentMarker);
             if (nextMarker == currentMarker) {
-                throw new Error("Something went badly wrong.");
+                throw new Error("The MarkerReference linked list structure has become broken.");
             }
             currentMarker = nextMarker;
         }
@@ -137,22 +136,15 @@ public void addMarker(DynamicObject object, MarkerAction action) {
     }
 
     private void runMarker(MarkerReference markerReference) {
-        try {
-            if (!context.isFinalizing()) {
-                DynamicObject owner = markerReference.get();
-                if (owner != null) {
-                    final MarkerAction action = markerReference.action;
-                    action.mark(owner);
-                }
-            }
-        } catch (TerminationException e) {
-            throw e;
-        } catch (RaiseException e) {
-            context.getCoreExceptions().showExceptionIfDebug(e.getException());
-        } catch (Exception e) {
-            // Do nothing, the finalizer thread must continue to process objects.
-            if (context.getCoreLibrary().getDebug() == Boolean.TRUE) {
-                e.printStackTrace();
+        runCatchingErrors(this::runMarkerInternal, markerReference);
+    }
+
+    private void runMarkerInternal(MarkerReference markerReference) {
+        if (!context.isFinalizing()) {
+            DynamicObject owner = markerReference.get();
+            if (owner != null) {
+                final MarkerAction action = markerReference.action;
+                action.mark(owner);
             }
         }
     }

src/main/java/org/truffleruby/core/ReferenceProcessingService.java

@@ -1,26 +1,29 @@
 package org.truffleruby.core;
 
 import java.lang.ref.ReferenceQueue;
+import java.util.function.Consumer;
 
 import org.truffleruby.RubyContext;
 import org.truffleruby.core.thread.ThreadManager;
+import org.truffleruby.language.control.RaiseException;
+import org.truffleruby.language.control.TerminationException;
 
 import com.oracle.truffle.api.object.DynamicObject;
 
-public abstract class ReferenceProcessingService<T extends ReferenceProcessingService.ProcessingReference<T>> {
+public abstract class ReferenceProcessingService<R extends ReferenceProcessingService.ProcessingReference<R>> {
 
-    public static interface ProcessingReference<U extends ProcessingReference<U>> {
-        public U getPrevious();
+    public static interface ProcessingReference<R extends ProcessingReference<R>> {
+        public R getPrevious();
 
-        public void setPrevious(U previous);
+        public void setPrevious(R previous);
 
-        public U getNext();
+        public R getNext();
 
-        public void setNext(U next);
+        public void setNext(R next);
     }
 
     /** The head of a doubly-linked list of FinalizerReference, needed to collect finalizer Procs for ObjectSpace. */
-    private T first = null;
+    private R first = null;
 
     protected final ReferenceQueue<Object> processingQueue = new ReferenceQueue<>();
 
@@ -34,7 +37,7 @@ public ReferenceProcessingService(RubyContext context) {
     protected final void drainReferenceQueue() {
         while (true) {
             @SuppressWarnings("unchecked")
-            final T reference = (T) processingQueue.poll();
+            final R reference = (R) processingQueue.poll();
 
             if (reference == null) {
                 break;
@@ -52,17 +55,32 @@ protected void createProcessingThread() {
         threadManager.initialize(processingThread, null, getThreadName(), () -> {
             while (true) {
                 @SuppressWarnings("unchecked")
-                final T reference = (T) threadManager.runUntilResult(null, processingQueue::remove);
+                final R reference = (R) threadManager.runUntilResult(null, processingQueue::remove);
 
                 processReference(reference);
             }
         });
     }
 
-    protected void processReference(T reference) {
+    protected void processReference(R reference) {
         remove(reference);
     }
 
+    protected void runCatchingErrors(Consumer<R> action, R reference) {
+        try {
+            action.accept(reference);
+        } catch (TerminationException e) {
+            throw e;
+        } catch (RaiseException e) {
+            context.getCoreExceptions().showExceptionIfDebug(e.getException());
+        } catch (Exception e) {
+            // Do nothing, the finalizer thread must continue to process objects.
+            if (context.getCoreLibrary().getDebug() == Boolean.TRUE) {
+                e.printStackTrace();
+            }
+        }
+    }
+
     protected abstract String getThreadName();
 
     protected void processReferenceQueue() {
@@ -89,7 +107,7 @@ protected void processReferenceQueue() {
         }
     }
 
-    protected synchronized void remove(T ref) {
+    protected synchronized void remove(R ref) {
         if (ref.getNext() == ref) {
             // Already removed.
             return;
@@ -116,15 +134,15 @@ protected synchronized void remove(T ref) {
         ref.setPrevious(ref);
     }
 
-    protected synchronized void add(T newRef) {
+    protected synchronized void add(R newRef) {
         if (first != null) {
             newRef.setNext(first);
             first.setPrevious(newRef);
         }
         first = newRef;
     }
 
-    protected T getFirst() {
+    protected R getFirst() {
         return first;
     }
 }