Hi Naoto,
Sorry for joining late for review. Thanks for taking this on. I think it
is shaping well...
On 01/14/2017 01:54 AM, Naoto Sato wrote:
http://cr.openjdk.java.net/~naoto/8171139/webrev.05/
Tho things...
1. As said, the "cloning" of CacheKey has no purpose in the following
section of code (lines 1685...1709):
CacheKey constKey = new CacheKey(cacheKey);
trace("findBundle: %d %s %s formats: %s%n", index,
candidateLocales, cacheKey, formats);
try {
if (module.isNamed()) {
bundle = loadBundle(cacheKey, formats, control,
module, callerModule);
} else {
bundle = loadBundle(cacheKey, formats, control,
expiredBundle);
}
if (bundle != null) {
if (bundle.parent == null) {
bundle.setParent(parent);
}
bundle.locale = targetLocale;
bundle = putBundleInCache(cacheKey, bundle, control);
return bundle;
}
// Put NONEXISTENT_BUNDLE in the cache as a mark that
there's no bundle
// instance for the locale.
putBundleInCache(cacheKey, NONEXISTENT_BUNDLE, control);
} finally {
if (constKey.getCause() instanceof InterruptedException) {
Thread.currentThread().interrupt();
}
}
The 'constKey' is copied of the 'cacheKey' and is not used anywhere
except in finally block where it is asked for .getCause() which will
always be null since copying of CacheKey never copies the cause...
2. We can make sure that CacheKey copy constructor always copies a
CacheKey that has non-cleared moduleRef and callerRef. The original
cacheKey is created in getBundleImpl() from nun-null module and
callerModule. getBundleImpl() then calls down to findModule() with this
cacheKey. If we keep a reference to module and callerModule in
getBundleImpl() alive until the end of this method, we guarantee that
moduleRef and callerRef will not be cleared while using such cacheKey to
construct copies of it.
Here's those two changes applied to your webrev.05 and also a race in
clearCacheImpl() fixed (as reported by Daniel Fuchs):
http://cr.openjdk.java.net/~plevart/jdk9-dev/8170772_ResourceBundle.caching/webrev.06/
What do you think?
Regards, Peter
