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.


Reply via email to