From 74500ec8da5244716908ba1a6c5fc4d5baa7603b Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 6 Nov 2014 17:15:30 +0100 Subject: [PATCH] Avoid unnecessary synchronization in ContextCache, plus forward-ported polishing Issue: SPR-12409 --- .../CacheAwareContextLoaderDelegate.java | 9 +- .../test/context/ContextCache.java | 190 ++++++++---------- ...efaultCacheAwareContextLoaderDelegate.java | 37 ++-- .../test/context/DefaultTestContext.java | 49 ++--- .../web/client/MockRestServiceServer.java | 22 +- .../test/context/TestContextTestUtils.java | 14 +- .../MockClientHttpRequestFactoryTests.java | 1 + 7 files changed, 129 insertions(+), 193 deletions(-) diff --git a/spring-test/src/main/java/org/springframework/test/context/CacheAwareContextLoaderDelegate.java b/spring-test/src/main/java/org/springframework/test/context/CacheAwareContextLoaderDelegate.java index 958b210820a..1a59940476b 100644 --- a/spring-test/src/main/java/org/springframework/test/context/CacheAwareContextLoaderDelegate.java +++ b/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}. - * *

If the context is present in the context cache 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 context cache * and {@linkplain ConfigurableApplicationContext#close() close} it if it is * an instance of {@link ConfigurableApplicationContext}. - * *

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. - * *

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 diff --git a/spring-test/src/main/java/org/springframework/test/context/ContextCache.java b/spring-test/src/main/java/org/springframework/test/context/ContextCache.java index 77e482ef6f3..576021bdaa5 100644 --- a/spring-test/src/main/java/org/springframework/test/context/ContextCache.java +++ b/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 contextMap = new ConcurrentHashMap( - 64); + private final Map contextMap = + new ConcurrentHashMap(64); /** * Map of parent keys to sets of children keys, representing a top-down tree @@ -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> hierarchyMap = new ConcurrentHashMap>( - 64); + private final Map> hierarchyMap = + new ConcurrentHashMap>(64); private final AtomicInteger hitCount = new AtomicInteger(); @@ -71,67 +69,57 @@ 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) { - this.contextMap.clear(); - this.hierarchyMap.clear(); - } + 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); - } + return this.contextMap.containsKey(key); } /** * Obtain a cached {@code ApplicationContext} for the given key. - * - *

The {@link #getHitCount() hit} and {@link #getMissCount() miss} counts will be - * updated accordingly. - * + *

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(); - } - else { - this.hitCount.incrementAndGet(); - } - return context; + ApplicationContext context = this.contextMap.get(key); + if (context == null) { + this.missCount.incrementAndGet(); + } + else { + this.hitCount.incrementAndGet(); } + return context; } /** * Get the overall hit count for this cache. - *

A hit is an access to the cache, which returned a non-null context for - * a queried key. + *

A hit 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,36 +128,31 @@ class ContextCache { *

A miss 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 list = hierarchyMap.get(parent); - if (list == null) { - list = new HashSet(); - hierarchyMap.put(parent, list); - } - list.add(child); - child = parent; - parent = child.getParent(); + this.contextMap.put(key, context); + MergedContextConfiguration child = key; + MergedContextConfiguration parent = child.getParent(); + while (parent != null) { + Set list = this.hierarchyMap.get(parent); + if (list == null) { + list = new HashSet(); + this.hierarchyMap.put(parent, list); } + list.add(child); + child = parent; + parent = child.getParent(); } } @@ -177,19 +160,16 @@ class ContextCache { * 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}. - * *

Generally speaking, you would only call this method if you change the * state of a singleton bean, potentially affecting future interaction with * the context. - * *

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 removedContexts = new ArrayList(); - - remove(removedContexts, startKey); + List removedContexts = new ArrayList(); + remove(removedContexts, startKey); - // Remove all remaining references to any removed contexts from the - // hierarchy map. - for (MergedContextConfiguration currentKey : removedContexts) { - for (Set children : hierarchyMap.values()) { - children.remove(currentKey); - } + // Remove all remaining references to any removed contexts from the + // hierarchy map. + for (MergedContextConfiguration currentKey : removedContexts) { + for (Set 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); - } + // Remove empty entries from the hierarchy map. + for (MergedContextConfiguration currentKey : this.hierarchyMap.keySet()) { + if (this.hierarchyMap.get(currentKey).isEmpty()) { + this.hierarchyMap.remove(currentKey); } } } @@ -226,26 +203,23 @@ class ContextCache { private void remove(List removedContexts, MergedContextConfiguration key) { Assert.notNull(key, "Key must not be null"); - synchronized (monitor) { - Set children = 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); + Set 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. + 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); - if (context instanceof ConfigurableApplicationContext) { - ((ConfigurableApplicationContext) context).close(); - } - removedContexts.add(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 = this.contextMap.remove(key); + if (context instanceof ConfigurableApplicationContext) { + ((ConfigurableApplicationContext) context).close(); } + removedContexts.add(key); } /** @@ -253,34 +227,30 @@ class ContextCache { * contains more than Integer.MAX_VALUE elements, returns * Integer.MAX_VALUE. */ - int size() { - synchronized (monitor) { - return this.contextMap.size(); - } + public int size() { + return this.contextMap.size(); } /** * Determine the number of parent contexts currently tracked within the cache. */ - int getParentContextCount() { - synchronized (monitor) { - return this.hierarchyMap.size(); - } + 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())// - .toString(); + return new ToStringCreator(this) + .append("size", size()) + .append("hitCount", getHitCount()) + .append("missCount", getMissCount()) + .append("parentContextCount", getParentContextCount()) + .toString(); } } diff --git a/spring-test/src/main/java/org/springframework/test/context/DefaultCacheAwareContextLoaderDelegate.java b/spring-test/src/main/java/org/springframework/test/context/DefaultCacheAwareContextLoaderDelegate.java index 26cdb28776f..a9d38dfa08d 100644 --- a/spring-test/src/main/java/org/springframework/test/context/DefaultCacheAwareContextLoaderDelegate.java +++ b/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. *

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].", - mergedContextConfiguration)); + 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].", - mergedContextConfiguration)); + 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); + } } } diff --git a/spring-test/src/main/java/org/springframework/test/context/DefaultTestContext.java b/spring-test/src/main/java/org/springframework/test/context/DefaultTestContext.java index b3fcfc13826..eac358e8e52 100644 --- a/spring-test/src/main/java/org/springframework/test/context/DefaultTestContext.java +++ b/spring-test/src/main/java/org/springframework/test/context/DefaultTestContext.java @@ -67,69 +67,50 @@ 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)// - .toString(); + 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(); } } diff --git a/spring-test/src/main/java/org/springframework/test/web/client/MockRestServiceServer.java b/spring-test/src/main/java/org/springframework/test/web/client/MockRestServiceServer.java index ae7ab5cb6de..31a347f2e0a 100644 --- a/spring-test/src/main/java/org/springframework/test/web/client/MockRestServiceServer.java +++ b/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 expectedRequests = new LinkedList(); + private final List expectedRequests = + new LinkedList(); - private final List actualRequests = new LinkedList(); + private final List actualRequests = + new LinkedList(); /** @@ -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. - * - *

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. - * + *

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 "); } @@ -228,4 +224,4 @@ public class MockRestServiceServer { } } -} \ No newline at end of file +} diff --git a/spring-test/src/test/java/org/springframework/test/context/TestContextTestUtils.java b/spring-test/src/test/java/org/springframework/test/context/TestContextTestUtils.java index f5e412d1e82..3f1eb86edfb 100644 --- a/spring-test/src/test/java/org/springframework/test/context/TestContextTestUtils.java +++ b/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); } diff --git a/spring-test/src/test/java/org/springframework/test/web/client/MockClientHttpRequestFactoryTests.java b/spring-test/src/test/java/org/springframework/test/web/client/MockClientHttpRequestFactoryTests.java index 230c7ce263c..dfd3a7147e2 100644 --- a/spring-test/src/test/java/org/springframework/test/web/client/MockClientHttpRequestFactoryTests.java +++ b/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;