RFC 7038914: VM could throw uncaught OOME in ReferenceHandler thread

[David Holmes]

Hi Peter,

Thanks for clarifying the test details. A few follow ups:

- The reference class does get initialized early in the VM startup well 
before Main will be loaded. But the use of the weakref doesn't hurt.

- 500ms may not be enough on some platforms, particularly some embedded 
systems. Ideally we could code up a handshake using another weakref that 
would guarantee that the handler thread really survived past the OOM. 
But at some point this just becomes a no-reg-hard situation :)

- I'm not certain that setting waste=null is sufficient to guarantee the 
next allocation will succeed under all GC's. GC folk can confim/deny that.

Thanks,
David

On 9/05/2013 6:35 PM, Peter Levart wrote:
> 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
>>>
>