RFC 7038914: VM could throw uncaught OOME in ReferenceHandler thread

[Peter Levart]

On 05/09/2013 07:53 AM, David Holmes wrote:
> Hi Thomas,
>
> On 9/05/2013 1:28 AM, Thomas Schatzl wrote:
>> Hi,
>>
>>    please review the latest webrev for this patch that is available at
>>
>> http://cr.openjdk.java.net/~tschatzl/7038914/webrev.2/
>>
>> As mentioned, it incorporates the fix and reproducer from Peter Levart.
>
> Fix is fine.
>
> I'm not sure about the test (sorry I didn't pay it attention earlier). 
> Have you instrumented the code to verify that the test actually 
> triggers an OOME? It may be possible that if the OOME did kill the 
> reference thread that we wouldn't necessarily detect it using the 
> sleeps etc. Also I don't understand the need for the actual weakRef 
> usage and System.gc() etc. If the heap is full then the interrupt will 
> wake the reference handler thread and the allocation will trigger the 
> OOME. It doesn't matter if there are any references to process. The 
> main logic might then reduce to:
>
>  referenceHandlerThread.interrupt();
>  for (int i = 0; i < 10; i++) {
>    if (!referenceHandlerThread.isAlive())
>        throw new Exception("Reference-handler thread died");
>    Thread.sleep(1000);
>  }
>
> which just gives it 10s to die else assumes all ok. ?
>
> But I can live with existing test (it just might vacuously pass)
>
> Cheers,
> David

Hi David, Thomas,

Yes, this was just a left-over from my bug reproducer that demonstrated 
the situation where WeakReference was cleared, but nothing was enqueue-d 
into the ReferenceQueue. Reference Handler thread otherwise just sleeps 
in lock.wait() most of the time if there's nothing to do and 
interrupting it's thread will trigger allocation of InterruptedException 
and consequently OOME nevertheless.

One think to watch: Reference Handler thread is started in the 
j.l.r.Reference static initializer, so the Reference class must be 
initialized. I don't know if there is any guarantee that this is done 
before entering main(). So there should be something explicit in main() 
method that ensures it.

Also, throwing test-fail exception should be done after releasing the 
heap or we'll just trigger another OOME without any message to describe 
the situation.

Waiting in a loop for ReferenceHandler thread is not needed since the 
test will normally succeed and so the whole loop will execute to the 
end. Only when the test fails the loop will exit faster. In my 
experience, the thread dies between 10-20ms after interrupting, so 
waiting for about 500ms is enough I think. Also no System.gc() and 
additional waiting is needed to report the outcome - just releasing the 
reference to 'waste' variable is enuugh in my experience to restore the 
heap into working condition.

Here's the updated test that does all that:


public class OOMEInReferenceHandler {
     static Object[] fillHeap() {
         Object[] first = null, last = null;
         int size = 1 << 20;
         while (size > 0) {
             try {
                 Object[] array = new Object[size];
                 if (first == null) {
                     first = array;
                 }
                 else {
                     last[0] = array;
                 }
                 last = array;
             }
             catch (OutOfMemoryError oome) {
                 size = size >>> 1;
             }
         }
         return first;
     }

     public static void main(String[] args) throws Exception {
         // ensure ReferenceHandler thread is started
         new WeakReference<Object>(new Object());

         ThreadGroup tg = Thread.currentThread().getThreadGroup();
         for (ThreadGroup tgn = tg;
              tgn != null;
              tg = tgn, tgn = tg.getParent());

         Thread[] threads = new Thread[tg.activeCount()];
         Thread referenceHandlerThread = null;
         int n = tg.enumerate(threads);
         for (int i = 0; i < n; i++) {
             if("Reference Handler".equals(threads[i].getName())) {
                 referenceHandlerThread = threads[i];
             }
         }

         if (referenceHandlerThread == null) {
             throw new IllegalStateException("Couldn't find Reference 
Handler thread.");
         }

         Object waste = fillHeap();

         referenceHandlerThread.interrupt();

         Thread.sleep(500L);

         // release waste so we can allocate and throw normal exceptions
         waste = null;

          // check that the handler is still alive
         if (!referenceHandlerThread.isAlive()) {
             throw new Exception("Reference Handler thread died.");
         }
     }
}



Regards, Peter

>
>> Bugs.sun.com:
>> http://bugs.sun.com/view_bug.do?bug_id=7038914
>>
>> JIRA:
>> https://jbs.oracle.com/bugs/browse/JDK-7038914
>>
>> Testing:
>> JPRT, with the new reproducer passing
>>
>> In need of reviewers and a sponsor for this patch; as mentioned earlier
>> I will set Peter (plevart) as author for this patch, i.e. send a
>> patchset to the sponsor for this change with that author set.
>>
>> Thanks,
>> Thomas
>>