CachedIntrospectionResults and co consistently avoid synchronization (through use of ConcurrentReferenceHashMap)

As a side effect, through ConcurrentReferenceHashMap, we're using soft references for non-cache-safe bean classes.

Issue: SPR-11867

spring-beans/src/main/java/org/springframework/beans/BeanUtils.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -29,8 +29,7 @@ import java.util.Collections;
 import java.util.Date;
 import java.util.List;
 import java.util.Locale;
-import java.util.Map;
-import java.util.WeakHashMap;
+import java.util.Set;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -38,6 +37,7 @@ import org.apache.commons.logging.LogFactory;
 import org.springframework.core.MethodParameter;
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
+import org.springframework.util.ConcurrentReferenceHashMap;
 import org.springframework.util.ReflectionUtils;
 import org.springframework.util.StringUtils;
 
@@ -57,9 +57,8 @@ public abstract class BeanUtils {
 
 	private static final Log logger = LogFactory.getLog(BeanUtils.class);
 
-	// Effectively using a WeakHashMap as a Set
-	private static final Map<Class<?>, Boolean> unknownEditorTypes =
-			Collections.synchronizedMap(new WeakHashMap<Class<?>, Boolean>());
+	private static final Set<Class<?>> unknownEditorTypes =
+			Collections.newSetFromMap(new ConcurrentReferenceHashMap<Class<?>, Boolean>(64));
 
 
 	/**
@@ -404,7 +403,7 @@ public abstract class BeanUtils {
 	 * @return the corresponding editor, or {@code null} if none found
 	 */
 	public static PropertyEditor findEditorByConvention(Class<?> targetType) {
-		if (targetType == null || targetType.isArray() || unknownEditorTypes.containsKey(targetType)) {
+		if (targetType == null || targetType.isArray() || unknownEditorTypes.contains(targetType)) {
 			return null;
 		}
 		ClassLoader cl = targetType.getClassLoader();
@@ -431,7 +430,7 @@ public abstract class BeanUtils {
 					logger.warn("Editor class [" + editorName +
 							"] does not implement [java.beans.PropertyEditor] interface");
 				}
-				unknownEditorTypes.put(targetType, Boolean.TRUE);
+				unknownEditorTypes.add(targetType);
 				return null;
 			}
 			return (PropertyEditor) instantiateClass(editorClass);
@@ -441,7 +440,7 @@ public abstract class BeanUtils {
 				logger.debug("No property editor [" + editorName + "] found for type " +
 						targetType.getName() + " according to 'Editor' suffix convention");
 			}
-			unknownEditorTypes.put(targetType, Boolean.TRUE);
+			unknownEditorTypes.add(targetType);
 			return null;
 		}
 	}

spring-beans/src/main/java/org/springframework/beans/CachedIntrospectionResults.java

@@ -20,15 +20,12 @@ import java.beans.BeanInfo;
 import java.beans.IntrospectionException;
 import java.beans.Introspector;
 import java.beans.PropertyDescriptor;
-import java.lang.ref.Reference;
-import java.lang.ref.WeakReference;
-import java.util.HashSet;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.WeakHashMap;
 import java.util.concurrent.ConcurrentHashMap;
 
 import org.apache.commons.logging.Log;
@@ -38,6 +35,7 @@ import org.springframework.core.SpringProperties;
 import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.core.io.support.SpringFactoriesLoader;
 import org.springframework.util.ClassUtils;
+import org.springframework.util.ConcurrentReferenceHashMap;
 import org.springframework.util.StringUtils;
 
 /**
@@ -107,14 +105,22 @@ public class CachedIntrospectionResults {
 	 * Set of ClassLoaders that this CachedIntrospectionResults class will always
 	 * accept classes from, even if the classes do not qualify as cache-safe.
 	 */
-	static final Set<ClassLoader> acceptedClassLoaders = new HashSet<ClassLoader>();
+	static final Set<ClassLoader> acceptedClassLoaders =
+			Collections.newSetFromMap(new ConcurrentHashMap<ClassLoader, Boolean>(16));
 
 	/**
-	 * Map keyed by class containing CachedIntrospectionResults.
-	 * Needs to be a WeakHashMap with WeakReferences as values to allow
-	 * for proper garbage collection in case of multiple class loaders.
+	 * Map keyed by Class containing CachedIntrospectionResults, strongly held.
+	 * This variant is being used for cache-safe bean classes.
 	 */
