Hi,
Unrelated to this issue, I think I found a bug which may cause thread
interrupt to be swallowed. There's a 2nd place where CacheKey is cloned,
but for a reason I cannot explain:
1760 if (bundle != NONEXISTENT_BUNDLE) {
1761 CacheKey constKey = (CacheKey) cacheKey.clone();
1762
1763 try {
1764 if (module.isNamed()) {
1765 bundle = loadBundle(cacheKey, formats, control,
module);
1766 } else {
1767 bundle = loadBundle(cacheKey, formats, control,
expiredBundle);
1768 }
1769 if (bundle != null) {
1770 if (bundle.parent == null) {
1771 bundle.setParent(parent);
1772 }
1773 bundle.locale = targetLocale;
1774 bundle = putBundleInCache(cacheKey, bundle,
control);
1775 return bundle;
1776 }
1777
1778 // Put NONEXISTENT_BUNDLE in the cache as a mark
that there's no bundle
1779 // instance for the locale.
1780 putBundleInCache(cacheKey, NONEXISTENT_BUNDLE,
control);
1781 } finally {
1782 if (constKey.getCause() instanceof
InterruptedException) {
1783 Thread.currentThread().interrupt();
1784 }
1785 }
... the clone ('constKey') is only used in finally clause to check
whether its 'cause' is an InterruptedException and set the thread
interrupt status if it is. The trouble is, CacheKey.clone() clears the
'cause' on the clone (line 795):
782 @Override
783 public Object clone() {
784 try {
785 CacheKey clone = (CacheKey) super.clone();
786 if (loaderRef != null) {
787 clone.loaderRef = new
KeyElementReference<>(getLoader(),
788 referenceQueue, clone);
789 }
790 clone.moduleRef = new
KeyElementReference<>(getModule(),
791 referenceQueue, clone);
792 // Clear the reference to ResourceBundleProviders
793 clone.providers = null;
794 // Clear the reference to a Throwable
795 clone.cause = null;
796 // Clear callerHasProvider
797 clone.callerHasProvider = null;
798 return clone;
799 } catch (CloneNotSupportedException e) {
800 //this should never happen
801 throw new InternalError(e);
802 }
803 }
...so the if statement in finally clause never executes its body. Now
what was the programmers intent? Did (s)he want to check the cacheKey's
cause as it was before executing loadBundle statements with the
cacheKey? If so, then it would be better to just remember the cause and
not clone the whole cacheKey...
Regards, Peter
On 07/21/2016 04:08 PM, Peter Levart wrote:
Hi Claes,
On 07/21/2016 03:21 PM, Claes Redestad wrote:
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
I thought of that too, but then I checked where these methods are
called from and it seems that the CacheKey instance that is used to
obtain providers is freshly allocated at each call to static
getBundleImpl() and just passed down to other methods. So only a
single thread is ever accessing a particular CackeKey instance.
A clone of CackeKey is then inserted into the shared cache, but
without a reference to providers.
Regards, Peter
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
