On 2016-07-21 15:13, 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.
Seems to me that simply setting providersChecked = true; in clone()
should keep the old behavior?
Also, while not trying to make ResourceBundle thread-safe, perhaps
getProviders() would be more idiomatically implemented as:
ServiceLoader<ResourceBundleProvider> getProviders() {
ServiceLoader<ResourceBundleProvider> providers = this.providers;
if (!providersChecked) {
this.providers = providers = getServiceLoader(getModule(), name);
providersChecked = true;
}
return providers;
}
... to guard against the method ever returning null on weakly-ordered CPUs.
Thanks!
/Claes
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
