DefaultSubscriptionRegistry returns safe to iterate Map

Prior to this change when adding subscriptions
DefaultSubscriptionRegistry (incorrectly) made a copy of the given map
for its "access" cache rather than for its "update" cache.

Issue: SPR-12665

Author: rstoyanchev

spring-messaging/src/main/java/org/springframework/messaging/simp/broker/DefaultSubscriptionRegistry.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.
@@ -32,6 +32,7 @@
 import org.springframework.util.MultiValueMap;
 import org.springframework.util.PathMatcher;
 
+
 /**
  * A default, simple in-memory implementation of {@link SubscriptionRegistry}.
  *
@@ -165,8 +166,8 @@ public MultiValueMap<String, String> getSubscriptions(String destination) {
 
 		public void addSubscriptions(String destination, MultiValueMap<String, String> subscriptions) {
 			synchronized (this.updateCache) {
-				this.updateCache.put(destination, subscriptions);
-				this.accessCache.put(destination, new LinkedMultiValueMap<String, String>(subscriptions));
+				this.updateCache.put(destination, new LinkedMultiValueMap<String, String>(subscriptions));
+				this.accessCache.put(destination, subscriptions);
 			}
 		}
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 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.
@@ -45,9 +45,11 @@ public interface SubscriptionRegistry {
 	void unregisterAllSubscriptions(String sessionId);
 
 	/**
-	 * Find all subscriptions that should receive the given message.
+	 * Find all subscriptions that should receive the given message. The map
+	 * returned is safe to iterate and will never be modified.
 	 * @param message the message
-	 * @return a {@link MultiValueMap} from sessionId to subscriptionId's, possibly empty.
+	 * @return a {@code MultiValueMap} with sessionId-subscriptionId pairs,
+	 * possibly empty.
 	 */
 	MultiValueMap<String, String> findSubscriptions(Message<?> message);
 

spring-messaging/src/test/java/org/springframework/messaging/simp/broker/DefaultSubscriptionRegistryTests.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.
@@ -18,6 +18,7 @@
 
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.List;
 
 import org.junit.Before;
@@ -31,6 +32,7 @@
 
 import static org.junit.Assert.*;
 
+
 /**
  * Test fixture for {@link org.springframework.messaging.simp.broker.DefaultSubscriptionRegistry}.
  *
@@ -326,6 +328,23 @@ public void findSubscriptionsNoMatches() {
 		assertEquals("Expected no elements " + actual, 0, actual.size());
 	}
 
+	// SPR-12665
+
+	@Test
+	public void findSubscriptionsReturnsMapSafeToIterate() throws Exception {
+		this.registry.registerSubscription(subscribeMessage("sess1", "1", "/foo"));
+		this.registry.registerSubscription(subscribeMessage("sess2", "1", "/foo"));
+		MultiValueMap<String, String> subscriptions = this.registry.findSubscriptions(message("/foo"));
+		assertEquals(2, subscriptions.size());
+
+		Iterator iterator = subscriptions.entrySet().iterator();
+		iterator.next();
+
+		this.registry.registerSubscription(subscribeMessage("sess3", "1", "/foo"));
+
+		iterator.next();
+		// no ConcurrentModificationException
+	}
 
 	private Message<?> subscribeMessage(String sessionId, String subscriptionId, String destination) {
 		SimpMessageHeaderAccessor headers = SimpMessageHeaderAccessor.create(SimpMessageType.SUBSCRIBE);