Shutdown CleanerThread once the last cleanable is removed #1555
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,35 +45,11 @@ public static Cleaner getCleaner() { | |
| } | ||
|
|
||
| private final ReferenceQueue<Object> referenceQueue; | ||
| private Thread cleanerThread; | ||
| private CleanerRef firstCleanable; | ||
|
|
||
| private Cleaner() { | ||
| referenceQueue = new ReferenceQueue<Object>(); | ||
| } | ||
|
|
||
| public synchronized Cleanable register(Object obj, Runnable cleanupTask) { | ||
|
|
@@ -83,34 +59,43 @@ public synchronized Cleanable register(Object obj, Runnable 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; | ||
| } | ||
| } | ||
|
|
||
| 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 static class CleanerRef extends PhantomReference<Object> implements Cleanable { | ||
|
|
@@ -125,6 +110,7 @@ public CleanerRef(Cleaner cleaner, Object referent, ReferenceQueue<? super Objec | |
| this.cleanupTask = cleanupTask; | ||
| } | ||
|
|
||
| @Override | ||
| public void clean() { | ||
| if(cleaner.remove(this)) { | ||
| cleanupTask.run(); | ||
|
|
@@ -151,4 +137,52 @@ void setNext(CleanerRef next) { | |
| public static interface Cleanable { | ||
| public void clean(); | ||
| } | ||
|
|
||
| private class CleanerThread extends Thread { | ||
|
|
||
| private static final long CLEANER_LINGER_TIME = 30000; | ||
|
|
||
| public CleanerThread() { | ||
| super("JNA Cleaner"); | ||
| setDaemon(true); | ||
| } | ||
|
|
||
| @Override | ||
| public void run() { | ||
| while (true) { | ||
| try { | ||
| Reference<? extends Object> 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; | ||
|
There was a problem hiding this comment. I'm wondering if using an |
||
| 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); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be
volatileto ensure it works correctly? Or is the presence ofstart()sufficient to ensure this is not a problem?See any discussion of double checked locking for the relevant technical concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think
cleanerThreadneeds to be volatile. I tried to get through the JLS regarding synchronization, but some google foo led me to this summary:https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/package-summary.html#MemoryVisibility
In this case the critical monitor is the one associated with
referenceQueue. Both places where thecleanerThreadis modified are protected by a synchronization onreferenceQueueand so to my reading of the happens-before guarantee for synchronized blocks, there can be no phantom-reads fromcleanerThread. The same is true forfirstCleanable, which needs to be considered also.Usage of the
referenceQueuemonitor is sane from my POV, because there are two different variables affected (cleanerThreadandfirstCleanable) and multiple statement that must be executed atomically, which excludes the usage of a volatile variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I did enough rubber-ducking to convince myself the use of
referenceQueuewas rather effective.My main concern was that the initial assignment of
cleanerThreadto an object happens-before, but there is no guarantee that the remainder of object initialization happens.... except in this case it must complete (within the block) in order for thestart()method to be valid.