Hi Masayoshi,
The whole story about InterruptedException (in my previous message) is
moot. I have checked all the places where the 'cause' is reported into
the CacheKey and there's nowhere a possibility that an
InterruptedException would get set. So the cloning of cacheKey (in line
1766) and checking for InterruptedException in the clone can all be just
removed from the code.
Code around providers can be simplified too. Without the need for a
boolean flag, you just have to change the signature of 'providers' field
from ServiceLoader<ResourceBundleProvider> to
Iterable<ResourceBundleProvider> and assign a Collections.emptyList() to
it in clone().
getServiceLoader() method(s) can similarly just return
Collections.emptyList() instead of null after changing their
signature(s). CacheKey does not need a hasProviders() method and any
checking for null can get away.
I also found a race in putBundleInCache() method. It tries hard to
return the bundle that gets installed into cache 1st, but it doesn't try
hard enough. If there is an expired entry already in the cache, it just
overwrites it with new bundle, but doesn't take into account the
possibility that two or more threads can do the same thing - just
overwrite each other's entry and each return its own instance to the caller.
The effort that the code takes in this method to prevent the newly
allocated BundleReference from being enqueued into the ReferenceQueue is
also moot. The reference will only get discovered and enqueued if it
remains reachable. If if doesn't get installed into the cache it will be
GCed and not enqueued.
All of the above can be fixed in the following way:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ResourceBundle.cleanup/webrev.01/
Take whatever you want from it. The rest can be included in some new
cleanup task if you like it.
Regards, Peter
On 07/21/2016 03: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
