RFR: 8175010: ImageReader is not thread-safe

[Alan Bateman]

On 16/02/2017 08:08, David Holmes wrote:

>
> 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?
There are essentially two usages:

1. At run-time then the jimage file is memory mapped and that mapping is 
shared between the VM and libraries. There should be one ImageReader and 
it should never be closed. If it accidentally closed then it leads to 
spurious NCDFE and a painful death.

The patch that Claes pushed to move to computeIfAbsent ensures that we 
only create the ImageReader once. It's possible this will mask the issue 
that is lurking  we don't know yet.


2. jrtfs, used by javac and other tools. It's not in current picture but 
with jrtfs then jimage files will be opened + closed. The close method 
that you pasted in clearly doesn't handle async close but it requires 
going into the reader implementations to see if there are issues or not.

-Alan