fix: deduplicate config listeners by identity

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)

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<ConfigChangeListener> m_listeners = Lists.newCopyOnWriteArrayList();
-  private final Map<ConfigChangeListener, Set<String>> m_interestedKeys = Maps.newConcurrentMap();
-  private final Map<ConfigChangeListener, Set<String>> m_interestedKeyPrefixes = Maps.newConcurrentMap();
+  private final Map<ConfigChangeListener, Set<String>> m_interestedKeys =
+      Collections.synchronizedMap(new IdentityHashMap<>());
+  private final Map<ConfigChangeListener, Set<String>> m_interestedKeyPrefixes =
+      Collections.synchronizedMap(new IdentityHashMap<>());
   private final ConfigUtil m_configUtil;
   private volatile Cache<String, Integer> m_integerCache;
   private volatile Cache<String, Long> m_longCache;
@@ -99,7 +101,7 @@ public void addChangeListener(ConfigChangeListener listener, Set<String> interes
 
   @Override
   public void addChangeListener(ConfigChangeListener listener, Set<String> interestedKeys, Set<String> 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<String> 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<ConfigChange> 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;
+  }
 }

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<String, ConfigChange> 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();
     }
   }
-}
+
+  private static class CountingCachedCompositePropertySource extends CachedCompositePropertySource {
+    private final AtomicInteger changeCount = new AtomicInteger();
+    private volatile SettableFuture<ConfigChangeEvent> 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<String, ConfigChange> createSingleKeyChanges(String namespace, String key) {
+    Map<String, ConfigChange> changes = new HashMap<>();
+    changes.put(key,
+        new ConfigChange(someAppId, namespace, key, "old-value", "new-value", PropertyChangeType.MODIFIED));
+    return changes;
+  }
+}