From 85b3cf407975bde011e6d336412b2902a5840f0c Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 8 Feb 2026 11:42:26 +0800 Subject: [PATCH] fix: deduplicate config listeners by identity Fixes #88 --- CHANGES.md | 1 + .../apollo/internals/AbstractConfig.java | 19 ++- .../apollo/internals/AbstractConfigTest.java | 141 +++++++++++++++++- 3 files changed, 156 insertions(+), 5 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 2060a6a5..ad3806c4 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,7 @@ Apollo Java 2.5.0 * [Feature Added a new feature to get instance count by namespace.](https://github.com/apolloconfig/apollo-java/pull/103) * [Feature Support retry in open api client.](https://github.com/apolloconfig/apollo-java/pull/105) * [Support Spring Boot 4.0 bootstrap context package relocation for apollo-client-config-data](https://github.com/apolloconfig/apollo-java/pull/115) +* [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) ------------------ All issues and pull requests are [here](https://github.com/apolloconfig/apollo-java/milestone/5?closed=1) diff --git a/apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java b/apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java index 3afddcbe..196a6079 100644 --- a/apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java +++ b/apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java @@ -56,8 +56,10 @@ public abstract class AbstractConfig implements Config { protected static final ExecutorService m_executorService; private final List m_listeners = Lists.newCopyOnWriteArrayList(); - private final Map> m_interestedKeys = Maps.newConcurrentMap(); - private final Map> m_interestedKeyPrefixes = Maps.newConcurrentMap(); + private final Map> m_interestedKeys = + Collections.synchronizedMap(new IdentityHashMap<>()); + private final Map> m_interestedKeyPrefixes = + Collections.synchronizedMap(new IdentityHashMap<>()); private final ConfigUtil m_configUtil; private volatile Cache m_integerCache; private volatile Cache m_longCache; @@ -99,7 +101,7 @@ public void addChangeListener(ConfigChangeListener listener, Set interes @Override public void addChangeListener(ConfigChangeListener listener, Set interestedKeys, Set interestedKeyPrefixes) { - if (!m_listeners.contains(listener)) { + if (!containsListenerInstance(listener)) { m_listeners.add(listener); if (interestedKeys != null && !interestedKeys.isEmpty()) { m_interestedKeys.put(listener, Sets.newHashSet(interestedKeys)); @@ -114,7 +116,7 @@ public void addChangeListener(ConfigChangeListener listener, Set interes public boolean removeChangeListener(ConfigChangeListener listener) { m_interestedKeys.remove(listener); m_interestedKeyPrefixes.remove(listener); - return m_listeners.remove(listener); + return m_listeners.removeIf(addedListener -> addedListener == listener); } @Override @@ -608,4 +610,13 @@ List calcPropertyChanges(String appId, String namespace, Propertie return changes; } + + private boolean containsListenerInstance(ConfigChangeListener listener) { + for (ConfigChangeListener configChangeListener : m_listeners) { + if (configChangeListener == listener) { + return true; + } + } + return false; + } } diff --git a/apollo-client/src/test/java/com/ctrip/framework/apollo/internals/AbstractConfigTest.java b/apollo-client/src/test/java/com/ctrip/framework/apollo/internals/AbstractConfigTest.java index 32499ae5..0f6494d0 100644 --- a/apollo-client/src/test/java/com/ctrip/framework/apollo/internals/AbstractConfigTest.java +++ b/apollo-client/src/test/java/com/ctrip/framework/apollo/internals/AbstractConfigTest.java @@ -17,6 +17,9 @@ package com.ctrip.framework.apollo.internals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.spy; @@ -28,6 +31,7 @@ import com.ctrip.framework.apollo.enums.PropertyChangeType; import com.ctrip.framework.apollo.model.ConfigChange; import com.ctrip.framework.apollo.model.ConfigChangeEvent; +import com.ctrip.framework.apollo.spring.config.CachedCompositePropertySource; import com.google.common.util.concurrent.SettableFuture; import java.util.Collections; import java.util.HashMap; @@ -122,6 +126,88 @@ public void onChange(ConfigChangeEvent changeEvent) { verify(configChangeListener2, times(1)).onChange(eq(configChangeEvent)); } + @Test + public void testFireConfigChange_twoCachedCompositePropertySourcesWithSameName_shouldBothBeNotified() + throws ExecutionException, InterruptedException, TimeoutException { + AbstractConfig abstractConfig = new ErrorConfig(); + final String namespace = "app-namespace-listener-equals"; + final String key = "great-key"; + + ListenerPair listenerPair = createSameNameListeners(); + final CountingCachedCompositePropertySource listener1 = listenerPair.listener1; + final CountingCachedCompositePropertySource listener2 = listenerPair.listener2; + + abstractConfig.addChangeListener(listener1, Collections.singleton(key)); + abstractConfig.addChangeListener(listener2, Collections.singleton(key)); + + Map changes = createSingleKeyChanges(namespace, key); + + abstractConfig.fireConfigChange(someAppId, namespace, changes); + + assertEquals(Collections.singleton(key), listener1.awaitChange(500, TimeUnit.MILLISECONDS).changedKeys()); + assertEquals(Collections.singleton(key), listener2.awaitChange(500, TimeUnit.MILLISECONDS).changedKeys()); + + assertEquals(1, listener1.changeCount.get()); + assertEquals(1, listener2.changeCount.get()); + } + + @Test + public void testFireConfigChange_twoCachedCompositePropertySourcesWithSameNameAndDifferentInterestedKeys_shouldNotConflict() + throws ExecutionException, InterruptedException, TimeoutException { + AbstractConfig abstractConfig = new ErrorConfig(); + final String namespace = "app-namespace-listener-interested-keys"; + final String key1 = "great-key-1"; + final String key2 = "great-key-2"; + + ListenerPair listenerPair = createSameNameListeners(); + final CountingCachedCompositePropertySource listener1 = listenerPair.listener1; + final CountingCachedCompositePropertySource listener2 = listenerPair.listener2; + + abstractConfig.addChangeListener(listener1, Collections.singleton(key1)); + abstractConfig.addChangeListener(listener2, Collections.singleton(key2)); + + abstractConfig.fireConfigChange(someAppId, namespace, createSingleKeyChanges(namespace, key1)); + + assertEquals(Collections.singleton(key1), listener1.awaitChange(500, TimeUnit.MILLISECONDS).changedKeys()); + assertThrows(TimeoutException.class, () -> listener2.awaitChange(200, TimeUnit.MILLISECONDS)); + + listener1.resetChangeFuture(); + listener2.resetChangeFuture(); + + abstractConfig.fireConfigChange(someAppId, namespace, createSingleKeyChanges(namespace, key2)); + + assertThrows(TimeoutException.class, () -> listener1.awaitChange(200, TimeUnit.MILLISECONDS)); + assertEquals(Collections.singleton(key2), listener2.awaitChange(500, TimeUnit.MILLISECONDS).changedKeys()); + + assertEquals(1, listener1.changeCount.get()); + assertEquals(1, listener2.changeCount.get()); + } + + @Test + public void testRemoveChangeListener_twoCachedCompositePropertySourcesWithSameName_shouldRemoveSpecifiedInstance() + throws ExecutionException, InterruptedException, TimeoutException { + AbstractConfig abstractConfig = new ErrorConfig(); + final String namespace = "app-namespace-listener-remove"; + final String key = "great-key"; + + ListenerPair listenerPair = createSameNameListeners(); + final CountingCachedCompositePropertySource listener1 = listenerPair.listener1; + final CountingCachedCompositePropertySource listener2 = listenerPair.listener2; + + abstractConfig.addChangeListener(listener1, Collections.singleton(key)); + abstractConfig.addChangeListener(listener2, Collections.singleton(key)); + + assertTrue(abstractConfig.removeChangeListener(listener2)); + + abstractConfig.fireConfigChange(someAppId, namespace, createSingleKeyChanges(namespace, key)); + + assertEquals(Collections.singleton(key), listener1.awaitChange(500, TimeUnit.MILLISECONDS).changedKeys()); + assertThrows(TimeoutException.class, () -> listener2.awaitChange(200, TimeUnit.MILLISECONDS)); + + assertEquals(1, listener1.changeCount.get()); + assertEquals(0, listener2.changeCount.get()); + } + @Test public void testFireConfigChange_changes_notify_once() throws ExecutionException, InterruptedException, TimeoutException { @@ -188,4 +274,57 @@ public ConfigSourceType getSourceType() { throw new UnsupportedOperationException(); } } -} \ No newline at end of file + + private static class CountingCachedCompositePropertySource extends CachedCompositePropertySource { + private final AtomicInteger changeCount = new AtomicInteger(); + private volatile SettableFuture changeFuture = SettableFuture.create(); + + private CountingCachedCompositePropertySource(String name) { + super(name); + } + + @Override + public void onChange(ConfigChangeEvent changeEvent) { + changeCount.incrementAndGet(); + changeFuture.set(changeEvent); + super.onChange(changeEvent); + } + + private void resetChangeFuture() { + changeFuture = SettableFuture.create(); + } + + private ConfigChangeEvent awaitChange(long timeout, TimeUnit unit) + throws ExecutionException, InterruptedException, TimeoutException { + return changeFuture.get(timeout, unit); + } + } + + private static ListenerPair createSameNameListeners() { + CountingCachedCompositePropertySource listener1 = + new CountingCachedCompositePropertySource("ApolloBootstrapPropertySources"); + CountingCachedCompositePropertySource listener2 = + new CountingCachedCompositePropertySource("ApolloBootstrapPropertySources"); + assertNotSame(listener1, listener2); + assertEquals(listener1, listener2); + return new ListenerPair(listener1, listener2); + } + + private static class ListenerPair { + private final CountingCachedCompositePropertySource listener1; + private final CountingCachedCompositePropertySource listener2; + + private ListenerPair(CountingCachedCompositePropertySource listener1, + CountingCachedCompositePropertySource listener2) { + this.listener1 = listener1; + this.listener2 = listener2; + } + } + + private static Map createSingleKeyChanges(String namespace, String key) { + Map changes = new HashMap<>(); + changes.put(key, + new ConfigChange(someAppId, namespace, key, "old-value", "new-value", PropertyChangeType.MODIFIED)); + return changes; + } +}