From 445651340a121788c89f5b0f589a4f1509e9d1b1 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Tue, 1 Aug 2023 11:51:28 +0200 Subject: [PATCH 01/14] Added test case for cleaner thread overload --- build.xml | 1 + .../jna/different_package/CleanerTest.java | 47 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 test/com/sun/jna/different_package/CleanerTest.java diff --git a/build.xml b/build.xml index 5ba1b0e143..a528e1a0f4 100644 --- a/build.xml +++ b/build.xml @@ -1232,6 +1232,7 @@ cd .. + diff --git a/test/com/sun/jna/different_package/CleanerTest.java b/test/com/sun/jna/different_package/CleanerTest.java new file mode 100644 index 0000000000..e1af9c250d --- /dev/null +++ b/test/com/sun/jna/different_package/CleanerTest.java @@ -0,0 +1,47 @@ +package com.sun.jna.different_package; + +import com.sun.jna.Structure; +import junit.framework.TestCase; + +import java.util.ArrayList; +import java.util.List; + +public class CleanerTest extends TestCase { + private static final int NUM_THREADS = 16; + private static final long ITERATIONS = 100000; + + @Structure.FieldOrder({"bytes"}) + public static class Dummy extends Structure { + public byte[] bytes; + + public Dummy() {} + + public Dummy(byte[] what) { bytes = what; } + } + + private static class Allocator implements Runnable { + @Override + public void run() { + for (long i = 0; i < ITERATIONS; ++i) { + Dummy d = new Dummy(new byte[1024]); + d.write(); + } + } + } + + public void testOOM() { + List threads = new ArrayList<>(NUM_THREADS); + for (int i = 0; i < NUM_THREADS; ++i) { + Thread t = new Thread(new Allocator()); + t.start(); + threads.add(t); + } + for (Thread t : threads) { + while (t.isAlive()) { + try { + t.join(); + } catch (InterruptedException ignore) {} + } + } + } +} From cebfafa86ff33ea417fdc3aacf2d15861d6dcbcd Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Wed, 2 Aug 2023 19:46:17 +0200 Subject: [PATCH 02/14] Re-implemented Cleaner --- src/com/sun/jna/internal/Cleaner.java | 286 ++++++++++++++++---------- test/com/sun/jna/CallbacksTest.java | 7 +- 2 files changed, 177 insertions(+), 116 deletions(-) diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index a2095937fc..6479ac1840 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -27,6 +27,11 @@ import java.lang.ref.PhantomReference; import java.lang.ref.Reference; import java.lang.ref.ReferenceQueue; +import java.util.Iterator; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; import java.util.logging.Level; import java.util.logging.Logger; @@ -35,154 +40,209 @@ * objects. It replaces the {@code Object#finalize} based resource deallocation * that is deprecated for removal from the JDK. * - *

This class is intented to be used only be JNA itself.

+ *

This class is intended to be used only be JNA itself.

*/ public class Cleaner { - private static final Cleaner INSTANCE = new Cleaner(); - - public static Cleaner getCleaner() { - return INSTANCE; - } + /* General idea: + * + * There's one Cleaner per thread, kept in a ThreadLocal static variable. + * This instance handles all to-be-cleaned objects registered by this + * thread. Whenever the thread registers another object, it first checks + * if there are references in the queue and cleans them up, then continues + * with the registration. + * + * This leaves two cases open, for which we employ a "Master Cleaner" and + * a separate cleaning thread. + * 1. If a long-lived thread registers some objects in the beginning, but + * then stops registering more objects, the previously registered + * objects will never be cleared. + * 2. When a thread exists before all its registered objects have been + * cleared, the ThreadLocal instance are lost, and so are the pending + * objects. + * + * The Master Cleaner handles the first issue by regularly handling the + * queues of the Cleaners registered with it. + * The seconds issue is handled by registering the per-thread Cleaner + * instances with the Master's reference queue. + */ + + private static class CleanerImpl { + protected final ReferenceQueue referenceQueue = new ReferenceQueue(); + protected final Map cleanables = new ConcurrentHashMap(); + private final AtomicBoolean lock = new AtomicBoolean(false); + + private void cleanQueue() { + if (lock.compareAndSet(false, true)) { + try { + Reference ref; + while ((ref = referenceQueue.poll()) != null) { + try { + if (ref instanceof Cleanable) { + ((Cleanable) ref).clean(); + } + } catch (RuntimeException ex) { + Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex); + } + } + } finally { + lock.set(false); + } + } + } - private final ReferenceQueue referenceQueue; - private Thread cleanerThread; - private CleanerRef firstCleanable; + public Cleanable register(Object obj, Runnable cleanupTask) { + cleanQueue(); + // The important side effect is the PhantomReference, that is yielded + // after the referent is GCed + return new CleanerRef(this, obj, referenceQueue, cleanupTask); + } - private Cleaner() { - referenceQueue = new ReferenceQueue<>(); - } + protected void put(long n, CleanerRef ref) { + cleanables.put(n, ref); + } - 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)); + protected boolean remove(long n) { + return cleanables.remove(n) != null; + } } - 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; + private static class MasterCleanerImpl extends CleanerImpl { + @Override + protected synchronized void put(long n, CleanerRef ref) { + super.put(n, 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; + @Override + protected synchronized boolean remove(long n) { + return super.remove(n); } } - private static class CleanerRef extends PhantomReference implements Cleanable { - private final Cleaner cleaner; - private final Runnable cleanupTask; - private CleanerRef previous; - private CleanerRef next; + public static class MasterCleaner extends Cleaner { + public static final long CLEANUP_INTERVAL_MS = 5000; + public static final long MAX_LINGER_MS = 30000; - public CleanerRef(Cleaner cleaner, Object referent, ReferenceQueue q, Runnable cleanupTask) { - super(referent, q); - this.cleaner = cleaner; - this.cleanupTask = cleanupTask; + private static MasterCleaner INSTANCE; + + public static synchronized void add(Cleaner cleaner) { + if (INSTANCE == null) { + INSTANCE = new MasterCleaner(); + } + final CleanerImpl impl = cleaner.impl; + INSTANCE.cleanerImpls.put(impl, true); + INSTANCE.register(cleaner, new Runnable() { + @Override + public void run() { + INSTANCE.cleanerImpls.put(impl, false); + } + }); } - @Override - public void clean() { - if(cleaner.remove(this)) { - cleanupTask.run(); + private static synchronized boolean deleteIfEmpty() { + if (INSTANCE != null && INSTANCE.cleanerImpls.isEmpty()) { + INSTANCE = null; + return true; } + return false; } - CleanerRef getPrevious() { - return previous; + private final Map cleanerImpls = new ConcurrentHashMap(); + private long lastNonEmpty = System.currentTimeMillis(); + + private MasterCleaner() { + super(true); + Thread cleanerThread = new Thread() { + @Override + public void run() { + long now; + while ((now = System.currentTimeMillis()) < lastNonEmpty + MAX_LINGER_MS || !deleteIfEmpty()) { + if (!cleanerImpls.isEmpty()) { lastNonEmpty = now; } + try { + Reference ref = impl.referenceQueue.remove(CLEANUP_INTERVAL_MS); + if(ref instanceof CleanerRef) { + ((CleanerRef) ref).clean(); + } else { + masterCleanup(); + } + } catch (InterruptedException ex) { + // Can be raised on shutdown. If anyone else messes with + // our reference queue, well, there is no way to separate + // the two cases. + // https://groups.google.com/g/jna-users/c/j0fw96PlOpM/m/vbwNIb2pBQAJ + break; + } catch (Exception ex) { + Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex); + } + } + } + }; + cleanerThread.setName("JNA Cleaner"); + cleanerThread.setDaemon(true); + cleanerThread.start(); } - void setPrevious(CleanerRef previous) { - this.previous = previous; + private void masterCleanup() { + Iterator> it = cleanerImpls.entrySet().iterator(); + while (it.hasNext()) { + Map.Entry entry = it.next(); + entry.getKey().cleanQueue(); + if (!entry.getValue() && entry.getKey().cleanables.isEmpty()) { + it.remove(); + } + } } + } - CleanerRef getNext() { - return next; + private static final ThreadLocal MY_INSTANCE = new ThreadLocal() { + @Override + protected Cleaner initialValue() { + return new Cleaner(false); } + }; - void setNext(CleanerRef next) { - this.next = next; + public static Cleaner getCleaner() { + return MY_INSTANCE.get(); + } + + protected final CleanerImpl impl; + + private Cleaner(boolean master) { + if (master) { + impl = new MasterCleanerImpl(); + } else { + impl = new CleanerImpl(); + MasterCleaner.add(this); } } - public static interface Cleanable { - public void clean(); + public Cleanable register(Object obj, Runnable cleanupTask) { + return impl.register(obj, cleanupTask); } - private class CleanerThread extends Thread { + private static class CleanerRef extends PhantomReference implements Cleanable { + private static final AtomicLong COUNTER = new AtomicLong(Long.MIN_VALUE); - private static final long CLEANER_LINGER_TIME = 30000; + private final CleanerImpl cleaner; + private final long number = COUNTER.incrementAndGet(); + private Runnable cleanupTask; - public CleanerThread() { - super("JNA Cleaner"); - setDaemon(true); + public CleanerRef(CleanerImpl impl, Object referent, ReferenceQueue q, Runnable cleanupTask) { + super(referent, q); + this.cleaner = impl; + this.cleanupTask = cleanupTask; + cleaner.put(number, this); } @Override - public void run() { - while (true) { - try { - 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()); - if (firstCleanable == null) { - cleanerThread = null; - logger.log(Level.FINE, "Shutting down CleanerThread"); - break; - } else if (logger.isLoggable(Level.FINER)) { - StringBuilder registeredCleaners = new StringBuilder(); - for(CleanerRef cleanerRef = firstCleanable; cleanerRef != null; cleanerRef = cleanerRef.next) { - if(registeredCleaners.length() != 0) { - registeredCleaners.append(", "); - } - registeredCleaners.append(cleanerRef.cleanupTask.toString()); - } - logger.log(Level.FINER, "Registered Cleaners: {0}", registeredCleaners.toString()); - } - } - } - } catch (InterruptedException ex) { - // Can be raised on shutdown. If anyone else messes with - // our reference queue, well, there is no way to separate - // the two cases. - // https://groups.google.com/g/jna-users/c/j0fw96PlOpM/m/vbwNIb2pBQAJ - break; - } catch (Exception ex) { - Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex); - } + public void clean() { + if(cleaner.remove(this.number) && cleanupTask != null) { + cleanupTask.run(); + cleanupTask = null; } } } + + public static interface Cleanable { + public void clean(); + } } diff --git a/test/com/sun/jna/CallbacksTest.java b/test/com/sun/jna/CallbacksTest.java index ff801fb7e5..212c8d7a5c 100644 --- a/test/com/sun/jna/CallbacksTest.java +++ b/test/com/sun/jna/CallbacksTest.java @@ -38,6 +38,7 @@ import com.sun.jna.Callback.UncaughtExceptionHandler; import com.sun.jna.CallbacksTest.TestLibrary.CbCallback; +import com.sun.jna.internal.Cleaner; import com.sun.jna.ptr.IntByReference; import com.sun.jna.ptr.PointerByReference; import com.sun.jna.win32.W32APIOptions; @@ -362,7 +363,7 @@ public void callback() { cb = null; System.gc(); - for (int i = 0; i < 100 && (ref.get() != null || refs.containsValue(ref)); ++i) { + for (int i = 0; i < Cleaner.MasterCleaner.CLEANUP_INTERVAL_MS / 10 + 5 && (ref.get() != null || refs.containsValue(ref)); ++i) { Thread.sleep(10); // Give the GC a chance to run System.gc(); } @@ -371,11 +372,11 @@ public void callback() { ref = null; System.gc(); - for (int i = 0; i < 100 && (cbstruct.peer != 0 || refs.size() > 0); ++i) { + for (int i = 0; i < Cleaner.MasterCleaner.CLEANUP_INTERVAL_MS / 10 + 5 && (cbstruct.peer != 0 || refs.size() > 0); ++i) { // Flush weak hash map refs.size(); try { - Thread.sleep(10); // Give the GC a chance to run + Thread.sleep(Cleaner.MasterCleaner.CLEANUP_INTERVAL_MS + 10); // Give the GC a chance to run System.gc(); } finally {} } From b04b83c6204f412d1ba3c7a5e6a6f72359503a35 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Tue, 15 Aug 2023 09:45:03 +0200 Subject: [PATCH 03/14] Added missing license header --- .../jna/different_package/CleanerTest.java | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/com/sun/jna/different_package/CleanerTest.java b/test/com/sun/jna/different_package/CleanerTest.java index e1af9c250d..aabc22bcb8 100644 --- a/test/com/sun/jna/different_package/CleanerTest.java +++ b/test/com/sun/jna/different_package/CleanerTest.java @@ -1,3 +1,26 @@ +/* Copyright (c) 2023 Peter Conrad, All Rights Reserved + * + * The contents of this file is dual-licensed under 2 + * alternative Open Source/Free licenses: LGPL 2.1 or later and + * Apache License 2.0. (starting with JNA version 4.0.0). + * + * You can freely decide which license you want to apply to + * the project. + * + * You may obtain a copy of the LGPL License at: + * + * http://www.gnu.org/licenses/licenses.html + * + * A copy is also included in the downloadable source code package + * containing JNA, in file "LGPL2.1". + * + * You may obtain a copy of the Apache License at: + * + * http://www.apache.org/licenses/ + * + * A copy is also included in the downloadable source code package + * containing JNA, in file "AL2.0". + */ package com.sun.jna.different_package; import com.sun.jna.Structure; From 8549647ca5db33cc4409b5610f4dc0f7577ff6a6 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Tue, 15 Aug 2023 10:02:44 +0200 Subject: [PATCH 04/14] Make MasterCleaner private --- src/com/sun/jna/internal/Cleaner.java | 16 ++++++++-------- test/com/sun/jna/CallbacksTest.java | 6 +++--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index 6479ac1840..41e1ebc514 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -56,8 +56,8 @@ public class Cleaner { * 1. If a long-lived thread registers some objects in the beginning, but * then stops registering more objects, the previously registered * objects will never be cleared. - * 2. When a thread exists before all its registered objects have been - * cleared, the ThreadLocal instance are lost, and so are the pending + * 2. When a thread exits before all its registered objects have been + * cleared, the ThreadLocal instance is lost, and so are the pending * objects. * * The Master Cleaner handles the first issue by regularly handling the @@ -66,6 +66,9 @@ public class Cleaner { * instances with the Master's reference queue. */ + public static final long MASTER_CLEANUP_INTERVAL_MS = 5000; + public static final long MASTER_MAX_LINGER_MS = 30000; + private static class CleanerImpl { protected final ReferenceQueue referenceQueue = new ReferenceQueue(); protected final Map cleanables = new ConcurrentHashMap(); @@ -118,10 +121,7 @@ protected synchronized boolean remove(long n) { } } - public static class MasterCleaner extends Cleaner { - public static final long CLEANUP_INTERVAL_MS = 5000; - public static final long MAX_LINGER_MS = 30000; - + private static class MasterCleaner extends Cleaner { private static MasterCleaner INSTANCE; public static synchronized void add(Cleaner cleaner) { @@ -155,10 +155,10 @@ private MasterCleaner() { @Override public void run() { long now; - while ((now = System.currentTimeMillis()) < lastNonEmpty + MAX_LINGER_MS || !deleteIfEmpty()) { + while ((now = System.currentTimeMillis()) < lastNonEmpty + MASTER_MAX_LINGER_MS || !deleteIfEmpty()) { if (!cleanerImpls.isEmpty()) { lastNonEmpty = now; } try { - Reference ref = impl.referenceQueue.remove(CLEANUP_INTERVAL_MS); + Reference ref = impl.referenceQueue.remove(MASTER_CLEANUP_INTERVAL_MS); if(ref instanceof CleanerRef) { ((CleanerRef) ref).clean(); } else { diff --git a/test/com/sun/jna/CallbacksTest.java b/test/com/sun/jna/CallbacksTest.java index 212c8d7a5c..44655b8434 100644 --- a/test/com/sun/jna/CallbacksTest.java +++ b/test/com/sun/jna/CallbacksTest.java @@ -363,7 +363,7 @@ public void callback() { cb = null; System.gc(); - for (int i = 0; i < Cleaner.MasterCleaner.CLEANUP_INTERVAL_MS / 10 + 5 && (ref.get() != null || refs.containsValue(ref)); ++i) { + for (int i = 0; i < Cleaner.MASTER_CLEANUP_INTERVAL_MS / 10 + 5 && (ref.get() != null || refs.containsValue(ref)); ++i) { Thread.sleep(10); // Give the GC a chance to run System.gc(); } @@ -372,11 +372,11 @@ public void callback() { ref = null; System.gc(); - for (int i = 0; i < Cleaner.MasterCleaner.CLEANUP_INTERVAL_MS / 10 + 5 && (cbstruct.peer != 0 || refs.size() > 0); ++i) { + for (int i = 0; i < Cleaner.MASTER_CLEANUP_INTERVAL_MS / 10 + 5 && (cbstruct.peer != 0 || refs.size() > 0); ++i) { // Flush weak hash map refs.size(); try { - Thread.sleep(Cleaner.MasterCleaner.CLEANUP_INTERVAL_MS + 10); // Give the GC a chance to run + Thread.sleep(Cleaner.MASTER_CLEANUP_INTERVAL_MS + 10); // Give the GC a chance to run System.gc(); } finally {} } From 1464d73d2212288a0ae5bb36fbd561152880a206 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Tue, 15 Aug 2023 10:50:26 +0200 Subject: [PATCH 05/14] Attempt to fix windows test failures --- test/com/sun/jna/CallbacksTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/com/sun/jna/CallbacksTest.java b/test/com/sun/jna/CallbacksTest.java index 44655b8434..cb6a7386f2 100644 --- a/test/com/sun/jna/CallbacksTest.java +++ b/test/com/sun/jna/CallbacksTest.java @@ -375,10 +375,10 @@ public void callback() { for (int i = 0; i < Cleaner.MASTER_CLEANUP_INTERVAL_MS / 10 + 5 && (cbstruct.peer != 0 || refs.size() > 0); ++i) { // Flush weak hash map refs.size(); - try { - Thread.sleep(Cleaner.MASTER_CLEANUP_INTERVAL_MS + 10); // Give the GC a chance to run + Thread.sleep(10); // Give the GC a chance to run + synchronized (CallbacksTest.class) { // the cbstruct.peer cleanup happens in a different thread, make sure we see it here System.gc(); - } finally {} + } } assertEquals("Callback trampoline not freed", 0, cbstruct.peer); } From 5f43c72db77fbd58a4804a84a1dc031a96aac21d Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Tue, 15 Aug 2023 10:56:34 +0200 Subject: [PATCH 06/14] Ensure that masterCleanup runs at least every 2*MASTER_CLEANUP_INTERVAL --- src/com/sun/jna/internal/Cleaner.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index 41e1ebc514..d9f5ad1efc 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -155,14 +155,18 @@ private MasterCleaner() { @Override public void run() { long now; + long lastMasterRun = 0; while ((now = System.currentTimeMillis()) < lastNonEmpty + MASTER_MAX_LINGER_MS || !deleteIfEmpty()) { if (!cleanerImpls.isEmpty()) { lastNonEmpty = now; } try { Reference ref = impl.referenceQueue.remove(MASTER_CLEANUP_INTERVAL_MS); if(ref instanceof CleanerRef) { ((CleanerRef) ref).clean(); - } else { + } + // "now" is not really *now* at this point, but off by no more than MASTER_CLEANUP_INTERVAL_MS + if (lastMasterRun + MASTER_CLEANUP_INTERVAL_MS <= now) { masterCleanup(); + lastMasterRun = now; } } catch (InterruptedException ex) { // Can be raised on shutdown. If anyone else messes with From b2e103779ecdf4a2050a5d3a9697d81b24e807f3 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Wed, 16 Aug 2023 15:38:37 +0200 Subject: [PATCH 07/14] Refactored GC wait loops --- test/com/sun/jna/CallbacksTest.java | 93 ++++++++++++++++------------- 1 file changed, 50 insertions(+), 43 deletions(-) diff --git a/test/com/sun/jna/CallbacksTest.java b/test/com/sun/jna/CallbacksTest.java index cb6a7386f2..38f4e6832b 100644 --- a/test/com/sun/jna/CallbacksTest.java +++ b/test/com/sun/jna/CallbacksTest.java @@ -34,7 +34,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.WeakHashMap; import com.sun.jna.Callback.UncaughtExceptionHandler; import com.sun.jna.CallbacksTest.TestLibrary.CbCallback; @@ -80,6 +79,24 @@ protected void waitFor(Thread thread) { } } + public static abstract class Condition { + protected final T obj; + + public Condition(T t) { obj = t; } + + public boolean evaluate() { return evaluate(obj); } + + abstract boolean evaluate(T t); + } + protected static void waitForGc(Condition condition) throws Exception { + for (int i = 0; i < 2 * Cleaner.MASTER_CLEANUP_INTERVAL_MS / 10 + 5 && condition.evaluate(); ++i) { + synchronized (CallbacksTest.class) { // cleanups happen in a different thread, make sure we see it here + System.gc(); + } + Thread.sleep(10); // Give the GC a chance to run + } + } + public static class SmallTestStructure extends Structure { public static final List FIELDS = createFieldsOrder("value"); public double value; @@ -355,31 +372,26 @@ public void callback() { lib.callVoidCallback(cb); assertTrue("Callback not called", called[0]); - Map refs = new WeakHashMap<>(callbackCache()); - assertTrue("Callback not cached", refs.containsKey(cb)); + assertTrue("Callback not cached", callbackCache().containsKey(cb)); + final Map refs = callbackCache(); CallbackReference ref = refs.get(cb); - refs = callbackCache(); - Pointer cbstruct = ref.cbstruct; + final Pointer cbstruct = ref.cbstruct; cb = null; - System.gc(); - for (int i = 0; i < Cleaner.MASTER_CLEANUP_INTERVAL_MS / 10 + 5 && (ref.get() != null || refs.containsValue(ref)); ++i) { - Thread.sleep(10); // Give the GC a chance to run - System.gc(); - } + waitForGc(new Condition(ref) { + public boolean evaluate(CallbackReference _ref) { + return _ref.get() != null || refs.containsValue(_ref); + } + }); assertNull("Callback not GC'd", ref.get()); assertFalse("Callback still in map", refs.containsValue(ref)); ref = null; - System.gc(); - for (int i = 0; i < Cleaner.MASTER_CLEANUP_INTERVAL_MS / 10 + 5 && (cbstruct.peer != 0 || refs.size() > 0); ++i) { - // Flush weak hash map - refs.size(); - Thread.sleep(10); // Give the GC a chance to run - synchronized (CallbacksTest.class) { // the cbstruct.peer cleanup happens in a different thread, make sure we see it here - System.gc(); + waitForGc(new Condition(null) { + public boolean evaluate(Object o) { + return refs.size() > 0 || cbstruct.peer != 0; } - } + }); assertEquals("Callback trampoline not freed", 0, cbstruct.peer); } @@ -679,7 +691,7 @@ public String callback(String arg, String arg2) { assertEquals("Wrong String return", VALUE + VALUE2, value); } - public void testStringCallbackMemoryReclamation() throws InterruptedException { + public void testStringCallbackMemoryReclamation() throws Exception { TestLibrary.StringCallback cb = new TestLibrary.StringCallback() { @Override public String callback(String arg, String arg2) { @@ -688,24 +700,22 @@ public String callback(String arg, String arg2) { }; // A little internal groping - Map m = CallbackReference.allocations; + final Map m = CallbackReference.allocations; m.clear(); Charset charset = Charset.forName(Native.getDefaultStringEncoding()); String arg = getName() + "1" + charset.decode(charset.encode(UNICODE)); String arg2 = getName() + "2" + charset.decode(charset.encode(UNICODE)); String value = lib.callStringCallback(cb, arg, arg2); - WeakReference ref = new WeakReference<>(value); + final WeakReference ref = new WeakReference<>(value); arg = null; value = null; - System.gc(); - for (int i = 0; i < 100 && (ref.get() != null || m.size() > 0); ++i) { - try { - Thread.sleep(10); // Give the GC a chance to run - System.gc(); - } finally {} - } + waitForGc(new Condition(null) { + public boolean evaluate(Object o) { + return m.size() > 0 || ref.get() != null; + } + }); assertNull("NativeString reference not GC'd", ref.get()); assertEquals("NativeString reference still held: " + m.values(), 0, m.size()); } @@ -1478,30 +1488,27 @@ public void callback() { assertEquals("Wrong module HANDLE for DLL function pointer", handle, pref.getValue()); // Check slot re-use - Map refs = new WeakHashMap<>(callbackCache()); - assertTrue("Callback not cached", refs.containsKey(cb)); + assertTrue("Callback not cached", callbackCache().containsKey(cb)); + final Map refs = callbackCache(); CallbackReference ref = refs.get(cb); - refs = callbackCache(); Pointer cbstruct = ref.cbstruct; Pointer first_fptr = cbstruct.getPointer(0); cb = null; - System.gc(); - for (int i = 0; i < 100 && (ref.get() != null || refs.containsValue(ref)); ++i) { - Thread.sleep(10); // Give the GC a chance to run - System.gc(); - } + waitForGc(new Condition(ref) { + public boolean evaluate(CallbackReference _ref) { + return _ref.get() != null || refs.containsValue(_ref); + } + }); assertNull("Callback not GC'd", ref.get()); assertFalse("Callback still in map", refs.containsValue(ref)); ref = null; - System.gc(); - for (int i = 0; i < 100 && (cbstruct.peer != 0 || refs.size() > 0); ++i) { - // Flush weak hash map - refs.size(); - Thread.sleep(10); // Give the GC a chance to run - System.gc(); - } + waitForGc(new Condition(cbstruct) { + public boolean evaluate(Pointer p) { + return refs.size() > 0 || p.peer != 0; + } + }); assertEquals("Callback trampoline not freed", 0, cbstruct.peer); // Next allocation should be at same place From 16975a3639cc66dd293cd93eeed0ea0e0d6d974f Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Wed, 28 Feb 2024 13:15:46 +0100 Subject: [PATCH 08/14] Added test for per-thread cleaner cleanup (failing) --- src/com/sun/jna/internal/Cleaner.java | 6 +- test/com/sun/jna/CallbacksTest.java | 2 +- test/com/sun/jna/MasterCleanerTest.java | 92 +++++++++++++++++++ test/com/sun/jna/internal/MasterAccessor.java | 14 +++ 4 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 test/com/sun/jna/MasterCleanerTest.java create mode 100644 test/com/sun/jna/internal/MasterAccessor.java diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index d9f5ad1efc..1f485f2e20 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -121,8 +121,8 @@ protected synchronized boolean remove(long n) { } } - private static class MasterCleaner extends Cleaner { - private static MasterCleaner INSTANCE; + static class MasterCleaner extends Cleaner { + static MasterCleaner INSTANCE; public static synchronized void add(Cleaner cleaner) { if (INSTANCE == null) { @@ -146,7 +146,7 @@ private static synchronized boolean deleteIfEmpty() { return false; } - private final Map cleanerImpls = new ConcurrentHashMap(); + final Map cleanerImpls = new ConcurrentHashMap(); private long lastNonEmpty = System.currentTimeMillis(); private MasterCleaner() { diff --git a/test/com/sun/jna/CallbacksTest.java b/test/com/sun/jna/CallbacksTest.java index 38f4e6832b..4780615cb4 100644 --- a/test/com/sun/jna/CallbacksTest.java +++ b/test/com/sun/jna/CallbacksTest.java @@ -88,7 +88,7 @@ public static abstract class Condition { abstract boolean evaluate(T t); } - protected static void waitForGc(Condition condition) throws Exception { + public static void waitForGc(Condition condition) throws Exception { for (int i = 0; i < 2 * Cleaner.MASTER_CLEANUP_INTERVAL_MS / 10 + 5 && condition.evaluate(); ++i) { synchronized (CallbacksTest.class) { // cleanups happen in a different thread, make sure we see it here System.gc(); diff --git a/test/com/sun/jna/MasterCleanerTest.java b/test/com/sun/jna/MasterCleanerTest.java new file mode 100644 index 0000000000..d7548be4c4 --- /dev/null +++ b/test/com/sun/jna/MasterCleanerTest.java @@ -0,0 +1,92 @@ +package com.sun.jna; + +import com.sun.jna.internal.MasterAccessor; +import junit.framework.TestCase; + +import java.util.function.Supplier; + +public class MasterCleanerTest extends TestCase { + private CallbacksTest.TestLibrary lib; + + public static boolean waitFor(Supplier cond, long maxWaitMs) { + long start = System.currentTimeMillis(); + while (System.currentTimeMillis() <= start + maxWaitMs && !cond.get()) { + try { + Thread.sleep(10); + } catch (InterruptedException ignore) {} + } + return cond.get(); + } + + @Override + protected void setUp() { + lib = Native.load("testlib", CallbacksTest.TestLibrary.class); + } + + @Override + protected void tearDown() { + lib = null; + } + + public void testGCCallbackOnFinalize() throws Exception { + final boolean[] latch = { false }; + Thread thread = new Thread(() -> { + CallbacksTest.TestLibrary.VoidCallback cb = new CallbacksTest.TestLibrary.VoidCallback() { + @Override + public void callback() { + latch[0] = true; + } + }; + synchronized (latch) { + lib.callVoidCallback(cb); + latch.notifyAll(); + try { + latch.wait(); + } catch (InterruptedException ignore) {} + } + }); + thread.start(); + + CallbackReference ref; + synchronized (latch) { + if (!latch[0]) { + latch.wait(); + } + assertTrue(latch[0]); + // Assert thread is running and has registered a callback and master is running too + assertTrue(thread.isAlive()); + assertTrue(MasterAccessor.masterIsRunning()); + assertFalse(MasterAccessor.getCleanerImpls().isEmpty()); + assertEquals(1, CallbackReference.callbackMap.size()); + ref = CallbackReference.callbackMap.values().iterator().next(); + CallbacksTest.waitForGc(new CallbacksTest.Condition(ref) { + public boolean evaluate(CallbackReference _ref) { + return _ref.get() != null || CallbackReference.callbackMap.containsValue(_ref); + } + }); + assertNotNull("Callback GC'd prematurely", ref.get()); + assertTrue("Callback no longer in map", CallbackReference.callbackMap.containsValue(ref)); + latch.notifyAll(); + } + thread.join(); + + // thread is no longer running, dummy is collectable, master is still running + final Pointer cbstruct = ref.cbstruct; + assertTrue(MasterAccessor.masterIsRunning()); + CallbacksTest.waitForGc(new CallbacksTest.Condition(ref) { + public boolean evaluate(CallbackReference _ref) { + return _ref.get() != null || CallbackReference.callbackMap.containsValue(_ref); + } + }); + assertNull("Callback not GC'd", ref.get()); + assertFalse("Callback still in map", CallbackReference.callbackMap.containsValue(ref)); + + assertTrue("CleanerImpl still exists", + waitFor(() -> MasterAccessor.getCleanerImpls().isEmpty(), 60_000)); // 1 minute + + thread = null; + // thread is collectable -> wait until master terminates + assertTrue("Master still running", + waitFor(() -> !MasterAccessor.masterIsRunning(), 60_000)); // 1 minute + } +} diff --git a/test/com/sun/jna/internal/MasterAccessor.java b/test/com/sun/jna/internal/MasterAccessor.java new file mode 100644 index 0000000000..5e52c9b9ff --- /dev/null +++ b/test/com/sun/jna/internal/MasterAccessor.java @@ -0,0 +1,14 @@ +package com.sun.jna.internal; + +import java.util.Map; + +public class MasterAccessor { + public static synchronized boolean masterIsRunning() { // synchronized is for memory synch not mutex + return Cleaner.MasterCleaner.INSTANCE != null; + } + + public static synchronized Map getCleanerImpls() { // synchronized is for memory synch not mutex + Cleaner.MasterCleaner mc = Cleaner.MasterCleaner.INSTANCE; + return mc == null ? null : mc.cleanerImpls; + } +} From 1d086fe94e3b645c2dd9afc102351297d731d194 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Wed, 19 Jun 2024 13:56:13 +0200 Subject: [PATCH 09/14] Simplification --- src/com/sun/jna/internal/Cleaner.java | 53 ++++++--------------------- 1 file changed, 12 insertions(+), 41 deletions(-) diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index 1f485f2e20..5c1c1e51ed 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -109,18 +109,6 @@ protected boolean remove(long n) { } } - private static class MasterCleanerImpl extends CleanerImpl { - @Override - protected synchronized void put(long n, CleanerRef ref) { - super.put(n, ref); - } - - @Override - protected synchronized boolean remove(long n) { - return super.remove(n); - } - } - static class MasterCleaner extends Cleaner { static MasterCleaner INSTANCE; @@ -130,20 +118,14 @@ public static synchronized void add(Cleaner cleaner) { } final CleanerImpl impl = cleaner.impl; INSTANCE.cleanerImpls.put(impl, true); - INSTANCE.register(cleaner, new Runnable() { - @Override - public void run() { - INSTANCE.cleanerImpls.put(impl, false); - } - }); + INSTANCE.register(cleaner, () -> INSTANCE.cleanerImpls.put(impl, false)); } - private static synchronized boolean deleteIfEmpty() { - if (INSTANCE != null && INSTANCE.cleanerImpls.isEmpty()) { + private static synchronized boolean deleteIfEmpty(MasterCleaner caller) { + if (INSTANCE == caller && INSTANCE.cleanerImpls.isEmpty()) { INSTANCE = null; - return true; } - return false; + return caller.cleanerImpls.isEmpty(); } final Map cleanerImpls = new ConcurrentHashMap(); @@ -151,12 +133,10 @@ private static synchronized boolean deleteIfEmpty() { private MasterCleaner() { super(true); - Thread cleanerThread = new Thread() { - @Override - public void run() { + Thread cleanerThread = new Thread(() -> { long now; long lastMasterRun = 0; - while ((now = System.currentTimeMillis()) < lastNonEmpty + MASTER_MAX_LINGER_MS || !deleteIfEmpty()) { + while ((now = System.currentTimeMillis()) < lastNonEmpty + MASTER_MAX_LINGER_MS || !deleteIfEmpty(MasterCleaner.this)) { if (!cleanerImpls.isEmpty()) { lastNonEmpty = now; } try { Reference ref = impl.referenceQueue.remove(MASTER_CLEANUP_INTERVAL_MS); @@ -178,9 +158,7 @@ public void run() { Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex); } } - } - }; - cleanerThread.setName("JNA Cleaner"); + }, "JNA Cleaner"); cleanerThread.setDaemon(true); cleanerThread.start(); } @@ -197,12 +175,7 @@ private void masterCleanup() { } } - private static final ThreadLocal MY_INSTANCE = new ThreadLocal() { - @Override - protected Cleaner initialValue() { - return new Cleaner(false); - } - }; + private static final ThreadLocal MY_INSTANCE = ThreadLocal.withInitial(() -> new Cleaner(false)); public static Cleaner getCleaner() { return MY_INSTANCE.get(); @@ -211,10 +184,8 @@ public static Cleaner getCleaner() { protected final CleanerImpl impl; private Cleaner(boolean master) { - if (master) { - impl = new MasterCleanerImpl(); - } else { - impl = new CleanerImpl(); + impl = new CleanerImpl(); + if (!master) { MasterCleaner.add(this); } } @@ -246,7 +217,7 @@ public void clean() { } } - public static interface Cleanable { - public void clean(); + public interface Cleanable { + void clean(); } } From 432739c32bff0c07c343424d1bc5c2d32b21acc4 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Wed, 19 Jun 2024 18:06:57 +0200 Subject: [PATCH 10/14] Replaced ThreadLocal-based "stealing" with our own mechanism --- src/com/sun/jna/internal/Cleaner.java | 74 +++++++++++++------ test/com/sun/jna/internal/MasterAccessor.java | 6 +- 2 files changed, 53 insertions(+), 27 deletions(-) diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index 5c1c1e51ed..c13c8ec3cb 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -27,8 +27,7 @@ import java.lang.ref.PhantomReference; import java.lang.ref.Reference; import java.lang.ref.ReferenceQueue; -import java.util.Iterator; -import java.util.Map; +import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; @@ -60,10 +59,11 @@ public class Cleaner { * cleared, the ThreadLocal instance is lost, and so are the pending * objects. * - * The Master Cleaner handles the first issue by regularly handling the - * queues of the Cleaners registered with it. - * The seconds issue is handled by registering the per-thread Cleaner - * instances with the Master's reference queue. + * The Master Cleaner handles the first issue by regularly checking the + * activity of the Cleaners registered with it, and taking over the queues + * of any cleaners appearing to be idle. + * Similarly, the second issue is handled by taking over the queues of threads + * that have terminated. */ public static final long MASTER_CLEANUP_INTERVAL_MS = 5000; @@ -116,28 +116,28 @@ public static synchronized void add(Cleaner cleaner) { if (INSTANCE == null) { INSTANCE = new MasterCleaner(); } - final CleanerImpl impl = cleaner.impl; - INSTANCE.cleanerImpls.put(impl, true); - INSTANCE.register(cleaner, () -> INSTANCE.cleanerImpls.put(impl, false)); + INSTANCE.cleaners.add(cleaner); } + /** @return true if the caller thread can terminate */ private static synchronized boolean deleteIfEmpty(MasterCleaner caller) { - if (INSTANCE == caller && INSTANCE.cleanerImpls.isEmpty()) { + if (INSTANCE == caller && INSTANCE.cleaners.isEmpty()) { INSTANCE = null; } - return caller.cleanerImpls.isEmpty(); + return caller.cleaners.isEmpty(); } - final Map cleanerImpls = new ConcurrentHashMap(); - private long lastNonEmpty = System.currentTimeMillis(); + final Set cleaners = Collections.synchronizedSet(new HashSet<>()); + final Set referencedCleaners = new HashSet<>(); + final Set watchedCleaners = new HashSet<>(); private MasterCleaner() { - super(true); Thread cleanerThread = new Thread(() -> { + long lastNonEmpty = System.currentTimeMillis(); long now; long lastMasterRun = 0; while ((now = System.currentTimeMillis()) < lastNonEmpty + MASTER_MAX_LINGER_MS || !deleteIfEmpty(MasterCleaner.this)) { - if (!cleanerImpls.isEmpty()) { lastNonEmpty = now; } + if (!cleaners.isEmpty()) { lastNonEmpty = now; } try { Reference ref = impl.referenceQueue.remove(MASTER_CLEANUP_INTERVAL_MS); if(ref instanceof CleanerRef) { @@ -164,33 +164,59 @@ private MasterCleaner() { } private void masterCleanup() { - Iterator> it = cleanerImpls.entrySet().iterator(); - while (it.hasNext()) { - Map.Entry entry = it.next(); - entry.getKey().cleanQueue(); - if (!entry.getValue() && entry.getKey().cleanables.isEmpty()) { + for (Iterator> it = Cleaner.INSTANCES.entrySet().iterator(); it.hasNext(); ) { + Map.Entry entry = it.next(); + if (!cleaners.contains(entry.getValue())) { continue; } + Cleaner cleaner = entry.getValue(); + long currentCount = cleaner.counter.get(); + if (currentCount == cleaner.lastCount // no new cleanables registered since last master cleanup interval -> assume it is no longer in use + || !entry.getKey().isAlive()) { // owning thread died -> assume it is no longer in use it.remove(); + CleanerImpl impl = cleaner.impl; + referencedCleaners.add(impl); + watchedCleaners.add(impl); + register(cleaner, () -> referencedCleaners.remove(impl)); + cleaners.remove(cleaner); + } else { + cleaner.lastCount = currentCount; + } + } + + for (Iterator it = watchedCleaners.iterator(); it.hasNext(); ) { + CleanerImpl impl = it.next(); + impl.cleanQueue(); + if (!referencedCleaners.contains(impl)) { + if (impl.cleanables.isEmpty()) { it.remove(); } } } } } - private static final ThreadLocal MY_INSTANCE = ThreadLocal.withInitial(() -> new Cleaner(false)); + private static final Map INSTANCES = new ConcurrentHashMap<>(); public static Cleaner getCleaner() { - return MY_INSTANCE.get(); + return INSTANCES.computeIfAbsent(Thread.currentThread(), Cleaner::new); } protected final CleanerImpl impl; + protected final Thread owner; + protected final AtomicLong counter = new AtomicLong(Long.MIN_VALUE); + protected long lastCount; // used by MasterCleaner only + + private Cleaner() { + this(null); + } - private Cleaner(boolean master) { + private Cleaner(Thread owner) { impl = new CleanerImpl(); - if (!master) { + this.owner = owner; + if (owner != null) { MasterCleaner.add(this); } } public Cleanable register(Object obj, Runnable cleanupTask) { + counter.incrementAndGet(); return impl.register(obj, cleanupTask); } diff --git a/test/com/sun/jna/internal/MasterAccessor.java b/test/com/sun/jna/internal/MasterAccessor.java index 5e52c9b9ff..ed5e89e8c3 100644 --- a/test/com/sun/jna/internal/MasterAccessor.java +++ b/test/com/sun/jna/internal/MasterAccessor.java @@ -1,14 +1,14 @@ package com.sun.jna.internal; -import java.util.Map; +import java.util.Set; public class MasterAccessor { public static synchronized boolean masterIsRunning() { // synchronized is for memory synch not mutex return Cleaner.MasterCleaner.INSTANCE != null; } - public static synchronized Map getCleanerImpls() { // synchronized is for memory synch not mutex + public static synchronized Set getCleanerImpls() { // synchronized is for memory synch not mutex Cleaner.MasterCleaner mc = Cleaner.MasterCleaner.INSTANCE; - return mc == null ? null : mc.cleanerImpls; + return mc == null ? null : mc.cleaners; } } From f03fed9bf4629ca5de96456263c5959b266f6f9c Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Wed, 19 Jun 2024 18:52:55 +0200 Subject: [PATCH 11/14] Added logging --- src/com/sun/jna/internal/Cleaner.java | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index c13c8ec3cb..16865b698b 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -42,6 +42,8 @@ *

This class is intended to be used only be JNA itself.

*/ public class Cleaner { + private static final Logger LOG = Logger.getLogger(Cleaner.class.getName()); + /* General idea: * * There's one Cleaner per thread, kept in a ThreadLocal static variable. @@ -158,7 +160,9 @@ private MasterCleaner() { Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex); } } + LOG.log(Level.FINE, "MasterCleaner thread {0} exiting", Thread.currentThread()); }, "JNA Cleaner"); + LOG.log(Level.FINE, "Starting new MasterCleaner thread {0}", cleanerThread); cleanerThread.setDaemon(true); cleanerThread.start(); } @@ -173,9 +177,13 @@ private void masterCleanup() { || !entry.getKey().isAlive()) { // owning thread died -> assume it is no longer in use it.remove(); CleanerImpl impl = cleaner.impl; + LOG.log(Level.FINE, () -> "MasterCleaner stealing cleaner " + impl + " from thread " + entry.getKey()); referencedCleaners.add(impl); watchedCleaners.add(impl); - register(cleaner, () -> referencedCleaners.remove(impl)); + register(cleaner, () -> { + referencedCleaners.remove(impl); + LOG.log(Level.FINE, "Cleaner {0} no longer referenced", impl); + }); cleaners.remove(cleaner); } else { cleaner.lastCount = currentCount; @@ -186,7 +194,10 @@ private void masterCleanup() { CleanerImpl impl = it.next(); impl.cleanQueue(); if (!referencedCleaners.contains(impl)) { - if (impl.cleanables.isEmpty()) { it.remove(); } + if (impl.cleanables.isEmpty()) { + it.remove(); + LOG.log(Level.FINE, "Discarding empty Cleaner {0}", impl); + } } } } @@ -213,6 +224,8 @@ private Cleaner(Thread owner) { if (owner != null) { MasterCleaner.add(this); } + LOG.log(Level.FINE, () -> owner == null ? "Created new MasterCleaner" + : "Created new Cleaner " + impl + " for thread " + owner); } public Cleanable register(Object obj, Runnable cleanupTask) { @@ -229,6 +242,7 @@ private static class CleanerRef extends PhantomReference implements Clea public CleanerRef(CleanerImpl impl, Object referent, ReferenceQueue q, Runnable cleanupTask) { super(referent, q); + LOG.log(Level.FINER, () -> "Registering " + referent + " with " + impl + " as " + this); this.cleaner = impl; this.cleanupTask = cleanupTask; cleaner.put(number, this); @@ -237,6 +251,7 @@ public CleanerRef(CleanerImpl impl, Object referent, ReferenceQueue q, R @Override public void clean() { if(cleaner.remove(this.number) && cleanupTask != null) { + LOG.log(Level.FINER, "Cleaning up {0}", this); cleanupTask.run(); cleanupTask = null; } From 3ee27258f9702513e815226338b01b5c745e79f9 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Wed, 3 Jul 2024 11:24:04 +0200 Subject: [PATCH 12/14] Added missing license header --- test/com/sun/jna/MasterCleanerTest.java | 23 +++++++++++++++++++ test/com/sun/jna/internal/MasterAccessor.java | 23 +++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/test/com/sun/jna/MasterCleanerTest.java b/test/com/sun/jna/MasterCleanerTest.java index d7548be4c4..a64b5183ea 100644 --- a/test/com/sun/jna/MasterCleanerTest.java +++ b/test/com/sun/jna/MasterCleanerTest.java @@ -1,3 +1,26 @@ +/* Copyright (c) 2024 Peter Conrad, All Rights Reserved + * + * The contents of this file is dual-licensed under 2 + * alternative Open Source/Free licenses: LGPL 2.1 or later and + * Apache License 2.0. (starting with JNA version 4.0.0). + * + * You can freely decide which license you want to apply to + * the project. + * + * You may obtain a copy of the LGPL License at: + * + * http://www.gnu.org/licenses/licenses.html + * + * A copy is also included in the downloadable source code package + * containing JNA, in file "LGPL2.1". + * + * You may obtain a copy of the Apache License at: + * + * http://www.apache.org/licenses/ + * + * A copy is also included in the downloadable source code package + * containing JNA, in file "AL2.0". + */ package com.sun.jna; import com.sun.jna.internal.MasterAccessor; diff --git a/test/com/sun/jna/internal/MasterAccessor.java b/test/com/sun/jna/internal/MasterAccessor.java index ed5e89e8c3..331a72157d 100644 --- a/test/com/sun/jna/internal/MasterAccessor.java +++ b/test/com/sun/jna/internal/MasterAccessor.java @@ -1,3 +1,26 @@ +/* Copyright (c) 2024 Peter Conrad, All Rights Reserved + * + * The contents of this file is dual-licensed under 2 + * alternative Open Source/Free licenses: LGPL 2.1 or later and + * Apache License 2.0. (starting with JNA version 4.0.0). + * + * You can freely decide which license you want to apply to + * the project. + * + * You may obtain a copy of the LGPL License at: + * + * http://www.gnu.org/licenses/licenses.html + * + * A copy is also included in the downloadable source code package + * containing JNA, in file "LGPL2.1". + * + * You may obtain a copy of the Apache License at: + * + * http://www.apache.org/licenses/ + * + * A copy is also included in the downloadable source code package + * containing JNA, in file "AL2.0". + */ package com.sun.jna.internal; import java.util.Set; From 094aea6b19cd03efb78c3550e7a448ca8d8ead28 Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Wed, 3 Jul 2024 11:27:38 +0200 Subject: [PATCH 13/14] Make checkstyle happy --- src/com/sun/jna/internal/Cleaner.java | 50 +++++++++++++-------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index 16865b698b..a164e9b5b3 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -135,33 +135,33 @@ private static synchronized boolean deleteIfEmpty(MasterCleaner caller) { private MasterCleaner() { Thread cleanerThread = new Thread(() -> { - long lastNonEmpty = System.currentTimeMillis(); - long now; - long lastMasterRun = 0; - while ((now = System.currentTimeMillis()) < lastNonEmpty + MASTER_MAX_LINGER_MS || !deleteIfEmpty(MasterCleaner.this)) { - if (!cleaners.isEmpty()) { lastNonEmpty = now; } - try { - Reference ref = impl.referenceQueue.remove(MASTER_CLEANUP_INTERVAL_MS); - if(ref instanceof CleanerRef) { - ((CleanerRef) ref).clean(); - } - // "now" is not really *now* at this point, but off by no more than MASTER_CLEANUP_INTERVAL_MS - if (lastMasterRun + MASTER_CLEANUP_INTERVAL_MS <= now) { - masterCleanup(); - lastMasterRun = now; - } - } catch (InterruptedException ex) { - // Can be raised on shutdown. If anyone else messes with - // our reference queue, well, there is no way to separate - // the two cases. - // https://groups.google.com/g/jna-users/c/j0fw96PlOpM/m/vbwNIb2pBQAJ - break; - } catch (Exception ex) { - Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex); + long lastNonEmpty = System.currentTimeMillis(); + long now; + long lastMasterRun = 0; + while ((now = System.currentTimeMillis()) < lastNonEmpty + MASTER_MAX_LINGER_MS || !deleteIfEmpty(MasterCleaner.this)) { + if (!cleaners.isEmpty()) { lastNonEmpty = now; } + try { + Reference ref = impl.referenceQueue.remove(MASTER_CLEANUP_INTERVAL_MS); + if(ref instanceof CleanerRef) { + ((CleanerRef) ref).clean(); + } + // "now" is not really *now* at this point, but off by no more than MASTER_CLEANUP_INTERVAL_MS + if (lastMasterRun + MASTER_CLEANUP_INTERVAL_MS <= now) { + masterCleanup(); + lastMasterRun = now; } + } catch (InterruptedException ex) { + // Can be raised on shutdown. If anyone else messes with + // our reference queue, well, there is no way to separate + // the two cases. + // https://groups.google.com/g/jna-users/c/j0fw96PlOpM/m/vbwNIb2pBQAJ + break; + } catch (Exception ex) { + Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex); } - LOG.log(Level.FINE, "MasterCleaner thread {0} exiting", Thread.currentThread()); - }, "JNA Cleaner"); + } + LOG.log(Level.FINE, "MasterCleaner thread {0} exiting", Thread.currentThread()); + }, "JNA Cleaner"); LOG.log(Level.FINE, "Starting new MasterCleaner thread {0}", cleanerThread); cleanerThread.setDaemon(true); cleanerThread.start(); From 21d46abe2cbfdff3018606ad072ed18ada4ba13c Mon Sep 17 00:00:00 2001 From: Peter Conrad Date: Wed, 3 Jul 2024 11:52:15 +0200 Subject: [PATCH 14/14] Added explanation of Cleaner lifecycle --- src/com/sun/jna/internal/Cleaner.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index a164e9b5b3..8213915887 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -129,6 +129,17 @@ private static synchronized boolean deleteIfEmpty(MasterCleaner caller) { return caller.cleaners.isEmpty(); } + /* The lifecycle of a Cleaner instance consists of four phases: + * 1. New instances are contained in Cleaner.INSTANCES and added to a MasterCleaner.cleaners set. + * 2. At some point, the master cleaner takes control of the instance by removing it + * from Cleaner.INSTANCES and MasterCleaner.cleaners, and then adding it to its + * referencedCleaners and watchedCleaners sets. Note that while it is no longer + * in Cleaner.INSTANCES, a thread may still be holding a reference to it. + * 3. Possibly some time later, the last reference to the cleaner instance is dropped and + * it is GC'd. It is then also removed from referencedCleaners but remains in watchedCleaners. + * 4. The master cleaner continues to monitor the watchedCleaners until they are empty and no + * longer referenced. At that point they are also removed from watchedCleaners. + */ final Set cleaners = Collections.synchronizedSet(new HashSet<>()); final Set referencedCleaners = new HashSet<>(); final Set watchedCleaners = new HashSet<>();