-	static final Map<Class<?>, Object> classCache = new WeakHashMap<Class<?>, Object>();
+	static final Map<Class<?>, CachedIntrospectionResults> strongClassCache =
+			new ConcurrentHashMap<Class<?>, CachedIntrospectionResults>(64);
+
+	/**
+	 * Map keyed by Class containing CachedIntrospectionResults, softly held.
+	 * This variant is being used for non-cache-safe bean classes.
+	 */
+	static final Map<Class<?>, CachedIntrospectionResults> softClassCache =
+			new ConcurrentReferenceHashMap<Class<?>, CachedIntrospectionResults>(64);
 
 
 	/**
@@ -131,9 +137,7 @@ public class CachedIntrospectionResults {
 	 */
 	public static void acceptClassLoader(ClassLoader classLoader) {
 		if (classLoader != null) {
-			synchronized (acceptedClassLoaders) {
-				acceptedClassLoaders.add(classLoader);
-			}
+			acceptedClassLoaders.add(classLoader);
 		}
 	}
 
@@ -144,20 +148,22 @@ public class CachedIntrospectionResults {
 	 * @param classLoader the ClassLoader to clear the cache for
 	 */
 	public static void clearClassLoader(ClassLoader classLoader) {
-		synchronized (classCache) {
-			for (Iterator<Class<?>> it = classCache.keySet().iterator(); it.hasNext();) {
-				Class<?> beanClass = it.next();
-				if (isUnderneathClassLoader(beanClass.getClassLoader(), classLoader)) {
-					it.remove();
-				}
+		for (Iterator<ClassLoader> it = acceptedClassLoaders.iterator(); it.hasNext();) {
+			ClassLoader registeredLoader = it.next();
+			if (isUnderneathClassLoader(registeredLoader, classLoader)) {
+				it.remove();
 			}
 		}
-		synchronized (acceptedClassLoaders) {
-			for (Iterator<ClassLoader> it = acceptedClassLoaders.iterator(); it.hasNext();) {
-				ClassLoader registeredLoader = it.next();
-				if (isUnderneathClassLoader(registeredLoader, classLoader)) {
-					it.remove();
-				}
+		for (Iterator<Class<?>> it = strongClassCache.keySet().iterator(); it.hasNext();) {
+			Class<?> beanClass = it.next();
+			if (isUnderneathClassLoader(beanClass.getClassLoader(), classLoader)) {
+				it.remove();
+			}
+		}
+		for (Iterator<Class<?>> it = softClassCache.keySet().iterator(); it.hasNext();) {
+			Class<?> beanClass = it.next();
+			if (isUnderneathClassLoader(beanClass.getClassLoader(), classLoader)) {
+				it.remove();
 			}
 		}
 	}
@@ -170,35 +176,25 @@ public class CachedIntrospectionResults {
 	 */
 	@SuppressWarnings("unchecked")
 	static CachedIntrospectionResults forClass(Class<?> beanClass) throws BeansException {
-		CachedIntrospectionResults results;
-		Object value;
-		synchronized (classCache) {
-			value = classCache.get(beanClass);
+		CachedIntrospectionResults results = strongClassCache.get(beanClass);
+		if (results != null) {
+			return results;
 		}
-		if (value instanceof Reference) {
-			Reference<CachedIntrospectionResults> ref = (Reference<CachedIntrospectionResults>) value;
-			results = ref.get();
+		results = softClassCache.get(beanClass);
+		if (results != null) {
+			return results;
 		}
-		else {
-			results = (CachedIntrospectionResults) value;
+
+		results = new CachedIntrospectionResults(beanClass);
+		if (ClassUtils.isCacheSafe(beanClass, CachedIntrospectionResults.class.getClassLoader()) ||
+				isClassLoaderAccepted(beanClass.getClassLoader())) {
+			strongClassCache.put(beanClass, results);
 		}
-		if (results == null) {
-			if (ClassUtils.isCacheSafe(beanClass, CachedIntrospectionResults.class.getClassLoader()) ||
-					isClassLoaderAccepted(beanClass.getClassLoader())) {
-				results = new CachedIntrospectionResults(beanClass);
-				synchronized (classCache) {
-					classCache.put(beanClass, results);
-				}
-			}
-			else {
-				if (logger.isDebugEnabled()) {
-					logger.debug("Not strongly caching class [" + beanClass.getName() + "] because it is not cache-safe");
-				}
-				results = new CachedIntrospectionResults(beanClass);
-				synchronized (classCache) {
-					classCache.put(beanClass, new WeakReference<CachedIntrospectionResults>(results));
-				}
+		else {
+			if (logger.isDebugEnabled()) {
+				logger.debug("Not strongly caching class [" + beanClass.getName() + "] because it is not cache-safe");
 			}
+			softClassCache.put(beanClass, results);
 		}
 		return results;
 	}
@@ -211,13 +207,7 @@ public class CachedIntrospectionResults {
 	 * @see #acceptClassLoader
 	 */
 	private static boolean isClassLoaderAccepted(ClassLoader classLoader) {
-		// Iterate over array copy in order to avoid synchronization for the entire
-		// ClassLoader check (avoiding a synchronized acceptedClassLoaders Iterator).
-		ClassLoader[] acceptedLoaderArray;
-		synchronized (acceptedClassLoaders) {
-			acceptedLoaderArray = acceptedClassLoaders.toArray(new ClassLoader[acceptedClassLoaders.size()]);
-		}
-		for (ClassLoader acceptedLoader : acceptedLoaderArray) {
+		for (ClassLoader acceptedLoader : acceptedClassLoaders) {
 			if (isUnderneathClassLoader(classLoader, acceptedLoader)) {
 				return true;
 			}

spring-beans/src/main/java/org/springframework/beans/GenericTypeAwarePropertyDescriptor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -42,18 +42,18 @@ class GenericTypeAwarePropertyDescriptor extends PropertyDescriptor {
 
 	private final Class<?> beanClass;
 
+	private final Class<?> propertyEditorClass;
+
 	private final Method readMethod;
 
 	private final Method writeMethod;
 
-	private final Class<?> propertyEditorClass;
-
 	private volatile Set<Method> ambiguousWriteMethods;
 
-	private Class<?> propertyType;
-
 	private MethodParameter writeMethodParameter;
 
+	private Class<?> propertyType;
+
 
 	public GenericTypeAwarePropertyDescriptor(Class<?> beanClass, String propertyName,
 			Method readMethod, Method writeMethod, Class<?> propertyEditorClass)
@@ -78,27 +78,44 @@ class GenericTypeAwarePropertyDescriptor extends PropertyDescriptor {
 		this.readMethod = readMethodToUse;
 		this.writeMethod = writeMethodToUse;
 
-		if (this.writeMethod != null && this.readMethod == null) {
-			// Write method not matched against read method: potentially ambiguous through
-			// several overloaded variants, in which case an arbitrary winner has been chosen
-			// by the JDK's JavaBeans Introspector...
-			Set<Method> ambiguousCandidates = new HashSet<Method>();
-			for (Method method : beanClass.getMethods()) {
-				if (method.getName().equals(writeMethodToUse.getName()) &&
-						!method.equals(writeMethodToUse) && !method.isBridge()) {
-					ambiguousCandidates.add(method);
+		if (this.writeMethod != null) {
+			if (this.readMethod == null) {
+				// Write method not matched against read method: potentially ambiguous through
+				// several overloaded variants, in which case an arbitrary winner has been chosen
+				// by the JDK's JavaBeans Introspector...
+				Set<Method> ambiguousCandidates = new HashSet<Method>();
+				for (Method method : beanClass.getMethods()) {
+					if (method.getName().equals(writeMethodToUse.getName()) &&
+							!method.equals(writeMethodToUse) && !method.isBridge()) {
+						ambiguousCandidates.add(method);
+					}
+				}
+				if (!ambiguousCandidates.isEmpty()) {
+					this.ambiguousWriteMethods = ambiguousCandidates;
 				}
 			}
-			if (!ambiguousCandidates.isEmpty()) {
-				this.ambiguousWriteMethods = ambiguousCandidates;
-			}
+			this.writeMethodParameter = new MethodParameter(this.writeMethod, 0);
+			GenericTypeResolver.resolveParameterType(this.writeMethodParameter, this.beanClass);
+		}
+
+		if (this.readMethod != null) {
+			this.propertyType = GenericTypeResolver.resolveReturnType(this.readMethod, this.beanClass);
+		}
+		else if (this.writeMethodParameter != null) {
+			this.propertyType = this.writeMethodParameter.getParameterType();
 		}
 	}
 
+
 	public Class<?> getBeanClass() {
 		return this.beanClass;
 	}
 
+	@Override
+	public Class<?> getPropertyEditorClass() {
+		return this.propertyEditorClass;
+	}
+
 	@Override
 	public Method getReadMethod() {
 		return this.readMethod;
@@ -120,39 +137,13 @@ class GenericTypeAwarePropertyDescriptor extends PropertyDescriptor {
 		return this.writeMethod;
 	}
 
-	@Override
-	public Class<?> getPropertyEditorClass() {
-		return this.propertyEditorClass;
+	public MethodParameter getWriteMethodParameter() {
+		return this.writeMethodParameter;
 	}
 
 	@Override
-	public synchronized Class<?> getPropertyType() {
-		if (this.propertyType == null) {
-			if (this.readMethod != null) {
-				this.propertyType = GenericTypeResolver.resolveReturnType(this.readMethod, this.beanClass);
-			}
-			else {
-				MethodParameter writeMethodParam = getWriteMethodParameter();
-				if (writeMethodParam != null) {
-					this.propertyType = writeMethodParam.getParameterType();
-				}
-				else {
-					this.propertyType = super.getPropertyType();
-				}
-			}
-		}
+	public Class<?> getPropertyType() {
 		return this.propertyType;
 	}
 
-	public synchronized MethodParameter getWriteMethodParameter() {
-		if (this.writeMethod == null) {
-			return null;
-		}
-		if (this.writeMethodParameter == null) {
-			this.writeMethodParameter = new MethodParameter(this.writeMethod, 0);
-			GenericTypeResolver.resolveParameterType(this.writeMethodParameter, this.beanClass);
-		}
-		return this.writeMethodParameter;
-	}
-
 }

spring-beans/src/test/java/org/springframework/beans/CachedIntrospectionResultsTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -33,35 +33,35 @@ import static org.junit.Assert.*;
  * @author Chris Beams
  * @author Arjen Poutsma
  */
-public final class CachedIntrospectionResultsTests {
+public class CachedIntrospectionResultsTests {
 
 	@Test
 	public void acceptAndClearClassLoader() throws Exception {
 		BeanWrapper bw = new BeanWrapperImpl(TestBean.class);
 		assertTrue(bw.isWritableProperty("name"));
 		assertTrue(bw.isWritableProperty("age"));
-		assertTrue(CachedIntrospectionResults.classCache.containsKey(TestBean.class));
+		assertTrue(CachedIntrospectionResults.strongClassCache.containsKey(TestBean.class));
 
 		ClassLoader child = new OverridingClassLoader(getClass().getClassLoader());
 		Class<?> tbClass = child.loadClass("org.springframework.tests.sample.beans.TestBean");
-		assertFalse(CachedIntrospectionResults.classCache.containsKey(tbClass));
+		assertFalse(CachedIntrospectionResults.strongClassCache.containsKey(tbClass));
 		CachedIntrospectionResults.acceptClassLoader(child);
 		bw = new BeanWrapperImpl(tbClass);
 		assertTrue(bw.isWritableProperty("name"));
 		assertTrue(bw.isWritableProperty("age"));
-		assertTrue(CachedIntrospectionResults.classCache.containsKey(tbClass));
+		assertTrue(CachedIntrospectionResults.strongClassCache.containsKey(tbClass));
 		CachedIntrospectionResults.clearClassLoader(child);
-		assertFalse(CachedIntrospectionResults.classCache.containsKey(tbClass));
+		assertFalse(CachedIntrospectionResults.strongClassCache.containsKey(tbClass));
 
-		assertTrue(CachedIntrospectionResults.classCache.containsKey(TestBean.class));
+		assertTrue(CachedIntrospectionResults.strongClassCache.containsKey(TestBean.class));
 	}
 
 	@Test
 	public void clearClassLoaderForSystemClassLoader() throws Exception {
 		BeanUtils.getPropertyDescriptors(ArrayList.class);
-		assertTrue(CachedIntrospectionResults.classCache.containsKey(ArrayList.class));
+		assertTrue(CachedIntrospectionResults.strongClassCache.containsKey(ArrayList.class));
 		CachedIntrospectionResults.clearClassLoader(ArrayList.class.getClassLoader());
-		assertFalse(CachedIntrospectionResults.classCache.containsKey(ArrayList.class));
+		assertFalse(CachedIntrospectionResults.strongClassCache.containsKey(ArrayList.class));
 	}
 
 	@Test