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

[Naoto Sato]

Thanks, Chris. Will do it after confirming all's well with the JPRT 
(currently it's down).

Naoto

On 10/10/11 7:27 AM, Chris Hegarty wrote:
> Naoto,
>
> Forgot to add, you can probably just push the changes for the test along
> with your source changes. I modified it a little since last review.
> Reproduces one in about every ten times on one of our Dual core Linux
> x64 boxes.
>
>
>  >: hg diff Bug6989440.java
> diff -r 1e89a13d9d8f test/java/util/Locale/Bug6989440.java
> --- a/test/java/util/Locale/Bug6989440.java Mon Oct 10 10:38:35 2011 +0100
> +++ b/test/java/util/Locale/Bug6989440.java Mon Oct 10 15:21:41 2011 +0100
> @@ -37,26 +37,49 @@ import sun.util.LocaleServiceProviderPoo
> import sun.util.LocaleServiceProviderPool;
>
> public class Bug6989440 {
> - public static void main(String[] args) {
> - TestThread t1 = new TestThread(LocaleNameProvider.class);
> - TestThread t2 = new TestThread(TimeZoneNameProvider.class);
> - TestThread t3 = new TestThread(DateFormatProvider.class);
> + static volatile boolean failed; // false
> + static final int THREADS = 50;
>
> - t1.start();
> - t2.start();
> - t3.start();
> + public static void main(String[] args) throws Exception {
> + Thread[] threads = new Thread[THREADS];
> + for (int i=0; i<threads.length; i++)
> + threads[i] = new TestThread();
> + for (int i=0; i<threads.length; i++)
> + threads[i].start();
> + for (int i=0; i<threads.length; i++)
> + threads[i].join();
> +
> + if (failed)
> + throw new RuntimeException("Failed: check output");
> }
>
> static class TestThread extends Thread {
> private Class<? extends LocaleServiceProvider> cls;
> + private static int count;
>
> public TestThread(Class<? extends LocaleServiceProvider> providerClass) {
> cls = providerClass;
> }
>
> + public TestThread() {
> + int which = count++ % 3;
> + switch (which) {
> + case 0 : cls = LocaleNameProvider.class; break;
> + case 1 : cls = TimeZoneNameProvider.class; break;
> + case 2 : cls = DateFormatProvider.class; break;
> + default : throw new AssertionError("Should not reach here");
> + }
> + }
> +
> public void run() {
> - LocaleServiceProviderPool pool = LocaleServiceProviderPool.getPool(cls);
> - pool.getAvailableLocales();
> + try {
> + LocaleServiceProviderPool pool = LocaleServiceProviderPool.getPool(cls);
> + pool.getAvailableLocales();
> + } catch (Exception e) {
> + System.out.println(e);
> + e.printStackTrace();
> + failed = true;
> + }
> }
> }
> }
>
> -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
>>>>>>>>>>
>>>>>>
>>>>>
>>>