Avoid unnecessary synchronization in ContextCache, plus forward-ported polishing

Issue: SPR-12409

spring-test/src/main/java/org/springframework/test/context/CacheAwareContextLoaderDelegate.java

@@ -38,15 +38,13 @@ public interface CacheAwareContextLoaderDelegate {
 	 * Load the {@linkplain ApplicationContext application context} for the supplied
 	 * {@link MergedContextConfiguration} by delegating to the {@link ContextLoader}
 	 * configured in the given {@code MergedContextConfiguration}.
-	 *
 	 * <p>If the context is present in the <em>context cache</em> it will simply
 	 * be returned; otherwise, it will be loaded, stored in the cache, and returned.
-	 *
 	 * @param mergedContextConfiguration the merged context configuration to use
 	 * to load the application context; never {@code null}
 	 * @return the application context
-	 * @throws IllegalStateException if an error occurs while retrieving or
-	 * loading the application context
+	 * @throws IllegalStateException if an error occurs while retrieving or loading
+	 * the application context
 	 */
 	ApplicationContext loadContext(MergedContextConfiguration mergedContextConfiguration);
 
@@ -55,16 +53,13 @@ public interface CacheAwareContextLoaderDelegate {
 	 * supplied {@link MergedContextConfiguration} from the <em>context cache</em>
 	 * and {@linkplain ConfigurableApplicationContext#close() close} it if it is
 	 * an instance of {@link ConfigurableApplicationContext}.
-	 *
 	 * <p>The semantics of the supplied {@code HierarchyMode} must be honored when
 	 * removing the context from the cache. See the Javadoc for {@link HierarchyMode}
 	 * for details.
-	 *
 	 * <p>Generally speaking, this method should only be called if the state of
 	 * a singleton bean has been changed (potentially affecting future interaction
 	 * with the context) or if the context needs to be prematurely removed from
 	 * the cache.
-	 *
 	 * @param mergedContextConfiguration the merged context configuration for the
 	 * application context to close; never {@code null}
 	 * @param hierarchyMode the hierarchy mode; may be {@code null} if the context

spring-test/src/main/java/org/springframework/test/context/ContextCache.java

@@ -48,13 +48,11 @@ import org.springframework.util.Assert;
  */
 class ContextCache {
 
-	private final Object monitor = new Object();
-
 	/**
 	 * Map of context keys to Spring {@code ApplicationContext} instances.
 	 */
-	private final Map<MergedContextConfiguration, ApplicationContext> contextMap = new ConcurrentHashMap<MergedContextConfiguration, ApplicationContext>(
-		64);
+	private final Map<MergedContextConfiguration, ApplicationContext> contextMap =
+			new ConcurrentHashMap<MergedContextConfiguration, ApplicationContext>(64);
 
 	/**
 	 * Map of parent keys to sets of children keys, representing a top-down <em>tree</em>
@@ -62,8 +60,8 @@ class ContextCache {
 	 * need to be recursively removed and closed when removing a context that is a parent
 	 * of other contexts.
 	 */
-	private final Map<MergedContextConfiguration, Set<MergedContextConfiguration>> hierarchyMap = new ConcurrentHashMap<MergedContextConfiguration, Set<MergedContextConfiguration>>(
-		64);
+	private final Map<MergedContextConfiguration, Set<MergedContextConfiguration>> hierarchyMap =
+			new ConcurrentHashMap<MergedContextConfiguration, Set<MergedContextConfiguration>>(64);
 
 	private final AtomicInteger hitCount = new AtomicInteger();
 
@@ -71,50 +69,41 @@ class ContextCache {
 
 
 	/**
-	 * Clears all contexts from the cache and clears context hierarchy information as
-	 * well.
+	 * Clear all contexts from the cache and clears context hierarchy information as well.
 	 */
-	void clear() {
-		synchronized (monitor) {
+	public void clear() {
 		this.contextMap.clear();
 		this.hierarchyMap.clear();
 	}
-	}
 
 	/**
-	 * Clears hit and miss count statistics for the cache (i.e., resets counters to zero).
+	 * Clear hit and miss count statistics for the cache (i.e., resets counters to zero).
 	 */
-	void clearStatistics() {
+	public void clearStatistics() {
 		this.hitCount.set(0);
 		this.missCount.set(0);
 	}
 
 	/**
 	 * Return whether there is a cached context for the given key.
-	 *
 	 * @param key the context key (never {@code null})
 	 */
-	boolean contains(MergedContextConfiguration key) {
+	public boolean contains(MergedContextConfiguration key) {
 		Assert.notNull(key, "Key must not be null");
-		synchronized (monitor) {
 		return this.contextMap.containsKey(key);
 	}
-	}
 
 	/**
 	 * Obtain a cached {@code ApplicationContext} for the given key.
-	 *
-	 * <p>The {@link #getHitCount() hit} and {@link #getMissCount() miss} counts will be
-	 * updated accordingly.
-	 *
+	 * <p>The {@link #getHitCount() hit} and {@link #getMissCount() miss} counts will
+	 * be updated accordingly.
 	 * @param key the context key (never {@code null})
-	 * @return the corresponding {@code ApplicationContext} instance, or {@code null} if
-	 * not found in the cache.
+	 * @return the corresponding {@code ApplicationContext} instance, or {@code null}
+	 * if not found in the cache
 	 * @see #remove
 	 */
-	ApplicationContext get(MergedContextConfiguration key) {
+	public ApplicationContext get(MergedContextConfiguration key) {
 		Assert.notNull(key, "Key must not be null");
-		synchronized (monitor) {
 		ApplicationContext context = this.contextMap.get(key);
 		if (context == null) {
 			this.missCount.incrementAndGet();
@@ -124,14 +113,13 @@ class ContextCache {
 		}
 		return context;
 	}
-	}
 
 	/**
 	 * Get the overall hit count for this cache.
-	 * <p>A <em>hit</em> is an access to the cache, which returned a non-null context for
-	 * a queried key.
+	 * <p>A <em>hit</em> is an access to the cache, which returned a non-null context
+	 * for a queried key.
 	 */
-	int getHitCount() {
+	public int getHitCount() {
 		return this.hitCount.get();
 	}
 
@@ -140,56 +128,48 @@ class ContextCache {
 	 * <p>A <em>miss</em> is an access to the cache, which returned a {@code null} context
 	 * for a queried key.
 	 */
-	int getMissCount() {
+	public int getMissCount() {
 		return this.missCount.get();
 	}
 
 	/**
-	 * Explicitly add an {@code ApplicationContext} instance to the cache under the given
-	 * key.
-	 *
+	 * Explicitly add an {@code ApplicationContext} instance to the cache under the given key.
 	 * @param key the context key (never {@code null})
 	 * @param context the {@code ApplicationContext} instance (never {@code null})
 	 */
-	void put(MergedContextConfiguration key, ApplicationContext context) {
+	public void put(MergedContextConfiguration key, ApplicationContext context) {
 		Assert.notNull(key, "Key must not be null");
 		Assert.notNull(context, "ApplicationContext must not be null");
 
-		synchronized (monitor) {
 		this.contextMap.put(key, context);
-
 		MergedContextConfiguration child = key;
 		MergedContextConfiguration parent = child.getParent();
 		while (parent != null) {
-				Set<MergedContextConfiguration> list = hierarchyMap.get(parent);
+			Set<MergedContextConfiguration> list = this.hierarchyMap.get(parent);
 			if (list == null) {
 				list = new HashSet<MergedContextConfiguration>();
-					hierarchyMap.put(parent, list);
+				this.hierarchyMap.put(parent, list);
 			}
 			list.add(child);
 			child = parent;
 			parent = child.getParent();
 		}
 	}
-	}
 
 	/**
 	 * Remove the context with the given key from the cache and explicitly
 	 * {@linkplain ConfigurableApplicationContext#close() close} it if it is an
 	 * instance of {@link ConfigurableApplicationContext}.
-	 *
 	 * <p>Generally speaking, you would only call this method if you change the
 	 * state of a singleton bean, potentially affecting future interaction with
 	 * the context.
-	 *
 	 * <p>In addition, the semantics of the supplied {@code HierarchyMode} will
 	 * be honored. See the Javadoc for {@link HierarchyMode} for details.
-	 *
 	 * @param key the context key; never {@code null}
 	 * @param hierarchyMode the hierarchy mode; may be {@code null} if the context
 	 * is not part of a hierarchy
 	 */
-	void remove(MergedContextConfiguration key, HierarchyMode hierarchyMode) {
+	public void remove(MergedContextConfiguration key, HierarchyMode hierarchyMode) {
 		Assert.notNull(key, "Key must not be null");
 
 		// startKey is the level at which to begin clearing the cache, depending
@@ -201,24 +181,21 @@ class ContextCache {
 			}
 		}
 
-		synchronized (monitor) {
-			final List<MergedContextConfiguration> removedContexts = new ArrayList<MergedContextConfiguration>();
-
+		List<MergedContextConfiguration> removedContexts = new ArrayList<MergedContextConfiguration>();
 		remove(removedContexts, startKey);
 
 		// Remove all remaining references to any removed contexts from the
 		// hierarchy map.
 		for (MergedContextConfiguration currentKey : removedContexts) {
-				for (Set<MergedContextConfiguration> children : hierarchyMap.values()) {
+			for (Set<MergedContextConfiguration> children : this.hierarchyMap.values()) {
 				children.remove(currentKey);
 			}
 		}
 
 		// Remove empty entries from the hierarchy map.
-			for (MergedContextConfiguration currentKey : hierarchyMap.keySet()) {
-				if (hierarchyMap.get(currentKey).isEmpty()) {
-					hierarchyMap.remove(currentKey);
-				}
+		for (MergedContextConfiguration currentKey : this.hierarchyMap.keySet()) {
+			if (this.hierarchyMap.get(currentKey).isEmpty()) {
+				this.hierarchyMap.remove(currentKey);
 			}
 		}
 	}
@@ -226,60 +203,53 @@ class ContextCache {
 	private void remove(List<MergedContextConfiguration> removedContexts, MergedContextConfiguration key) {
 		Assert.notNull(key, "Key must not be null");
 
-		synchronized (monitor) {
-			Set<MergedContextConfiguration> children = hierarchyMap.get(key);
+		Set<MergedContextConfiguration> children = this.hierarchyMap.get(key);
 		if (children != null) {
 			for (MergedContextConfiguration child : children) {
 				// Recurse through lower levels
 				remove(removedContexts, child);
 			}
-				// Remove the set of children for the current context from the
-				// hierarchy map.
-				hierarchyMap.remove(key);
+			// Remove the set of children for the current context from the hierarchy map.
+			this.hierarchyMap.remove(key);
 		}
 
 		// Physically remove and close leaf nodes first (i.e., on the way back up the
 		// stack as opposed to prior to the recursive call).
-			ApplicationContext context = contextMap.remove(key);
+		ApplicationContext context = this.contextMap.remove(key);
 		if (context instanceof ConfigurableApplicationContext) {
 			((ConfigurableApplicationContext) context).close();
 		}
 		removedContexts.add(key);
 	}
-	}
 
 	/**
 	 * Determine the number of contexts currently stored in the cache. If the cache
 	 * contains more than <tt>Integer.MAX_VALUE</tt> elements, returns
 	 * <tt>Integer.MAX_VALUE</tt>.
 	 */
-	int size() {
-		synchronized (monitor) {
+	public int size() {
 		return this.contextMap.size();
 	}
-	}
 
 	/**
 	 * Determine the number of parent contexts currently tracked within the cache.
 	 */
-	int getParentContextCount() {
-		synchronized (monitor) {
+	public int getParentContextCount() {
 		return this.hierarchyMap.size();
 	}
-	}
 
 	/**
 	 * Generates a text string, which contains the {@linkplain #size() size} as well
-	 * as the {@linkplain #getHitCount() hit}, {@linkplain #getMissCount() miss}, and
-	 * {@linkplain #getParentContextCount() parent context} counts.
+	 * as the {@linkplain #getHitCount() hit}, {@linkplain #getMissCount() miss},
+	 * and {@linkplain #getParentContextCount() parent context} counts.
 	 */
 	@Override
 	public String toString() {
-		return new ToStringCreator(this)//
-		.append("size", size())//
-		.append("hitCount", getHitCount())//
-		.append("missCount", getMissCount())//
-		.append("parentContextCount", getParentContextCount())//
+		return new ToStringCreator(this)
+				.append("size", size())
+				.append("hitCount", getHitCount())
+				.append("missCount", getMissCount())
+				.append("parentContextCount", getParentContextCount())
 				.toString();
 	}
 

spring-test/src/main/java/org/springframework/test/context/DefaultCacheAwareContextLoaderDelegate.java

@@ -48,6 +48,7 @@ class DefaultCacheAwareContextLoaderDelegate implements CacheAwareContextLoaderD
 		this.contextCache = contextCache;
 	}
 
+
 	/**
 	 * Load the {@code ApplicationContext} for the supplied merged context configuration.
 	 * <p>Supports both the {@link SmartContextLoader} and {@link ContextLoader} SPIs.
@@ -55,9 +56,10 @@ class DefaultCacheAwareContextLoaderDelegate implements CacheAwareContextLoaderD
 	 */
 	private ApplicationContext loadContextInternal(MergedContextConfiguration mergedContextConfiguration)
 			throws Exception {
+
 		ContextLoader contextLoader = mergedContextConfiguration.getContextLoader();
-		Assert.notNull(contextLoader, "Cannot load an ApplicationContext with a NULL 'contextLoader'. "
-				+ "Consider annotating your test class with @ContextConfiguration or @ContextHierarchy.");
+		Assert.notNull(contextLoader, "Cannot load an ApplicationContext with a NULL 'contextLoader'. " +
+				"Consider annotating your test class with @ContextConfiguration or @ContextHierarchy.");
 
 		ApplicationContext applicationContext;
 
@@ -67,28 +69,26 @@ class DefaultCacheAwareContextLoaderDelegate implements CacheAwareContextLoaderD
 		}
 		else {
 			String[] locations = mergedContextConfiguration.getLocations();
-			Assert.notNull(locations, "Cannot load an ApplicationContext with a NULL 'locations' array. "
-					+ "Consider annotating your test class with @ContextConfiguration or @ContextHierarchy.");
+			Assert.notNull(locations, "Cannot load an ApplicationContext with a NULL 'locations' array. " +
+					"Consider annotating your test class with @ContextConfiguration or @ContextHierarchy.");
 			applicationContext = contextLoader.loadContext(locations);
 		}
 
 		return applicationContext;
 	}
 
-	/**
-	 * {@inheritDoc}
-	 */
+	@Override
 	public ApplicationContext loadContext(MergedContextConfiguration mergedContextConfiguration) {
-		synchronized (contextCache) {
-			ApplicationContext context = contextCache.get(mergedContextConfiguration);
+		synchronized (this.contextCache) {
+			ApplicationContext context = this.contextCache.get(mergedContextConfiguration);
 			if (context == null) {
 				try {
 					context = loadContextInternal(mergedContextConfiguration);
 					if (logger.isDebugEnabled()) {
-						logger.debug(String.format("Storing ApplicationContext in cache under key [%s].",
+						logger.debug(String.format("Storing ApplicationContext in cache under key [%s]",
 								mergedContextConfiguration));
 					}
-					contextCache.put(mergedContextConfiguration, context);
+					this.contextCache.put(mergedContextConfiguration, context);
 				}
 				catch (Exception ex) {
 					throw new IllegalStateException("Failed to load ApplicationContext", ex);
@@ -96,25 +96,24 @@ class DefaultCacheAwareContextLoaderDelegate implements CacheAwareContextLoaderD
 			}
 			else {
 				if (logger.isDebugEnabled()) {
-					logger.debug(String.format("Retrieved ApplicationContext from cache with key [%s].",
+					logger.debug(String.format("Retrieved ApplicationContext from cache with key [%s]",
 							mergedContextConfiguration));
 				}
 			}
 
 			if (statsLogger.isDebugEnabled()) {
-				statsLogger.debug(String.format("Spring test ApplicationContext cache statistics: %s", contextCache));
+				statsLogger.debug("Spring test ApplicationContext cache statistics: " + this.contextCache);
 			}
 
 			return context;
 		}
 	}
 
-	/**
-	 * {@inheritDoc}
-	 */
 	@Override
 	public void closeContext(MergedContextConfiguration mergedContextConfiguration, HierarchyMode hierarchyMode) {
-		contextCache.remove(mergedContextConfiguration, hierarchyMode);
+		synchronized (this.contextCache) {
+			this.contextCache.remove(mergedContextConfiguration, hierarchyMode);
+		}
 	}
 
 }

spring-test/src/main/java/org/springframework/test/context/DefaultTestContext.java

@@ -67,68 +67,49 @@ class DefaultTestContext extends AttributeAccessorSupport implements TestContext
 		this.mergedContextConfiguration = testContextBootstrapper.buildMergedContextConfiguration();
 	}
 
-	/**
-	 * {@inheritDoc}
-	 */
+
 	public ApplicationContext getApplicationContext() {
-		return cacheAwareContextLoaderDelegate.loadContext(mergedContextConfiguration);
+		return this.cacheAwareContextLoaderDelegate.loadContext(this.mergedContextConfiguration);
 	}
 
-	/**
-	 * {@inheritDoc}
-	 */
 	public void markApplicationContextDirty(HierarchyMode hierarchyMode) {
-		cacheAwareContextLoaderDelegate.closeContext(mergedContextConfiguration, hierarchyMode);
+		this.cacheAwareContextLoaderDelegate.closeContext(this.mergedContextConfiguration, hierarchyMode);
 	}
 
-	/**
-	 * {@inheritDoc}
-	 */
 	public final Class<?> getTestClass() {
-		return testClass;
+		return this.testClass;
 	}
 
-	/**
-	 * {@inheritDoc}
-	 */
 	public final Object getTestInstance() {
-		return testInstance;
+		return this.testInstance;
 	}
 
-	/**
-	 * {@inheritDoc}
-	 */
 	public final Method getTestMethod() {
-		return testMethod;
+		return this.testMethod;
 	}
 
-	/**
-	 * {@inheritDoc}
-	 */
 	public final Throwable getTestException() {
-		return testException;
+		return this.testException;
 	}
 
-	/**
-	 * {@inheritDoc}
-	 */
 	public void updateState(Object testInstance, Method testMethod, Throwable testException) {
 		this.testInstance = testInstance;
 		this.testMethod = testMethod;
 		this.testException = testException;
 	}
 
+
 	/**
 	 * Provide a String representation of this test context's state.
 	 */
 	@Override
 	public String toString() {
-		return new ToStringCreator(this)//
-		.append("testClass", testClass)//
-		.append("testInstance", testInstance)//
-		.append("testMethod", testMethod)//
-		.append("testException", testException)//
-		.append("mergedContextConfiguration", mergedContextConfiguration)//
+		return new ToStringCreator(this)
+				.append("testClass", this.testClass)
+				.append("testInstance", this.testInstance)
+				.append("testMethod", this.testMethod)
+				.append("testException", this.testException)
+				.append("mergedContextConfiguration", this.mergedContextConfiguration)
 				.toString();
 	}
 

spring-test/src/main/java/org/springframework/test/web/client/MockRestServiceServer.java

@@ -84,9 +84,11 @@ import org.springframework.web.client.support.RestGatewaySupport;
  */
 public class MockRestServiceServer {
 
-	private final List<RequestMatcherClientHttpRequest> expectedRequests = new LinkedList<RequestMatcherClientHttpRequest>();
+	private final List<RequestMatcherClientHttpRequest> expectedRequests =
+			new LinkedList<RequestMatcherClientHttpRequest>();
 
-	private final List<RequestMatcherClientHttpRequest> actualRequests = new LinkedList<RequestMatcherClientHttpRequest>();
+	private final List<RequestMatcherClientHttpRequest> actualRequests =
+			new LinkedList<RequestMatcherClientHttpRequest>();
 
 
 	/**
@@ -97,10 +99,10 @@ public class MockRestServiceServer {
 	private MockRestServiceServer() {
 	}
 
+
 	/**
 	 * Create a {@code MockRestServiceServer} and set up the given
 	 * {@code RestTemplate} with a mock {@link ClientHttpRequestFactory}.
-	 *
 	 * @param restTemplate the RestTemplate to set up for mock testing
 	 * @return the created mock server
 	 */
@@ -115,7 +117,6 @@ public class MockRestServiceServer {
 	/**
 	 * Create a {@code MockRestServiceServer} and set up the given
 	 * {@code AsyRestTemplate} with a mock {@link AsyncClientHttpRequestFactory}.
-	 *
 	 * @param asyncRestTemplate the AsyncRestTemplate to set up for mock testing
 	 * @return the created mock server
 	 */
@@ -130,7 +131,6 @@ public class MockRestServiceServer {
 	/**
 	 * Create a {@code MockRestServiceServer} and set up the given
 	 * {@code RestGatewaySupport} with a mock {@link ClientHttpRequestFactory}.
-	 *
 	 * @param restGateway the REST gateway to set up for mock testing
 	 * @return the created mock server
 	 */
@@ -139,14 +139,12 @@ public class MockRestServiceServer {
 		return createServer(restGateway.getRestTemplate());
 	}
 
+
 	/**
 	 * Set up a new HTTP request expectation. The returned {@link ResponseActions}
 	 * is used to set up further expectations and to define the response.
-	 *
-	 * <p>This method may be invoked multiple times before starting the test, i.e.
-	 * before using the {@code RestTemplate}, to set up expectations for multiple
-	 * requests.
-	 *
+	 * <p>This method may be invoked multiple times before starting the test, i.e. before
+	 * using the {@code RestTemplate}, to set up expectations for multiple requests.
 	 * @param requestMatcher a request expectation, see {@link MockRestRequestMatchers}
 	 * @return used to set up further expectations or to define a response
 	 */
@@ -160,7 +158,6 @@ public class MockRestServiceServer {
 	/**
 	 * Verify that all expected requests set up via
 	 * {@link #expect(RequestMatcher)} were indeed performed.
-	 *
 	 * @throws AssertionError when some expectations were not met
 	 */
 	public void verify() {
@@ -172,7 +169,6 @@ public class MockRestServiceServer {
 
 	private String getVerifyMessage() {
 		StringBuilder sb = new StringBuilder("Further request(s) expected\n");
-
 		if (this.actualRequests.size() > 0) {
 			sb.append("The following ");
 		}

spring-test/src/test/java/org/springframework/test/context/TestContextTestUtils.java

@@ -24,21 +24,15 @@ package org.springframework.test.context;
  */
 public abstract class TestContextTestUtils {
 
-	private TestContextTestUtils() {
-		/* no-op */
-	}
-
 	public static TestContext buildTestContext(Class<?> testClass, ContextCache contextCache) {
-		CacheAwareContextLoaderDelegate cacheAwareContextLoaderDelegate = new DefaultCacheAwareContextLoaderDelegate(
-			contextCache);
-		return buildTestContext(testClass, null, cacheAwareContextLoaderDelegate);
+		return buildTestContext(testClass, new DefaultCacheAwareContextLoaderDelegate(contextCache));
 	}
 
-	public static TestContext buildTestContext(Class<?> testClass, String customDefaultContextLoaderClassName,
-			CacheAwareContextLoaderDelegate cacheAwareContextLoaderDelegate) {
+	public static TestContext buildTestContext(
+			Class<?> testClass, CacheAwareContextLoaderDelegate cacheAwareContextLoaderDelegate) {
+
 		BootstrapContext bootstrapContext = new DefaultBootstrapContext(testClass, cacheAwareContextLoaderDelegate);
 		TestContextBootstrapper testContextBootstrapper = BootstrapUtils.resolveTestContextBootstrapper(bootstrapContext);
-
 		return new DefaultTestContext(testContextBootstrapper);
 	}
 

spring-test/src/test/java/org/springframework/test/web/client/MockClientHttpRequestFactoryTests.java

@@ -13,6 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 package org.springframework.test.web.client;
 
 import java.net.URI;