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
