Revised AbstractCacheManager for consistent locking when caches get added

Issue: SPR-13492

spring-context/src/main/java/org/springframework/cache/support/AbstractCacheManager.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 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.
@@ -40,20 +40,36 @@ public abstract class AbstractCacheManager implements CacheManager, Initializing
 
 	private final ConcurrentMap<String, Cache> cacheMap = new ConcurrentHashMap<String, Cache>(16);
 
-	private Set<String> cacheNames = new LinkedHashSet<String>(16);
+	private volatile Set<String> cacheNames = Collections.emptySet();
 
 
 	// Early cache initialization on startup
 
 	@Override
 	public void afterPropertiesSet() {
+		initializeCaches();
+	}
+
+	/**
+	 * Initialize the static configuration of caches.
+	 * <p>Triggered on startup through {@link #afterPropertiesSet()};
+	 * can also be called to re-initialize at runtime.
+	 * @since 4.2.2
+	 * @see #loadCaches()
+	 */
+	public void initializeCaches() {
 		Collection<? extends Cache> caches = loadCaches();
 
-		// Preserve the initial order of the cache names
-		this.cacheMap.clear();
-		this.cacheNames.clear();
-		for (Cache cache : caches) {
-			addCache(cache);
+		synchronized (this.cacheMap) {
+			this.cacheNames = Collections.emptySet();
+			this.cacheMap.clear();
+			Set<String> cacheNames = new LinkedHashSet<String>(caches.size());
+			for (Cache cache : caches) {
+				String name = cache.getName();
+				this.cacheMap.put(name, decorateCache(cache));
+				cacheNames.add(name);
+			}
+			this.cacheNames = Collections.unmodifiableSet(cacheNames);
 		}
 	}
 
@@ -69,37 +85,79 @@ public abstract class AbstractCacheManager implements CacheManager, Initializing
 
 	@Override
 	public Cache getCache(String name) {
-		Cache cache = lookupCache(name);
+		Cache cache = this.cacheMap.get(name);
 		if (cache != null) {
 			return cache;
 		}
 		else {
-			Cache missingCache = getMissingCache(name);
-			if (missingCache != null) {
-				addCache(missingCache);
-				return lookupCache(name);  // may be decorated
+			// Fully synchronize now for missing cache creation...
+			synchronized (this.cacheMap) {
+				cache = this.cacheMap.get(name);
+				if (cache == null) {
+					cache = getMissingCache(name);
+					if (cache != null) {
+						cache = decorateCache(cache);
+						this.cacheMap.put(name, cache);
+						updateCacheNames(name);
+					}
+				}
+				return cache;
 			}
-			return null;
 		}
 	}
 
 	@Override
 	public Collection<String> getCacheNames() {
-		return Collections.unmodifiableSet(this.cacheNames);
+		return this.cacheNames;
 	}
 
 
-	// Common cache initialization delegates/callbacks
-
-	protected final void addCache(Cache cache) {
-		this.cacheMap.put(cache.getName(), decorateCache(cache));
-		this.cacheNames.add(cache.getName());
-	}
+	// Common cache initialization delegates for subclasses
 
+	/**
+	 * Check for a registered cache of the given name.
+	 * In contrast to {@link #getCache(String)}, this method does not trigger
+	 * the lazy creation of missing caches via {@link #getMissingCache(String)}.
+	 * @param name the cache identifier (must not be {@code null})
+	 * @return the associated Cache instance, or {@code null} if none found
+	 * @since 4.1
+	 * @see #getCache(String)
+	 * @see #getMissingCache(String)
+	 */
 	protected final Cache lookupCache(String name) {
 		return this.cacheMap.get(name);
 	}
 
+	/**
+	 * Dynamically register an additional Cache with this manager.
+	 * @param cache the Cache to register
+	 */
+	protected final void addCache(Cache cache) {
+		String name = cache.getName();
+		synchronized (this.cacheMap) {
+			if (this.cacheMap.put(name, decorateCache(cache)) == null) {
+				updateCacheNames(name);
+			}
+		}
+	}
+
+	/**
+	 * Update the exposed {@link #cacheNames} set with the given name.
+	 * <p>This will always be called within a full {@link #cacheMap} lock
+	 * and effectively behaves like a {@code CopyOnWriteArraySet} with
+	 * preserved order but exposed as an unmodifiable reference.
+	 * @param name the name of the cache to be added
+	 */
+	private void updateCacheNames(String name) {
+		Set<String> cacheNames = new LinkedHashSet<String>(this.cacheNames.size() + 1);
+		cacheNames.addAll(this.cacheNames);
+		cacheNames.add(name);
+		this.cacheNames = Collections.unmodifiableSet(cacheNames);
+	}
+
+
+	// Overridable template methods for cache initialization
+
 	/**
 	 * Decorate the given Cache object if necessary.
 	 * @param cache the Cache object to be added to this CacheManager
@@ -120,6 +178,7 @@ public abstract class AbstractCacheManager implements CacheManager, Initializing
 	 * @param name the name of the cache to retrieve
 	 * @return the missing cache or {@code null} if no such cache exists or could be
 	 * created
+	 * @since 4.1
 	 * @see #getCache(String)
 	 */
 	protected Cache getMissingCache(String name) {