Looks good now. Serialization logic is a little confusing but seems OK. On Aug 11, 2010, at 4:53 PM, Mandy Chung wrote:
> 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 >
