<i18n dev> testcase failure java/util/Locale/Bug6989440.java

[David Holmes]

Hi Naoto,

This looks okay to me, but is missing the change to make 
availableJRELocales volatile (which is needed to ensure safe publication)

David

On 8/10/2011 4:40 AM, Naoto Sato wrote:
> OK here is the proposed fix (including David's suggestion for using
> putIfAbsent). Can anyone please review this?
>
> --- a/src/share/classes/sun/util/LocaleServiceProviderPool.java
> +++ b/src/share/classes/sun/util/LocaleServiceProviderPool.java
> @@ -40,6 +40,7 @@
> import java.util.ServiceLoader;
> import java.util.Set;
> import java.util.concurrent.ConcurrentHashMap;
> +import java.util.concurrent.ConcurrentMap;
> import java.util.spi.LocaleServiceProvider;
>
> import sun.util.logging.PlatformLogger;
> @@ -57,8 +58,8 @@
> * A Map that holds singleton instances of this class. Each instance holds a
> * set of provider implementations of a particular locale sensitive service.
> */
> - private static Map<Class, LocaleServiceProviderPool> poolOfPools =
> - new ConcurrentHashMap<Class, LocaleServiceProviderPool>();
> + private static ConcurrentMap<Class, LocaleServiceProviderPool>
> poolOfPools =
> + new ConcurrentHashMap<>();
>
> /**
> * A Set containing locale service providers that implement the
> @@ -109,7 +110,7 @@
> if (pool == null) {
> LocaleServiceProviderPool newPool =
> new LocaleServiceProviderPool(providerClass);
> - pool = poolOfPools.put(providerClass, newPool);
> + pool = poolOfPools.putIfAbsent(providerClass, newPool);
> if (pool == null) {
> pool = newPool;
> }
> @@ -257,10 +258,11 @@
> synchronized (LocaleServiceProviderPool.class) {
> if (availableJRELocales == null) {
> Locale[] allLocales = LocaleData.getAvailableLocales();
> - availableJRELocales = new ArrayList<Locale>(allLocales.length);
> + List<Locale> tmpList = new ArrayList<>(allLocales.length);
> for (Locale locale : allLocales) {
> - availableJRELocales.add(getLookupLocale(locale));
> + tmpList.add(getLookupLocale(locale));
> }
> + availableJRELocales = tmpList;
> }
> }
> }
> ---
>
> Naoto
>
> On 10/6/11 2:59 PM, Naoto Sato wrote:
>> Hi David,
>>
>> Thank you for the quick look and the fix!
>>
>> Naoto
>>
>> On 10/6/11 10:09 AM, David Holmes wrote:
>>> Hi Chris,
>>>
>>> Thanks. Here's the bug:
>>>
>>> private List<Locale> getJRELocales() {
>>> if (availableJRELocales == null) {
>>> synchronized (LocaleServiceProviderPool.class) {
>>> if (availableJRELocales == null) {
>>> Locale[] allLocales = LocaleData.getAvailableLocales();
>>> availableJRELocales = new ArrayList<Locale>(allLocales.length); <<====
>>> YIKES we just published an empty ArrayList
>>> for (Locale locale : allLocales) {
>>> availableJRELocales.add(getLookupLocale(locale));
>>> }
>>> }
>>> }
>>> }
>>> return availableJRELocales;
>>> }
>>>
>>>
>>> availableJRELocales is a static variable shared across all pools. But it
>>> is being published before it gets populated. We need to use a temporary
>>> for the new ArrayList and only assign to availableJRELocales after it is
>>> populated.
>>>
>>> In addition, as you mentioned, availableJRELocales needs to be volatile
>>> for this DCL pattern to be correct.
>>>
>>> Thanks,
>>> David
>>>
>>> On 6/10/2011 9:02 PM, Chris Hegarty wrote:
>>>> I see several exceptions, similar to the following (numbers vary):
>>>>
>>>> Exception in thread "Thread-23"
>>>> java.util.ConcurrentModificationException: Expected: 96, got: 156
>>>> at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:819)
>>>> at java.util.ArrayList$Itr.next(ArrayList.java:791)
>>>> at java.util.AbstractCollection.addAll(AbstractCollection.java:333)
>>>> at java.util.HashSet.<init>(HashSet.java:117)
>>>> at
>>>> sun.util.LocaleServiceProviderPool.getAvailableLocales(LocaleServiceProviderPool.java:206)
>>>>
>>>>
>>>>
>>>>
>>>> at Interrupter$TestThread.run(Interrupter.java:49)
>>>
>>>
>>>>
>>>> There would appear to be 156 JRE Locales ( at least on this system ),
>>>> modCount is incremented for each add(), but when the iterator is
>>>> created
>>>> ( implicitly during the HastSet.addAll ) it sees a different value for
>>>> modCount.
>>>>
>>>> I think there is a visibility issue here. availableJRELocales is
>>>> volatile, but the List reference returned from getJRELocales is not. In
>>>> the case where availableJRELocales is not null there will be no memory
>>>> barrier to force a HB relationship. Or maybe I've gotten this wrong?
>>>> His
>>>> is quite bizarre, or maybe it is just the overly complicated use of
>>>> locking/DCL in this class.
>>>>
>>>> -Chris.
>>>>
>>>> On 10/ 5/11 07:01 PM, David Holmes wrote:
>>>>> This might not be related to the CME problem, but here:
>>>>>
>>>>> public static LocaleServiceProviderPool getPool(Class<? extends
>>>>> LocaleServiceProvider> providerClass) {
>>>>> LocaleServiceProviderPool pool = poolOfPools.get(providerClass);
>>>>> if (pool == null) {
>>>>> LocaleServiceProviderPool newPool =
>>>>> new LocaleServiceProviderPool(providerClass);
>>>>> pool = poolOfPools.put(providerClass, newPool);
>>>>> if (pool == null) {
>>>>> pool = newPool;
>>>>> }
>>>>> }
>>>>>
>>>>> return pool;
>>>>> }
>>>>>
>>>>> we should probably be using poolOfPools.putIfAbsent(providerClass,
>>>>> newPool)
>>>>>
>>>>> David
>>>>>
>>>>> On 6/10/2011 3:35 AM, Naoto Sato wrote:
>>>>>> I will look into this. Reopened the original CR.
>>>>>>
>>>>>> Naoto
>>>>>>
>>>>>> On 10/5/11 9:58 AM, Alan Bateman wrote:
>>>>>>> Chris Hegarty wrote:
>>>>>>>> Alan, Naoto, David
>>>>>>>>
>>>>>>>> I filed CR 7098100: java/util/Locale/Bug6989440.java fails
>>>>>>>> intermittently.
>>>>>>>>
>>>>>>>> If you're ok with it please review the patch (below) and I can
>>>>>>>> push it
>>>>>>>> to the tl repo. Job done!
>>>>>>> I assume there is also some underlying issue in the Locale code and
>>>>>>> this
>>>>>>> might get hidden if we fix the test (I"ve no objection to fixing the
>>>>>>> test of course, just thinking that we should study the Locale
>>>>>>> code to
>>>>>>> try to identify the deadlock or hang or whatever it is that is
>>>>>>> causing
>>>>>>> the threads in this test not to terminate).
>>>>>>>
>>>>>>> -Alan
>>>>>>
>>
>