Avoid ConcurrentModificationException

Removal of cached destination is now moved outside the for loop
that removes subscriptions to avoid ConcurrentModificationException.

Also since updateCache is a LinkedHashMap with accessOrder=true,
a simple access with updateCache.get() modify the map.
By iterating over updateCache.entrySet(), we avoid this update.

Issue: SPR-11755

spring-messaging/src/main/java/org/springframework/messaging/simp/broker/DefaultSubscriptionRegistry.java

@@ -55,7 +55,6 @@ public class DefaultSubscriptionRegistry extends AbstractSubscriptionRegistry {
 	private PathMatcher pathMatcher = new AntPathMatcher();
 
 
-
 	/**
 	 * Specify the maximum number of entries for the resolved destination cache.
 	 * Default is 1024.
@@ -176,9 +175,10 @@ public class DefaultSubscriptionRegistry extends AbstractSubscriptionRegistry {
 
 		public void updateAfterNewSubscription(String destination, String sessionId, String subsId) {
 			synchronized (this.updateCache) {
-				for (String cachedDestination : this.updateCache.keySet()) {
+				for (Map.Entry<String, MultiValueMap<String, String>> entry : this.updateCache.entrySet()) {
+					String cachedDestination = entry.getKey();
 					if (getPathMatcher().match(destination, cachedDestination)) {
-						MultiValueMap<String, String> subs = this.updateCache.get(cachedDestination);
+						MultiValueMap<String, String> subs = entry.getValue();
 						subs.add(sessionId, subsId);
 						this.accessCache.put(cachedDestination, new LinkedMultiValueMap<String, String>(subs));
 					}
@@ -188,42 +188,52 @@ public class DefaultSubscriptionRegistry extends AbstractSubscriptionRegistry {
 
 		public void updateAfterRemovedSubscription(String destination, String sessionId, String subsId) {
 			synchronized (this.updateCache) {
-				for (String cachedDestination : this.updateCache.keySet()) {
+				Set<String> destinationsToRemove = new HashSet<String>();
+				for (Map.Entry<String, MultiValueMap<String, String>> entry : this.updateCache.entrySet()) {
+					String cachedDestination = entry.getKey();
 					if (getPathMatcher().match(destination, cachedDestination)) {
-						MultiValueMap<String, String> subs = this.updateCache.get(cachedDestination);
+						MultiValueMap<String, String> subs = entry.getValue();
 						List<String> subsIds = subs.get(sessionId);
 						subsIds.remove(subsId);
 						if (subsIds.isEmpty()) {
 							subs.remove(sessionId);
 						}
 						if (subs.isEmpty()) {
-							this.updateCache.remove(cachedDestination);
-							this.accessCache.remove(cachedDestination);
+							destinationsToRemove.add(cachedDestination);
 						}
 						else {
 							this.accessCache.put(cachedDestination, new LinkedMultiValueMap<String, String>(subs));
 						}
 					}
 				}
+				for (String d : destinationsToRemove) {
+					this.updateCache.remove(d);
+					this.accessCache.remove(d);
+				}
 			}
 		}
 
 		public void updateAfterRemovedSession(SessionSubscriptionInfo info) {
 			synchronized (this.updateCache) {
 				for (String destination : info.getDestinations()) {
-					for (String cachedDestination : this.updateCache.keySet()) {
+					Set<String> destinationsToRemove = new HashSet<String>();
+					for (Map.Entry<String, MultiValueMap<String, String>> entry : this.updateCache.entrySet()) {
+						String cachedDestination = entry.getKey();
 						if (getPathMatcher().match(destination, cachedDestination)) {
-							MultiValueMap<String, String> subs = this.updateCache.get(cachedDestination);
+							MultiValueMap<String, String> subs = entry.getValue();
 							subs.remove(info.getSessionId());
 							if (subs.isEmpty()) {
-								this.updateCache.remove(cachedDestination);
-								this.accessCache.remove(cachedDestination);
+								destinationsToRemove.add(cachedDestination);
 							}
 							else {
 								this.accessCache.put(cachedDestination,new LinkedMultiValueMap<String, String>(subs));
 							}
 						}
 					}
+					for (String d : destinationsToRemove) {
+						this.updateCache.remove(d);
+						this.accessCache.remove(d);
+					}
 				}
 			}
 		}

spring-messaging/src/test/java/org/springframework/messaging/simp/broker/DefaultSubscriptionRegistryTests.java

@@ -185,6 +185,41 @@ public class DefaultSubscriptionRegistryTests {
 		assertEquals(0, actual.size());
 	}
 
+	// SPR-11755
+
+	@Test
+	public void registerAndUnregisterMultipleDestinations() {
+
+		String sess1 = "sess01";
+		String sess2 = "sess02";
+
+		String subs1 = "subs01";
+		String subs2 = "subs02";
+		String subs3 = "subs03";
+		String subs4 = "subs04";
+		String subs5 = "subs05";
+
+		this.registry.registerSubscription(subscribeMessage(sess1, subs1, "/topic/PRICE.STOCK.NASDAQ.IBM"));
+		this.registry.registerSubscription(subscribeMessage(sess1, subs2, "/topic/PRICE.STOCK.NYSE.IBM"));
+		this.registry.registerSubscription(subscribeMessage(sess1, subs3, "/topic/PRICE.STOCK.NASDAQ.GOOG"));
+
+		this.registry.findSubscriptions(message("/topic/PRICE.STOCK.NYSE.IBM"));
+		this.registry.findSubscriptions(message("/topic/PRICE.STOCK.NASDAQ.GOOG"));
+		this.registry.findSubscriptions(message("/topic/PRICE.STOCK.NASDAQ.IBM"));
+
+		this.registry.unregisterSubscription(unsubscribeMessage(sess1, subs1));
+		this.registry.unregisterSubscription(unsubscribeMessage(sess1, subs2));
+		this.registry.unregisterSubscription(unsubscribeMessage(sess1, subs3));
+
+		this.registry.registerSubscription(subscribeMessage(sess1, subs1, "/topic/PRICE.STOCK.NASDAQ.IBM"));
+		this.registry.registerSubscription(subscribeMessage(sess1, subs2, "/topic/PRICE.STOCK.NYSE.IBM"));
+		this.registry.registerSubscription(subscribeMessage(sess1, subs3, "/topic/PRICE.STOCK.NASDAQ.GOOG"));
+		this.registry.registerSubscription(subscribeMessage(sess1, subs4, "/topic/PRICE.STOCK.NYSE.IBM"));
+		this.registry.registerSubscription(subscribeMessage(sess2, subs5, "/topic/PRICE.STOCK.NASDAQ.GOOG"));
+		this.registry.unregisterAllSubscriptions(sess1);
+		this.registry.unregisterAllSubscriptions(sess2);
+	}
+
 	@Test
 	public void registerSubscriptionWithDestinationPatternRegex() {