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
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
