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
