Hi Claes and Masayoshi,
On 07/21/2016 06:54 PM, Claes Redestad wrote:
Doesn't this fail to address part of the regression, i.e., we
shouldn't go into various privileged actions at all if the module is
unnamed?
/Claes
Right, it addresses the part that lazily looks up providers via service
loader instead of in CacheKey constructor, but it introduces a
regression where previously the privileged action was not executed if
the lookup for providers failed (couldn't load the provider type or
ServiceLoader threw ServiceConfigurationError) as opposed to when the
lookup was successful but didn't find any providers in which case the
privileged action was executed and returned null, but it also set
cacheKey.callerHasProvider = Boolean.FALSE if it was null. A slight
semantical difference. I wonder if it was intended.
Anyway, here's how the old behavior can be restored:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ResourceBundle.cleanup/webrev.02/
Regards, Peter
On 2016-07-21 18:16, Peter Levart wrote:
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