Index: src/main/org/codehaus/groovy/runtime/metaclass/MetaClassRegistryImpl.java =================================================================== --- src/main/org/codehaus/groovy/runtime/metaclass/MetaClassRegistryImpl.java (revision 10870) +++ src/main/org/codehaus/groovy/runtime/metaclass/MetaClassRegistryImpl.java (working copy) @@ -244,6 +244,11 @@ answer = (MetaClass) weakMetaClasses.get(theClass); if (answer!=null) return answer; + // We've got a lock on the Class and we need to be sure that we're in + // the ReflectionCache before we call MetaClass.initialize(). + // There is probably another place to do this, but I want to be sure... + final CachedClass forEffect = ReflectionCache.getCachedClass(theClass); + answer = metaClassCreationHandle.create(theClass, this); answer.initialize(); if (GroovySystem.isKeepJavaMetaClasses()) { Index: src/main/org/codehaus/groovy/reflection/CachedClass.java =================================================================== --- src/main/org/codehaus/groovy/reflection/CachedClass.java (revision 10870) +++ src/main/org/codehaus/groovy/reflection/CachedClass.java (working copy) @@ -117,17 +117,42 @@ isInterface = klazz.isInterface(); isNumber = Number.class.isAssignableFrom(klazz); + } + + /** + * Initialization involves making calls back to ReflectionCache to popuplate + * the "assignable from" structure. + * Package scoped (like our constructor) because ReflectionCache is really the + * only place we should be called from. + * + * We don't need to be synchronized here because ReflectionCache is careful to + * make sure we're called exactly once. + * By the same token we could however safely lock ourself. + * But the idea here is to take out the bad locks. + */ + final void initialize() { for (Iterator it = getInterfaces().iterator(); it.hasNext(); ) { CachedClass inf = (CachedClass) it.next(); - ReflectionCache.isAssignableFrom(klazz, inf.cachedClass); + ReflectionCache.isAssignableFrom(cachedClass, inf.cachedClass); } + // If we *were* going to lock the Class/CachedClass of our parents, + // it would probably be a better idea to climb to the top then do the + // locking from the top down to here. + // But that shouldn't really be necessary since this is the wrong place for that. + // One of the keys is probably that the constructor and initialization need to be + // separated (like with MetaClassImpl). for (CachedClass cur = this; cur != null; cur = cur.getCachedSuperClass()) { - ReflectionCache.setAssignableFrom(cur.cachedClass, klazz); + ReflectionCache.setAssignableFrom(cur.cachedClass, cachedClass); } } - public synchronized CachedClass getCachedSuperClass() { + /** + * This can't be final because ReflectionClass has an inner class that extends + * CachedClass for java.lang.Object (ReflectionClass.OBJECT_CLASS) that returns + * null for this method. + */ + public CachedClass getCachedSuperClass() { if (cachedSuperClass == null) { if (!isArray) cachedSuperClass = ReflectionCache.getCachedClass(getCachedClass().getSuperclass()); @@ -229,7 +254,7 @@ return res; } - public int getModifiers() { + public final int getModifiers() { return modifiers; } @@ -238,7 +263,8 @@ } public int getSuperClassDistance() { - synchronized (getCachedClass()) { + // This method is idempotent. Don't put a dangerous lock here! + // synchronized (getCachedClass()) { if (distance == -1) { int distance = 0; for (Class klazz= getCachedClass(); klazz != null; klazz = klazz.getSuperclass()) { @@ -247,7 +273,7 @@ this.distance = distance; } return distance; - } + // } } public int hashCode() { @@ -291,7 +317,7 @@ return BytecodeHelper.getTypeDescription(getCachedClass()); } - public synchronized Reflector getReflector() { + public Reflector getReflector() { /*if (reflector == null) { final MetaClassRegistry metaClassRegistry = MetaClassRegistryImpl.getInstance(MetaClassRegistryImpl.LOAD_DEFAULT); reflector = ((MetaClassRegistryImpl)metaClassRegistry).loadReflector(getCachedClass(), Arrays.asList(getMethods())); Index: src/main/org/codehaus/groovy/reflection/ReflectionCache.java =================================================================== --- src/main/org/codehaus/groovy/reflection/ReflectionCache.java (revision 10870) +++ src/main/org/codehaus/groovy/reflection/ReflectionCache.java (working copy) @@ -78,11 +78,13 @@ static TripleKeyHashMap mopNames = new TripleKeyHashMap(); public static String getMOPMethodName(CachedClass declaringClass, String name, boolean useThis) { + synchronized (mopNames) { TripleKeyHashMap.Entry mopNameEntry = mopNames.getOrPut(declaringClass, name, Boolean.valueOf(useThis)); if (mopNameEntry.value == null) { mopNameEntry.value = new StringBuffer().append(useThis ? "this$" : "super$").append(declaringClass.getSuperClassDistance()).append("$").append(name).toString(); } return (String) mopNameEntry.value; + } } static final Map /*>*/ CACHED_CLASS_MAP = new WeakHashMap(); @@ -98,22 +100,26 @@ } static void setAssignableFrom(Class klazz, Class aClass) { + synchronized (assignableMap) { WeakDoubleKeyHashMap.Entry val = assignableMap.getOrPut(klazz, aClass); if (val.value == null) { val.value = Boolean.TRUE; } + } } public static boolean isAssignableFrom(Class klazz, Class aClass) { if (klazz == aClass) return true; + synchronized (assignableMap) { WeakDoubleKeyHashMap.Entry val = assignableMap.getOrPut(klazz, aClass); if (val.value == null) { val.value = Boolean.valueOf(klazz.isAssignableFrom(aClass)); } return ((Boolean)val.value).booleanValue(); // return klazz.isAssignableFrom(aClass); + } } static boolean arrayContentsEq(Object[] a1, Object[] a2) { @@ -161,40 +167,69 @@ return STRING_CLASS; CachedClass cachedClass; + SoftReference ref; + synchronized (CACHED_CLASS_MAP) { - SoftReference ref = (SoftReference) CACHED_CLASS_MAP.get(klazz); - if (ref == null || (cachedClass = (CachedClass) ref.get()) == null) { - if (Number.class.isAssignableFrom(klazz) || klazz.isPrimitive()) { + ref = (SoftReference) CACHED_CLASS_MAP.get(klazz); + } + + if (ref == null || (cachedClass = (CachedClass) ref.get()) == null) { + { + if (klazz.isPrimitive()) { + if (klazz == Integer.TYPE) + cachedClass = new CachedClass.IntegerCachedClass(klazz); + else + if (klazz == Double.TYPE ) + cachedClass = new CachedClass.DoubleCachedClass(klazz); + else + if (klazz == Long.TYPE) + cachedClass = new CachedClass.LongCachedClass(klazz); + else + if (klazz == Float.TYPE) + cachedClass = new CachedClass.FloatCachedClass(klazz); + else + if (klazz == Short.TYPE) + cachedClass = new CachedClass.ShortCachedClass(klazz); + else + if (klazz == Boolean.TYPE) + cachedClass = new CachedClass.BooleanCachedClass(klazz); + else + if (klazz == Character.TYPE) + cachedClass = new CachedClass.CharacterCachedClass(klazz); + else + if (klazz == Byte.TYPE) + cachedClass = new CachedClass.ByteCachedClass(klazz); + else { + //!FIXME: This shouldn't happen. assert false or throw exception... + cachedClass = new CachedClass(klazz); + } + } + else + if (Number.class.isAssignableFrom(klazz)) { if (klazz == Number.class) - cachedClass = new CachedClass.NumberCachedClass(klazz); + cachedClass = new CachedClass.NumberCachedClass(klazz); else - if (klazz == Integer.class || klazz == Integer.TYPE) - cachedClass = new CachedClass.IntegerCachedClass(klazz); + if (klazz == Integer.class) + cachedClass = new CachedClass.IntegerCachedClass(klazz); else - if (klazz == Double.class || klazz == Double.TYPE ) - cachedClass = new CachedClass.DoubleCachedClass(klazz); - else - if (klazz == BigDecimal.class ) - cachedClass = new CachedClass.BigDecimalCachedClass(klazz); - else - if (klazz == Long.class || klazz == Long.TYPE) - cachedClass = new CachedClass.LongCachedClass(klazz); - else - if (klazz == Float.class || klazz == Float.TYPE) - cachedClass = new CachedClass.FloatCachedClass(klazz); - else - if (klazz == Short.class || klazz == Short.TYPE) - cachedClass = new CachedClass.ShortCachedClass(klazz); - else - if (klazz == Boolean.TYPE) - cachedClass = new CachedClass.BooleanCachedClass(klazz); - else - if (klazz == Character.TYPE) - cachedClass = new CachedClass.CharacterCachedClass(klazz); - else - if (klazz == BigInteger.class) - cachedClass = new CachedClass.BigIntegerCachedClass(klazz); - else + if (klazz == Double.class) + cachedClass = new CachedClass.DoubleCachedClass(klazz); + else + if (klazz == BigDecimal.class ) + cachedClass = new CachedClass.BigDecimalCachedClass(klazz); + else + if (klazz == Long.class) + cachedClass = new CachedClass.LongCachedClass(klazz); + else + if (klazz == Float.class) + cachedClass = new CachedClass.FloatCachedClass(klazz); + else + if (klazz == Short.class) + cachedClass = new CachedClass.ShortCachedClass(klazz); + else + if (klazz == BigInteger.class) + cachedClass = new CachedClass.BigIntegerCachedClass(klazz); + else if (klazz == Byte.class) cachedClass = new CachedClass.ByteCachedClass(klazz); else @@ -211,9 +246,31 @@ cachedClass = new CachedClass.CharacterCachedClass(klazz); else cachedClass = new CachedClass(klazz); - CACHED_CLASS_MAP.put(klazz, new SoftReference(cachedClass)); } + + + CachedClass fasterCachedClass = null; + + // Double-check put. + synchronized (CACHED_CLASS_MAP) { + ref = (SoftReference) CACHED_CLASS_MAP.get(klazz); + + if (ref == null || (fasterCachedClass = (CachedClass) ref.get()) == null) { + CACHED_CLASS_MAP.put(klazz, new SoftReference(cachedClass)); + } else { + // We must use the one that there first, we should be able to safely toss the one we made. + // By locking Class we would eliminate this race, but until the design is corrected we risk + // deadlock. + cachedClass = fasterCachedClass; + } + } + + if (null == fasterCachedClass) { + // We've got a new CacheClass, now get loaded into the assignableMap. + cachedClass.initialize(); + } } + return cachedClass; } }