Skip to content

Commit 85b3cf4

Browse files
nobodyiamnobodyiam
committed
fix: deduplicate config listeners by identity
Fixes #88
1 parent df7aef2 commit 85b3cf4

File tree

3 files changed

+156
-5
lines changed

3 files changed

+156
-5
lines changed

‎CHANGES.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ Apollo Java 2.5.0
1010
* [Feature Added a new feature to get instance count by namespace.](https://github.com/apolloconfig/apollo-java/pull/103)
1111
* [Feature Support retry in open api client.](https://github.com/apolloconfig/apollo-java/pull/105)
1212
* [Support Spring Boot 4.0 bootstrap context package relocation for apollo-client-config-data](https://github.com/apolloconfig/apollo-java/pull/115)
13+
* [Fix change listener de-duplication by identity to avoid stale property names cache in Spring Cloud bootstrap dual-context initialization](https://github.com/apolloconfig/apollo-java/pull/121)
1314

1415
------------------
1516
All issues and pull requests are [here](https://github.com/apolloconfig/apollo-java/milestone/5?closed=1)

‎apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,10 @@ public abstract class AbstractConfig implements Config {
5656
protected static final ExecutorService m_executorService;
5757

5858
private final List<ConfigChangeListener> m_listeners = Lists.newCopyOnWriteArrayList();
59-
private final Map<ConfigChangeListener, Set<String>> m_interestedKeys = Maps.newConcurrentMap();
60-
private final Map<ConfigChangeListener, Set<String>> m_interestedKeyPrefixes = Maps.newConcurrentMap();
59+
private final Map<ConfigChangeListener, Set<String>> m_interestedKeys =
60+
Collections.synchronizedMap(new IdentityHashMap<>());
61+
private final Map<ConfigChangeListener, Set<String>> m_interestedKeyPrefixes =
62+
Collections.synchronizedMap(new IdentityHashMap<>());
6163
private final ConfigUtil m_configUtil;
6264
private volatile Cache<String, Integer> m_integerCache;
6365
private volatile Cache<String, Long> m_longCache;
@@ -99,7 +101,7 @@ public void addChangeListener(ConfigChangeListener listener, Set<String> interes
99101

100102
@Override
101103
public void addChangeListener(ConfigChangeListener listener, Set<String> interestedKeys, Set<String> interestedKeyPrefixes) {
102-
if (!m_listeners.contains(listener)) {
104+
if (!containsListenerInstance(listener)) {
103105
m_listeners.add(listener);
104106
if (interestedKeys != null && !interestedKeys.isEmpty()) {
105107
m_interestedKeys.put(listener, Sets.newHashSet(interestedKeys));
@@ -114,7 +116,7 @@ public void addChangeListener(ConfigChangeListener listener, Set<String> interes
114116
public boolean removeChangeListener(ConfigChangeListener listener) {
115117
m_interestedKeys.remove(listener);
116118
m_interestedKeyPrefixes.remove(listener);
117-
return m_listeners.remove(listener);
119+
return m_listeners.removeIf(addedListener -> addedListener == listener);
118120
}
119121

120122
@Override
@@ -608,4 +610,13 @@ List<ConfigChange> calcPropertyChanges(String appId, String namespace, Propertie
608610

609611
return changes;
610612
}
613+
614+
private boolean containsListenerInstance(ConfigChangeListener listener) {
615+
for (ConfigChangeListener configChangeListener : m_listeners) {
616+
if (configChangeListener == listener) {
617+
return true;
618+
}
619+
}
620+
return false;
621+
}
611622
}

‎apollo-client/src/test/java/com/ctrip/framework/apollo/internals/AbstractConfigTest.java‎

Lines changed: 140 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@
1717
package com.ctrip.framework.apollo.internals;
1818

1919
import static org.junit.Assert.assertEquals;
20+
import static org.junit.Assert.assertNotSame;
21+
import static org.junit.Assert.assertThrows;
22+
import static org.junit.Assert.assertTrue;
2023
import static org.mockito.ArgumentMatchers.any;
2124
import static org.mockito.ArgumentMatchers.eq;
2225
import static org.mockito.Mockito.spy;
@@ -28,6 +31,7 @@
2831
import com.ctrip.framework.apollo.enums.PropertyChangeType;
2932
import com.ctrip.framework.apollo.model.ConfigChange;
3033
import com.ctrip.framework.apollo.model.ConfigChangeEvent;
34+
import com.ctrip.framework.apollo.spring.config.CachedCompositePropertySource;
3135
import com.google.common.util.concurrent.SettableFuture;
3236
import java.util.Collections;
3337
import java.util.HashMap;
@@ -122,6 +126,88 @@ public void onChange(ConfigChangeEvent changeEvent) {
122126
verify(configChangeListener2, times(1)).onChange(eq(configChangeEvent));
123127
}
124128

129+
@Test
130+
public void testFireConfigChange_twoCachedCompositePropertySourcesWithSameName_shouldBothBeNotified()
131+
throws ExecutionException, InterruptedException, TimeoutException {
132+
AbstractConfig abstractConfig = new ErrorConfig();
133+
final String namespace = "app-namespace-listener-equals";
134+
final String key = "great-key";
135+
136+
ListenerPair listenerPair = createSameNameListeners();
137+
final CountingCachedCompositePropertySource listener1 = listenerPair.listener1;
138+
final CountingCachedCompositePropertySource listener2 = listenerPair.listener2;
139+
140+
abstractConfig.addChangeListener(listener1, Collections.singleton(key));
141+
abstractConfig.addChangeListener(listener2, Collections.singleton(key));
142+
143+
Map<String, ConfigChange> changes = createSingleKeyChanges(namespace, key);
144+
145+
abstractConfig.fireConfigChange(someAppId, namespace, changes);
146+
147+
assertEquals(Collections.singleton(key), listener1.awaitChange(500, TimeUnit.MILLISECONDS).changedKeys());
148+
assertEquals(Collections.singleton(key), listener2.awaitChange(500, TimeUnit.MILLISECONDS).changedKeys());
149+
150+
assertEquals(1, listener1.changeCount.get());
151+
assertEquals(1, listener2.changeCount.get());
152+
}
153+
154+
@Test
155+
public void testFireConfigChange_twoCachedCompositePropertySourcesWithSameNameAndDifferentInterestedKeys_shouldNotConflict()
156+
throws ExecutionException, InterruptedException, TimeoutException {
157+
AbstractConfig abstractConfig = new ErrorConfig();
158+
final String namespace = "app-namespace-listener-interested-keys";
159+
final String key1 = "great-key-1";
160+
final String key2 = "great-key-2";
161+
162+
ListenerPair listenerPair = createSameNameListeners();
163+
final CountingCachedCompositePropertySource listener1 = listenerPair.listener1;
164+
final CountingCachedCompositePropertySource listener2 = listenerPair.listener2;
165+
166+
abstractConfig.addChangeListener(listener1, Collections.singleton(key1));
167+
abstractConfig.addChangeListener(listener2, Collections.singleton(key2));
168+
169+
abstractConfig.fireConfigChange(someAppId, namespace, createSingleKeyChanges(namespace, key1));
170+
171+
assertEquals(Collections.singleton(key1), listener1.awaitChange(500, TimeUnit.MILLISECONDS).changedKeys());
172+
assertThrows(TimeoutException.class, () -> listener2.awaitChange(200, TimeUnit.MILLISECONDS));
173+
174+
listener1.resetChangeFuture();
175+
listener2.resetChangeFuture();
176+
177+
abstractConfig.fireConfigChange(someAppId, namespace, createSingleKeyChanges(namespace, key2));
178+
179+
assertThrows(TimeoutException.class, () -> listener1.awaitChange(200, TimeUnit.MILLISECONDS));
180+
assertEquals(Collections.singleton(key2), listener2.awaitChange(500, TimeUnit.MILLISECONDS).changedKeys());
181+
182+
assertEquals(1, listener1.changeCount.get());
183+
assertEquals(1, listener2.changeCount.get());
184+
}
185+
186+
@Test
187+
public void testRemoveChangeListener_twoCachedCompositePropertySourcesWithSameName_shouldRemoveSpecifiedInstance()
188+
throws ExecutionException, InterruptedException, TimeoutException {
189+
AbstractConfig abstractConfig = new ErrorConfig();
190+
final String namespace = "app-namespace-listener-remove";
191+
final String key = "great-key";
192+
193+
ListenerPair listenerPair = createSameNameListeners();
194+
final CountingCachedCompositePropertySource listener1 = listenerPair.listener1;
195+
final CountingCachedCompositePropertySource listener2 = listenerPair.listener2;
196+
197+
abstractConfig.addChangeListener(listener1, Collections.singleton(key));
198+
abstractConfig.addChangeListener(listener2, Collections.singleton(key));
199+
200+
assertTrue(abstractConfig.removeChangeListener(listener2));
201+
202+
abstractConfig.fireConfigChange(someAppId, namespace, createSingleKeyChanges(namespace, key));
203+
204+
assertEquals(Collections.singleton(key), listener1.awaitChange(500, TimeUnit.MILLISECONDS).changedKeys());
205+
assertThrows(TimeoutException.class, () -> listener2.awaitChange(200, TimeUnit.MILLISECONDS));
206+
207+
assertEquals(1, listener1.changeCount.get());
208+
assertEquals(0, listener2.changeCount.get());
209+
}
210+
125211
@Test
126212
public void testFireConfigChange_changes_notify_once()
127213
throws ExecutionException, InterruptedException, TimeoutException {
@@ -188,4 +274,57 @@ public ConfigSourceType getSourceType() {
188274
throw new UnsupportedOperationException();
189275
}
190276
}
191-
}
277+
278+
private static class CountingCachedCompositePropertySource extends CachedCompositePropertySource {
279+
private final AtomicInteger changeCount = new AtomicInteger();
280+
private volatile SettableFuture<ConfigChangeEvent> changeFuture = SettableFuture.create();
281+
282+
private CountingCachedCompositePropertySource(String name) {
283+
super(name);
284+
}
285+
286+
@Override
287+
public void onChange(ConfigChangeEvent changeEvent) {
288+
changeCount.incrementAndGet();
289+
changeFuture.set(changeEvent);
290+
super.onChange(changeEvent);
291+
}
292+
293+
private void resetChangeFuture() {
294+
changeFuture = SettableFuture.create();
295+
}
296+
297+
private ConfigChangeEvent awaitChange(long timeout, TimeUnit unit)
298+
throws ExecutionException, InterruptedException, TimeoutException {
299+
return changeFuture.get(timeout, unit);
300+
}
301+
}
302+
303+
private static ListenerPair createSameNameListeners() {
304+
CountingCachedCompositePropertySource listener1 =
305+
new CountingCachedCompositePropertySource("ApolloBootstrapPropertySources");
306+
CountingCachedCompositePropertySource listener2 =
307+
new CountingCachedCompositePropertySource("ApolloBootstrapPropertySources");
308+
assertNotSame(listener1, listener2);
309+
assertEquals(listener1, listener2);
310+
return new ListenerPair(listener1, listener2);
311+
}
312+
313+
private static class ListenerPair {
314+
private final CountingCachedCompositePropertySource listener1;
315+
private final CountingCachedCompositePropertySource listener2;
316+
317+
private ListenerPair(CountingCachedCompositePropertySource listener1,
318+
CountingCachedCompositePropertySource listener2) {
319+
this.listener1 = listener1;
320+
this.listener2 = listener2;
321+
}
322+
}
323+
324+
private static Map<String, ConfigChange> createSingleKeyChanges(String namespace, String key) {
325+
Map<String, ConfigChange> changes = new HashMap<>();
326+
changes.put(key,
327+
new ConfigChange(someAppId, namespace, key, "old-value", "new-value", PropertyChangeType.MODIFIED));
328+
return changes;
329+
}
330+
}

0 commit comments

Comments
 (0)