RFR: 8175010: ImageReader is not thread-safe

[Claes Redestad]

Hi David,

On 2017-02-16 09:08, David Holmes wrote:
> 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?

reader.close(this) calls will synchronize on 
SharedImageReader.OPEN_FILES, so concurrent calls on the same 
ImageReader should be OK.  We should add a comment to point
this out.

/Claes

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