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

[Naoto Sato]

Hi David,

Thank you for your review. availableJRELocales is already declared with 
volatile keyword in the current source, so no change is involved.

Naoto

On 10/9/11 6:22 PM, David Holmes wrote:
> 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
>>>>>>>
>>>
>>