Hi Peter,
are you suggesting that if I have class Foo { Bar b; }, then creating
and putting Foo b in a CHM before returning it to a consumer which is
then read from another thread is enough to force b to be safely
published even when the other thread does *not* get the object via a
call to the same CHM (which would go via the same volatile read and add
the necessary happens before relationship)? I recalled having seen
examples to the effect that a non-volatile b isn't safely published in
this case.
The (very shaky) hypothesis is thus that this could be what's happening
in any of the places which load and locally cache the system ImageReader
for anyone to use, e.g., SystemModuleFinder.SystemImage or
JavaRuntimeURLConnection (which is implicitly called when there's a
security manager installed). I might be (in fact likely am) wrong, but
we discussed this offline and came to the conclusion that there was no
harm in implementing these improvements regardless of whether they
actually resolve 8174817 or not.
I think prior to this patch a concurrent ImageReader.close() could
happen if there was a race between 3 or more threads to resolve the
same Path from ImageReaderFactory.get (especially since there might be
a longish time window there since we might block to load a library
etc), so I don't think we lose anything from plugging that hole by
using computeIfAbsent here.
I think your observations about potential issues in JRTFS is correct,
but there was nothing to suggest JRTFS code was involved in JDK-8174817
(as it's not code that's used by the BuiltinClassLoader).
Thanks!
/Claes
On 2017-02-15 21:52, Peter Levart wrote:
Hi Claes,
Reading the https://bugs.openjdk.java.net/browse/JDK-8174817 and then
the change that was just pushed, I can't seem to figure out what was the
problem with original code. I can't find evidence for claims in
https://bugs.openjdk.java.net/browse/JDK-8175010 . Is the problem
publication of ImageReader via ImageReaderFactory? That can't be it,
since ImageReaderFactory is using ConcurrentHashMap which ensures
happens before relationships.
Is there any place else where ImageReader.open(Path) is called and then
the instance unsafely published to other threads? The only place I could
find is in jdk.internal.jrtfs.SystemImage.open():
static SystemImage open() throws IOException {
if (modulesImageExists) {
// open a .jimage and build directory structure
final ImageReader image = ImageReader.open(moduleImageFile);
image.getRootDirectory();
return new SystemImage() {
@Override
Node findNode(String path) throws IOException {
return image.findNode(path);
}
@Override
byte[] getResource(Node node) throws IOException {
return image.getResource(node);
}
@Override
void close() throws IOException {
image.close();
}
};
}
...here the final 'image' local variable is captured by anonymous inner
SystemImage subclass into a synthetic final field, so this final field
ensures that ImageReader is published safely as a delegate of SystemImage.
ImageReader.open(Path) factory method delegates to
ImageReader.SharedImageReader.open(Path, ByteOrder) which creates a new
instance of ImageReader encapsulating a SharedImageReader object:
public static ImageReader open(Path imagePath, ByteOrder
byteOrder) throws IOException {
Objects.requireNonNull(imagePath);
Objects.requireNonNull(byteOrder);
synchronized (OPEN_FILES) {
SharedImageReader reader = OPEN_FILES.get(imagePath);
if (reader == null) {
// Will fail with an IOException if wrong byteOrder.
reader = new SharedImageReader(imagePath, byteOrder);
OPEN_FILES.put(imagePath, reader);
} else if (reader.getByteOrder() != byteOrder) {
throw new IOException("\"" + reader.getName() + "\"
is not an image file");
}
ImageReader image = new ImageReader(reader); // <<-- HERE
reader.openers.add(image);
return image;
}
}
...the ImageReader returned from this method is not safe to publish via
data race, but as far as I can see, there is no such publishing going
on. So am I right in that all this patch solves is it eliminates a
possibility of NPE when ImageReader is close()-d concurrently with being
used from some other thread. If such NPE was observed, it means that
close() is being called concurrently with access and there are still
races possible which can cause undesired effects. For example: calling
ImageReader.close() while using it concurrently may close underlying
SharedImageReader and then after closing, access it.
So is there a concurrent ImageReader.close() possible? The only place I
see ImageReader.close() being invoked is from SystemImage.close() in the
anonymous inner class implementation which wraps ImageReader.
SystemImage.close() is only being invoked from JrtFileSystem.cleanup(),
which is called from JrtFileSystem.close() and JrtFileSystem.finalize().
The following is theoretically possible:
FileSystem fs = FileSystems.newFileSystem(URI.create("jrt:/"), ...);
Path p = fs.getPath(...);
FileSystemProvider provider = fs.provider();
InputStream is = provider.newInputStream(p, ...);
// 'fs' and 'p' (which has a reference to 'fs') may be found finalizable
and be finalized while the call to obtain content of input stream is in
progress
For this to be prevented, the following method in JrtFileSystem:
// returns the content of the file resource specified by the path
byte[] getFileContent(JrtPath path) throws IOException {
Node node = checkNode(path);
if (node.isDirectory()) {
throw new FileSystemException(path + " is a directory");
}
//assert node.isResource() : "resource node expected here";
return image.getResource(node);
}
...would have to be changed to:
byte[] getFileContent(JrtPath path) throws IOException {
Node node = checkNode(path);
if (node.isDirectory()) {
throw new FileSystemException(path + " is a directory");
}
//assert node.isResource() : "resource node expected here";
byte[] content = image.getResource(node);
Reference.reachabilityFence(this);
return content;
}
I don't claim that this is what causes the problems and I haven't found
any such usages, but this is theoretically possible.
Regards, Peter
On 02/15/2017 02: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:
- 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
