From 04840166af660a4aef1f7a1680403d4ff4309ca0 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Mon, 2 Feb 2015 13:52:12 -0500 Subject: [PATCH] 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 --- .../broker/DefaultSubscriptionRegistry.java | 7 ++++--- .../simp/broker/SubscriptionRegistry.java | 8 ++++--- .../DefaultSubscriptionRegistryTests.java | 21 ++++++++++++++++++- 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/spring-messaging/src/main/java/org/springframework/messaging/simp/broker/DefaultSubscriptionRegistry.java b/spring-messaging/src/main/java/org/springframework/messaging/simp/broker/DefaultSubscriptionRegistry.java index f4879ea472..613b2c6456 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/simp/broker/DefaultSubscriptionRegistry.java +++ b/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.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.util.PathMatcher; + /** * A default, simple in-memory implementation of {@link SubscriptionRegistry}. * @@ -165,8 +166,8 @@ public class DefaultSubscriptionRegistry extends AbstractSubscriptionRegistry { public void addSubscriptions(String destination, MultiValueMap subscriptions) { synchronized (this.updateCache) { - this.updateCache.put(destination, subscriptions); - this.accessCache.put(destination, new LinkedMultiValueMap(subscriptions)); + this.updateCache.put(destination, new LinkedMultiValueMap(subscriptions)); + this.accessCache.put(destination, subscriptions); } } diff --git a/spring-messaging/src/main/java/org/springframework/messaging/simp/broker/SubscriptionRegistry.java b/spring-messaging/src/main/java/org/springframework/messaging/simp/broker/SubscriptionRegistry.java index 396907c6e7..ac9dc15e54 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/simp/broker/SubscriptionRegistry.java +++ b/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 findSubscriptions(Message message); diff --git a/spring-messaging/src/test/java/org/springframework/messaging/simp/broker/DefaultSubscriptionRegistryTests.java b/spring-messaging/src/test/java/org/springframework/messaging/simp/broker/DefaultSubscriptionRegistryTests.java index 248749663d..d14a158846 100644 --- a/spring-messaging/src/test/java/org/springframework/messaging/simp/broker/DefaultSubscriptionRegistryTests.java +++ b/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 @@ package org.springframework.messaging.simp.broker; 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 org.springframework.util.MultiValueMap; import static org.junit.Assert.*; + /** * Test fixture for {@link org.springframework.messaging.simp.broker.DefaultSubscriptionRegistry}. * @@ -326,6 +328,23 @@ public class DefaultSubscriptionRegistryTests { 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 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); -- GitLab