Use Cleaner instead of finalizer in core JNA and JNA-platform

The following uses of finalizers were replaced by a Cleaner or removed
because they are unneeded:

- c.s.j.Memory#finalize
- c.s.j.CallbackReference#finalize
- c.s.j.NativeLibrary#finalize

The cleanup handling is moved to the #close method and #dispose is being
deprecated.

Anyone relying on #dispose being called from the GC has to take action
now! If a subclass needs a cleanup after this object goes out of scope,
it will need to register a cleaner.

CHANGES.md

@@ -8,10 +8,18 @@ Next Release (5.12.0)
 Features
 --------
 * [#1433](https://github.com/java-native-access/jna/pull/1433): Add `CFEqual`, `CFDictionaryRef.ByReference`, `CFStringRef.ByReference` to `c.s.j.p.mac.CoreFoundation` - [@shalupov](https://github.com/shalupov)
+* [#978](https://github.com/java-native-access/jna/issues/978): Remove use of finalizers in JNA and improve concurrency for `Memory`, `CallbackReference` and `NativeLibrary` - [@matthiasblaesing](https://github.com/matthiasblaesing).
 
 Bug Fixes
 ---------
 
+Important Changes
+-----------------
+* `Memory#dispose`, `CallbackReference#dispose` and `NativeLibrary#dispose`
+   were called by the `Object#finalize` override. These calls were replaced by
+   the use of a cleaner. It is not guaranteed anymore, that `dispose` is called
+   on subclasses on finalization.
+
 Release 5.11.0
 ==============
 

contrib/platform/test/com/sun/jna/platform/win32/Crypt32UtilClearSecuritySensitiveDataTest.java

@@ -34,6 +34,8 @@
 import org.junit.rules.ErrorCollector;
 
 import com.sun.jna.Memory;
+import java.lang.ref.Reference;
+import java.util.Map;
 
 import static org.hamcrest.CoreMatchers.is;
 
@@ -42,37 +44,22 @@
  */
 public class Crypt32UtilClearSecuritySensitiveDataTest {
     @Rule public ErrorCollector errors = new ErrorCollector();
-    Field fieldMemory_HEAD;
-    Field fieldLinkedReference_next;
-    Field fieldLinkedReference_prev;
+    Field allocatedMemory;
 
     @Before
     public void setUp() throws NoSuchFieldException, ClassNotFoundException {
-        fieldMemory_HEAD = Memory.class.getDeclaredField("HEAD");
-        fieldMemory_HEAD.setAccessible(true);
-        Class<?> classLinkedReference = Class.forName("com.sun.jna.Memory$LinkedReference");
-        fieldLinkedReference_next = classLinkedReference.getDeclaredField("next");
-        fieldLinkedReference_next.setAccessible(true);
-        fieldLinkedReference_prev = classLinkedReference.getDeclaredField("prev");
-        fieldLinkedReference_prev.setAccessible(true);
+        allocatedMemory = Memory.class.getDeclaredField("allocatedMemory");
+        allocatedMemory.setAccessible(true);
     }
 
     boolean stillHover(byte[] sample) throws IllegalAccessException {
-        Object head = fieldMemory_HEAD.get(null);
-        return stillHover(sample, head, fieldLinkedReference_next) || stillHover(sample, head, fieldLinkedReference_prev);
-    }
-
-    boolean stillHover(byte[] sample, Object head, Field fieldNext) throws IllegalAccessException {
-        Object next = head;
-        do {
-            Object curr = next;
-            Memory memory = (Memory) ((WeakReference<?>) curr).get();
+        for(Reference<Memory> memRef: ((Map<Long, Reference<Memory>>) allocatedMemory.get(null)).values()) {
+            Memory memory = memRef.get();
             byte[] array = memory.getByteArray(0, (int) memory.size());
             if (Arrays.equals(array, sample)) {
                 return true;
             }
-            next = fieldNext.get(curr);
-        } while (next != null);
+        }
         return false;
     }
 

src/com/sun/jna/Callback.java

@@ -27,15 +27,15 @@
 import java.util.Collections;
 import java.util.List;
 
-/** All callback definitions must derive from this interface.  Any
- * derived interfaces must define a single public method (which may not be named
+/**
+ * All callback definitions must derive from this interface. Any derived
+ * interfaces must define a single public method (which may not be named
  * "hashCode", "equals", or "toString"), or one public method named "callback".
- * You are responsible for deregistering your callback (if necessary)
- * in its {@link Object#finalize} method.  If native code attempts to call
- * a callback which has been GC'd, you will likely crash the VM.  If
- * there is no method to deregister the callback (e.g. <code>atexit</code>
- * in the C library), you must ensure that you always keep a live reference
- * to the callback object.<p>
+ * You are responsible for deregistering your callback (if necessary). If native
+ * code attempts to call a callback which has been GC'd, you will likely crash
+ * the VM. If there is no method to deregister the callback (e.g.
+ * <code>atexit</code> in the C library), you must ensure that you always keep a
+ * live reference to the callback object.<p>
  * A callback should generally never throw an exception, since it doesn't
  * necessarily have an encompassing Java environment to catch it.  Any
  * exceptions thrown will be passed to the default callback exception

src/com/sun/jna/CallbackReference.java

@@ -23,6 +23,8 @@
  */
 package com.sun.jna;
 
+import com.sun.jna.internal.Cleaner;
+import java.io.Closeable;
 import java.lang.ref.Reference;
 import java.lang.ref.WeakReference;
 import java.lang.reflect.InvocationHandler;
@@ -31,7 +33,6 @@
 import java.lang.reflect.Proxy;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -40,12 +41,15 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.WeakHashMap;
+import java.util.Collections;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
- * Provides a reference to an association between a native callback closure and
- * a Java {@link Callback} closure.
+ * Provides a reference to an association between a native callback closure
+ * and a Java {@link Callback} closure.
  */
-public class CallbackReference extends WeakReference<Callback> {
+
+public class CallbackReference extends WeakReference<Callback> implements Closeable {
 
     // Access to callbackMap, directCallbackMap, pointerCallbackMap is protected
     // by synchonizing on pointerCallbackMap
@@ -57,8 +61,8 @@ public class CallbackReference extends WeakReference<Callback> {
     static final Map<Object, Object> allocations =
             Collections.synchronizedMap(new WeakHashMap<Object, Object>());
     // Global map of allocated closures to facilitate centralized cleanup
-    private static final Map<CallbackReference, Reference<CallbackReference>> allocatedMemory =
-            Collections.synchronizedMap(new WeakHashMap<CallbackReference, Reference<CallbackReference>>());
+    private static final Map<Long, Reference<CallbackReference>> allocatedMemory =
+            new ConcurrentHashMap<Long, Reference<CallbackReference>>();
     private static final Method PROXY_CALLBACK_METHOD;
 
     static {
@@ -220,6 +224,8 @@ private static Callback createCallback(Class<?> type, Pointer p) {
         return (Callback)Proxy.newProxyInstance(type.getClassLoader(), new Class[] { type }, h);
     }
 
+
+    Cleaner.Cleanable cleanable;
     Pointer cbstruct;
     Pointer trampoline;
     // Keep a reference to the proxy to avoid premature GC of it
@@ -323,7 +329,10 @@ private CallbackReference(Callback callback, int callingConvention, boolean dire
                                                encoding);
         }
         cbstruct = peer != 0 ? new Pointer(peer) : null;
-        allocatedMemory.put(this, new WeakReference<CallbackReference>(this));
+        if(peer != 0) {
+            allocatedMemory.put(peer, new WeakReference<CallbackReference>(this));
+            cleanable = Cleaner.getCleaner().register(this, new CallbackReferenceDisposer(cbstruct));
+        }
     }
 
     private Class<?> getNativeType(Class<?> cls) {
@@ -431,31 +440,28 @@ public Pointer getTrampoline() {
         return trampoline;
     }
 
-    /** Free native resources associated with this callback when GC'd. */
-    @Override
-    protected void finalize() {
-        dispose();
-    }
-
     /** Free native resources associated with this callback. */
-    protected synchronized void dispose() {
-        if (cbstruct != null) {
-            try {
-                Native.freeNativeCallback(cbstruct.peer);
-            } finally {
-                cbstruct.peer = 0;
-                cbstruct = null;
-                allocatedMemory.remove(this);
-            }
+    public void close() {
+        if (cleanable != null) {
+            cleanable.clean();
         }
+        cbstruct = null;
+    }
+
+    @Deprecated
+    protected void dispose() {
+        close();
     }
 
     /** Dispose of all memory allocated for callbacks. */
     static void disposeAll() {
         // use a copy since dispose() modifes the map
-        Collection<CallbackReference> refs = new LinkedList<CallbackReference>(allocatedMemory.keySet());
-        for (CallbackReference r : refs) {
-            r.dispose();
+        Collection<Reference<CallbackReference>> refs = new LinkedList<Reference<CallbackReference>>(allocatedMemory.values());
+        for (Reference<CallbackReference> r : refs) {
+            CallbackReference ref = r.get();
+            if(ref != null) {
+                ref.close();
+            }
         }
     }
 
@@ -770,4 +776,26 @@ private static Pointer getNativeString(Object value, boolean wide) {
         }
         return null;
     }
+
+    private static final class CallbackReferenceDisposer implements Runnable {
+
+        private Pointer cbstruct;
+
+        public CallbackReferenceDisposer(Pointer cbstruct) {
+            this.cbstruct = cbstruct;
+        }
+
+        public synchronized void run() {
+            if (cbstruct != null) {
+                try {
+                    Native.freeNativeCallback(cbstruct.peer);
+                } finally {
+                    allocatedMemory.remove(cbstruct.peer);
+                    cbstruct.peer = 0;
+                    cbstruct = null;
+                }
+            }
+        }
+
+    }
 }