Hi Claes and Masayoshi again,
Ah sorry, I totally missed the part that skips looking up of the
providers for unnamed module. Here's that part included:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ResourceBundle.cleanup/webrev.03/
Regards, Peter
On 07/21/2016 09:49 PM, Peter Levart wrote:
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