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

[David Holmes]

On 10/10/2011 1:35 PM, Naoto Sato wrote:
> Hi David,
>
> Thank you for your review. availableJRELocales is already declared with
> volatile keyword in the current source, so no change is involved.

Sorry - my mistake. I misread something Chris posted earlier.

Cheers,
David

> 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
>>>>>>>>
>>>>
>>>
>