On 08/11/10 12:35, Brian Goetz wrote:
This code has thread-safety issues. There is no happens-before between the
writes to suppressedExceptions (and its referenced state) and the places where
it is iterated (e.g., line 576).
You could plug this hole by replacing the idiom at line 576 (and similar) with
for (Throwable se : getSuppressedExceptions()).
Good point. I fixed getSuppressedExceptions() be a synchronized method
but missed the unsynchronized reads.
This would be needed for all unsynchronized reads of suppressedException. You
could get away without it at the null check if you really were trying to
squeeze some performance out and you didn't mind occasionally missing writes
from other threads.
The updated webrev is at:
http://cr.openjdk.java.net/~mchung/6973831/webrev.01/
I leave the unsynchronized read in the readObject() method as the
Throwable object is still being deserialized and the
suppressedExceptions is going to be replaced with a new list.
Thanks
Mandy
On Aug 11, 2010, at 3:09 PM, Mandy Chung wrote:
Fix for 6973831: NPE when printing stack trace of OOME
Webrev at:
http://cr.openjdk.java.net/~mchung/6973831/webrev.00/
The VM preallocates OutOfMemoryError objects to improve OOM diagnosability. The
constructor is not invoked since the OOME object allocation is done at VM
initialization time and thus the new suppressedExceptions field is null which
causes NPE when Throwable.printStackTrace() method is called on the VM
preallocated OOM object.
The fix is to initialize the suppressedExceptions field to null instead of
Collections.emptyList(). An alternative could be to initialize the
suppressedException field in the VM after VM initialization is finished. It
doesn't handle the case when any new field initialized to be non-null is added
in the OOM object in the future and it would require a VM synchronized change.
The libraries fix is simpler and preferred.
Thanks
Mandy
