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

[Chris Hegarty]

Thumbs up from me too.

-Chris.

On 10/10/2011 05:44, David Holmes wrote:
> 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
>>>>>>>>>
>>>>>
>>>>
>>