Hi Masayoshi,
Thinking of how the ResourceBundle caching is implemented, I think it
has a serious flaw. The cache is currently the following:
private static final ConcurrentMap<CacheKey, BundleReference> cacheList
...where the CacheKey is an object which contains WeakReference(s) to
the caller's ClassLoader and Module. This is OK.
BundleReference, OTOH, is a SoftReference<ResourceBundle>. The problem
is ResourceBundle(s) can be arbitrary user subclasses, which means that
they may have an implicit reference to the ClassLoader that appears in
the CacheKey. The consequence is that such cache can prevent GC-ing of
the ClassLoader for arbitrary amount of time (SoftReferences are cleared
on memory pressure).
Luckily there might be a solution. If the ResourceBundle subclasses are
always resolvable from the ClassLoader that appears in the CacheKey,
then there is a utility class that I created recently:
java.lang.reflect.ClassLoaderValue. This a package-private class as it
is currently used only in java.lang.reflect.Proxy for caching of dynamic
Proxy classes, but could be made public and moved to some
jdk.internal... package.
Basing ResourceBundle caching on this utility class could also simplify
the implementation as you wouldn't have to deal with WeakReference(s) to
ClassLoader and Module objects. You could still have a BundleReference
extending SoftReference<ResourceBundle> to release the bundles on memory
pressure but since such SoftReference(s) would be anchored in the
particular ClassLoader instance that can resolve the ResourceBundle
subclasses, it would not prevent such ClassLoader from being GCed
immediately.
I can prototype such caching if you like.
Regards, Peter
On 07/22/2016 06:07 AM, Masayoshi Okutsu wrote:
Hi Peter,
Thank you for your suggestion. Actually CacheKey is used for storing
information required to load resource bundles during a
ResourceBundle.getBundle call. The current CacheKey usage may be too
tricky and cause some maintenance problems. I will file a separate
issue on that problem. I wanted to work on some clean up of the
CacheKey usage, but I haven't had a chance.
Thanks,
Masayoshi
On 7/21/2016 10:13 PM, Peter Levart wrote:
Hi Masayoshi,
Previously the CacheKey::clone() method cleared a reference to
'providers' in the clone making the provides unreachable from the
clone and making the clone unable to obtain providers. Now you also
reset the 'providersChecked' flag which makes the clone be able to
re-obtain the providers. This is dangerous as the clone is used as a
key in the cache and is strongly reachable from the cache. A slight
future modification of code could unintentionally produce a class
loader leak. To prevent that, I would somehow mark the clone so that
any attempt to invoke getProviders() on the clone would throw
IllegalStateException.
Regards, Peter
On 07/21/2016 06:14 AM, Masayoshi Okutsu wrote:
Hi,
Please review the fix for JDK-8161203. The fix is to lazily load
ResourceBundleProviders. It's not necessary to load providers before
cache look-up.
Issue:
https://bugs.openjdk.java.net/browse/JDK-8161203
Webrev:
http://cr.openjdk.java.net/~okutsu/9/8161203/webrev.01
Thanks,
Masayoshi
