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
