RFR: 8175010: ImageReader is not thread-safe

[David Holmes]

Hi Claes,

On 15/02/2017 11:22 PM, Claes Redestad wrote:
> Hi,
>
> a few intermittent but rare test failures[1] that has appeared
> since the latest jake integration, and since one of the changes
> in there was to make initialization of the system ImageReader
> lazy there appears to be cases where ImageReaders are not
> safely published:

I find it very hard to discern exactly how these classes are intended to 
be used concurrently. Is there some lifecycle description of the 
ImageReader and how it is intended to be used? Without any 
synchronization I still see lots of not-thread-safe code. Simple example:

   75     public void close() throws IOException {
   76         if (closed) {
   77             throw new IOException("image file already closed");
   78         }
   79         reader.close(this);
   80         closed = true;
   81     }

two threads can still both call reader.close(this) concurrently. And if 
the new closed volatile boolean solves something then wouldn't making 
the reader variable volatile achieve the same JMM affects?

David
-----

> - Ensure ImageReader::open is called only once per Path in
> ImageReaderFactory by using CHM.computeIfAbsent
> - Ensure ImageReader.reader is safely published to a
> final field and signal close using a volatile boolean instead
>
> webrev: http://cr.openjdk.java.net/~redestad/8175010/webrev.02/
> bug: https://bugs.openjdk.java.net/browse/JDK-8175010
>
> Testing shows no issues (which admittedly doesn't mean we're
> actually solving the root cause for JDK-8174817), and performance
> numbers from adding a volatile read indicate that any overhead
> is lost in the noise on ImageReader-heavy workloads.
>
> Thanks!
>
> /Claes
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8174817