RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use > alternative to finalization (Roger Riggs)

[Peter Levart]

On 10/02/2015 10:16 PM, Roger Riggs wrote:
> Hi Mike,
>
> Thanks for the review and comments...
>
> On 10/2/2015 3:33 PM, Mike Duigou wrote:
>> Hi Roger;
>>
>> This looks like an interesting proposal and would be a good 
>> replacement for alternative solutions.
>>
>> I am curious why the thread is run at max priority. Also, why not set 
>> priority and daemon status in the factory?
> Yes, that should probably be left to the factory and for built-in 
> factory would be moved to the InnocuousThreadFactory.
>>
>> You should clear the Thread intrerruption if you are going to ignore 
>> it. Thread.interrupted();
> Will do.
>>
>> I would suggest surrounding the thunk.run() with a catch of Throwable 
>> or Exception. In the current implementation one bad egg spoils the 
>> party for everyone. There is a catch of RuntimeException described as 
>> "catch (RuntimeException e) { // ignore exceptions from the cleanup 
>> thunk }" but I would suggest that this is insufficiently paranoid.
> Good point,  catches more bad eggs.
>>
>> Consider a pattern like:
>>
>> private static final ReferenceQueue<Object> weakListeners = new 
>> ReferenceQueue<>();
>>
>>
>> private static class CleanerThread extends Thread {
>> { setDaemon(true); setPriority(Thread.MIN_PRIORITY); }
>>     @Override
>>     @SuppressWarnings("unchecked")
>>     public void run() {
>>     // Using weak ref to queue allows shutdown if class is unloaded
>>     WeakReference<ReferenceQueue<Object>> weakQueue = new 
>> WeakReference<>(weakListeners);
>>     ReferenceQueue<Object> queue = null;
>>     while((queue = (queue == null ? weakQueue.get() : queue) != null) 
>> try {
>>       Object dead = queue.remove(10000);
>>       if(dead != null) {
>>          // do stuff with "dead"
>>       } else {
>>         queue = null; // recheck to see if queue is gone.
>>       }
>>     } catch(InterruptedException woken) {
>>       Thread.interrupted();
>>     }
>>   }
>> }
>>
>> When 'weakListeners' is cleared when the containing class is unloaded 
>> then the thread will shut down after roughly 10 seconds.
>>
>> I have been meaning to check whether CleanerThread needs to be in a 
>> separate class--an inner class, even a static one, may prevent it's 
>> containing class from being GCed. IDK.
> I think that 'nested' classes aka  static class has no links to the 
> outer class and can be GC'ed but will check.
>
> Thanks, Roger

Hi Mike, Roger,

Nested classes are typically loaded by the same ClassLoader as their 
outer classes. Normal named classes (as opposed to VM-annonymous 
classes) can't be GC-ed individually. Their defining ClassLoader keeps 
them referenced in it's ClassLoader#classes Vector. Each class has an 
implicit reference to it's defining ClassLoader, therefore each class 
indirectly references every class defined by the same ClassLoader as 
itself, and by delegation of ClassLoaders also every class defined by 
predecessor ClassLoaders. Only when all classes defined by a particular 
ClassLoader become unreachable and the ClassLoader too, do they become 
eligible for GC at the same time.

In Roger's API, the class loader of those classes is bootstrap 
classloader which never goes away, so any objects referenced by static 
fields in bootstrap classes are never released.

Above code has a flaw even if the weakListeners ReferenceQueue is passed 
to the CleanerThread via constructor parameter and immediately wrapped 
by WeakReference, like:

private static class CleanerThread extends Thread {

     WeakReference<ReferenceQueue<Object>> weakQueue;

     CleanerThread(ReferenceQueue<Object> weakListeners) {
         setDaemon(true); setPriority(Thread.MIN_PRIORITY);
         weakQueue = new WeakReference<>(weakListeners);
     }

     public void run() {
         ReferenceQueue<Object> queue = null;
         while((queue = (queue == null ? weakQueue.get() : queue) != 
null) try {
             Object dead = queue.remove(10000);
             if(dead != null) {
                  // do stuff with "dead"
             } else {
                 queue = null; // recheck to see if queue is gone.
             }
         } catch(InterruptedException woken) {
             Thread.interrupted();
         }
     }
}


There is a big chance that this loop will run forever even after 
'weakListeners' is not referenced from other parts of code any more. The 
problem is the queue.remove(10000) call which spends 10 seconds waiting 
for a Reference to be enqueued, which never happens, but keeps the 
'queue' alive all that time. There's only a brief window of time between 
'queue = null' assignment and 'weakQueue.get()' which re-obtains a 
strong reference to the queue and happens periodically every 10 
seconds.If GC never runs in this small window of opportunity, the 
weakQueue will never be cleared and the thread will run forever.

Regards, Peter