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

[Naoto Sato]

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