fixes #1616 tighten up synchronization and conditional logic

Author: brettwooldridge

src/com/sun/jna/internal/Cleaner.java

@@ -27,6 +27,7 @@
 import java.lang.ref.PhantomReference;
 import java.lang.ref.Reference;
 import java.lang.ref.ReferenceQueue;
+import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -35,84 +36,80 @@
  * objects. It replaces the {@code Object#finalize} based resource deallocation
  * that is deprecated for removal from the JDK.
  *
- * <p><strong>This class is intented to be used only be JNA itself.</strong></p>
+ * <p><strong>This class is intended to be used only be JNA itself.</strong></p>
  */
 public class Cleaner {
     private static final Cleaner INSTANCE = new Cleaner();
+    private static final Logger logger = Logger.getLogger(Cleaner.class.getName());
+    private static final long CLEANER_LINGER_TIME = TimeUnit.SECONDS.toMillis(30);
 
     public static Cleaner getCleaner() {
         return INSTANCE;
     }
 
     private final ReferenceQueue<Object> referenceQueue;
-    private Thread cleanerThread;
+    private boolean cleanerRunning;
     private CleanerRef firstCleanable;
 
     private Cleaner() {
         referenceQueue = new ReferenceQueue<>();
     }
 
-    public synchronized Cleanable register(Object obj, Runnable cleanupTask) {
-        // The important side effect is the PhantomReference, that is yielded
-        // after the referent is GCed
-        return add(new CleanerRef(this, obj, referenceQueue, cleanupTask));
-    }
+    public synchronized Cleanable register(final Object obj, final Runnable cleanupTask) {
+        // The important side effect is the PhantomReference, that is yielded after the referent is GCed
+        final CleanerRef ref = new CleanerRef(obj, referenceQueue, cleanupTask);
 
-    private synchronized CleanerRef add(CleanerRef ref) {
-        synchronized (referenceQueue) {
-            if (firstCleanable == null) {
-                firstCleanable = ref;
-            } else {
-                ref.setNext(firstCleanable);
-                firstCleanable.setPrevious(ref);
-                firstCleanable = ref;
-            }
-            if (cleanerThread == null) {
-                Logger.getLogger(Cleaner.class.getName()).log(Level.FINE, "Starting CleanerThread");
-                cleanerThread = new CleanerThread();
-                cleanerThread.start();
-            }
-            return ref;
+        if (firstCleanable != null) {
+            ref.setNext(firstCleanable);
+            firstCleanable.setPrevious(ref);
+        }
+        firstCleanable = ref;
+
+        if (!cleanerRunning) {
+            logger.log(Level.FINE, "Starting CleanerThread");
+            Thread cleanerThread = new CleanerThread();
+            cleanerThread.start();
+            cleanerRunning = true;
         }
+
+        return ref;
     }
 
-    private synchronized boolean remove(CleanerRef ref) {
-        synchronized (referenceQueue) {
-            boolean inChain = false;
-            if (ref == firstCleanable) {
-                firstCleanable = ref.getNext();
-                inChain = true;
-            }
-            if (ref.getPrevious() != null) {
-                ref.getPrevious().setNext(ref.getNext());
-            }
-            if (ref.getNext() != null) {
-                ref.getNext().setPrevious(ref.getPrevious());
-            }
-            if (ref.getPrevious() != null || ref.getNext() != null) {
-                inChain = true;
-            }
-            ref.setNext(null);
-            ref.setPrevious(null);
-            return inChain;
+    private synchronized boolean remove(final CleanerRef ref) {
+        final CleanerRef prev = ref.getPrevious();
+        final CleanerRef next = ref.getNext();
+        boolean inChain = false;
+
+        if (ref == firstCleanable) {
+            firstCleanable = next;
+            inChain = true;
+        }
+        if (prev != null) {
+            prev.setNext(next);
+            inChain = true;
         }
+        if (next != null) {
+            next.setPrevious(prev);
+            inChain = true;
+        }
+        return inChain;
     }
 
     private static class CleanerRef extends PhantomReference<Object> implements Cleanable {
-        private final Cleaner cleaner;
         private final Runnable cleanupTask;
         private CleanerRef previous;
         private CleanerRef next;
 
-        public CleanerRef(Cleaner cleaner, Object referent, ReferenceQueue<? super Object> q, Runnable cleanupTask) {
-            super(referent, q);
-            this.cleaner = cleaner;
+        public CleanerRef(final Object referent, final ReferenceQueue<? super Object> queue, final Runnable cleanupTask) {
+            super(referent, queue);
             this.cleanupTask = cleanupTask;
         }
 
         @Override
         public void clean() {
-            if(cleaner.remove(this)) {
+            if (INSTANCE.remove(this)) {
+                previous = null;
+                next = null;
                 cleanupTask.run();
             }
         }
@@ -121,27 +118,25 @@ CleanerRef getPrevious() {
             return previous;
         }
 
-        void setPrevious(CleanerRef previous) {
+        void setPrevious(final CleanerRef previous) {
             this.previous = previous;
         }
 
         CleanerRef getNext() {
             return next;
         }
 
-        void setNext(CleanerRef next) {
+        void setNext(final CleanerRef next) {
             this.next = next;
         }
     }
 
-    public static interface Cleanable {
-        public void clean();
+    public interface Cleanable {
+        void clean();
     }
 
     private class CleanerThread extends Thread {
 
-        private static final long CLEANER_LINGER_TIME = 30000;
-
         public CleanerThread() {
             super("JNA Cleaner");
             setDaemon(true);
@@ -151,20 +146,19 @@ public CleanerThread() {
         public void run() {
             while (true) {
                 try {
-                    Reference<? extends Object> ref = referenceQueue.remove(CLEANER_LINGER_TIME);
+                    Reference<?> ref = referenceQueue.remove(CLEANER_LINGER_TIME);
                     if (ref instanceof CleanerRef) {
                         ((CleanerRef) ref).clean();
                     } else if (ref == null) {
-                        synchronized (referenceQueue) {
-                            Logger logger = Logger.getLogger(Cleaner.class.getName());
+                        synchronized (INSTANCE) {
                             if (firstCleanable == null) {
-                                cleanerThread = null;
                                 logger.log(Level.FINE, "Shutting down CleanerThread");
+                                cleanerRunning = false;
                                 break;
                             } else if (logger.isLoggable(Level.FINER)) {
                                 StringBuilder registeredCleaners = new StringBuilder();
-                                for(CleanerRef cleanerRef = firstCleanable; cleanerRef != null; cleanerRef = cleanerRef.next) {
-                                    if(registeredCleaners.length() != 0) {
+                                for (CleanerRef cleanerRef = firstCleanable; cleanerRef != null; cleanerRef = cleanerRef.next) {
+                                    if (registeredCleaners.length() != 0) {
                                         registeredCleaners.append(", ");
                                     }
                                     registeredCleaners.append(cleanerRef.cleanupTask.toString());
@@ -178,6 +172,9 @@ public void run() {
                     // our reference queue, well, there is no way to separate
                     // the two cases.
                     // https://groups.google.com/g/jna-users/c/j0fw96PlOpM/m/vbwNIb2pBQAJ
+                    synchronized (INSTANCE) {
+                        cleanerRunning = false;
+                    }
                     break;
                 } catch (Exception ex) {
                     Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex);