fix: deduplicate config listeners by identity

[nobodyiam]

What's the purpose of this PR

Fixes a listener registration edge case in Spring Cloud bootstrap dual-context initialization.

When two CachedCompositePropertySource listeners share the same name (ApolloBootstrapPropertySources),
AbstractConfig#addChangeListener used List.contains (equals-based) to de-duplicate listeners.
Because Spring PropertySource defines equality by name, the second listener instance could be skipped,
which leaves the active context listener unsubscribed and CachedCompositePropertySource#names stale after key add/delete.

This PR switches listener de-duplication/removal to instance identity (==) semantics and adds a regression test
covering two same-name CachedCompositePropertySource listeners.

Which issue(s) this PR fixes:

Fixes #88

Brief changelog

• Change AbstractConfig#addChangeListener to deduplicate by object identity instead of equals
• Change AbstractConfig#removeChangeListener to remove by object identity
• Add regression test for two same-name CachedCompositePropertySource listeners both receiving change events
• Update CHANGES.md

Follow this checklist to help us incorporate your contribution quickly and easily:

• Read the Contributing Guide before making this pull request.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Write necessary unit tests to verify the code.
• Run mvn clean test to make sure this pull request doesn't break anything.
• Update the CHANGES log.

Test run details

• mvn -pl apollo-client clean test
• mvn -pl apollo-client -Dtest=AbstractConfigTest,CachedCompositePropertySourceTest,ApolloApplicationContextInitializerTest test

Summary by CodeRabbit

• Bug Fixes

  • Fixed an issue where distinct change listeners could be incorrectly de-duplicated, which could cause stale configuration caches during dual-context initialization. Listeners are now correctly distinguished and all registered listeners are notified; removal of a specific listener instance is handled reliably.

• Tests

  • Added unit tests verifying multiple listeners with the same name each receive notifications and that specific listener removal works.

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaces equality-based listener deduplication in AbstractConfig with identity-based checks so distinct listener instances with identical characteristics can be registered, removed, and notified independently; adds unit tests validating multi-instance notification and removal; updates CHANGES.md with a changelog entry.

Changes

Cohort / File(s)	Summary
Listener deduplication logic apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java	Replaced m_listeners.contains(listener) and direct m_listeners.remove(listener) with identity-based checks. Added containsListenerInstance(ConfigChangeListener) helper. addChangeListener/removeChangeListener now operate by instance identity.
Test coverage apollo-client/src/test/java/com/ctrip/framework/apollo/internals/AbstractConfigTest.java	Added tests: testFireConfigChange_twoCachedCompositePropertySourcesWithSameName_shouldBothBeNotified, testFireConfigChange_twoCachedCompositePropertySourcesWithSameNameAndDifferentInterestedKeys_shouldNotConflict, and testRemoveChangeListener_twoCachedCompositePropertySourcesWithSameName_shouldRemoveSpecifiedInstance. Introduced CountingCachedCompositePropertySource and helper utilities to validate per-instance notifications and removal by identity.
Changelog CHANGES.md	Added a changelog bullet describing the fix for de-duplication of change listeners by identity to prevent stale property-names cache during Spring Cloud bootstrap dual-context initialization.

Sequence Diagram(s)

sequenceDiagram
  participant Initializer as ApolloApplicationContextInitializer
  participant Config as AbstractConfig
  participant SourceA as CachedCompositePropertySource#A
  participant SourceB as CachedCompositePropertySource#B

  Note over Initializer,Config: Bootstrap and application contexts create distinct CachedCompositePropertySource instances with same name

  Initializer->>Config: addChangeListener(listenerA, keysA, prefixesA)
  Config-->>Config: containsListenerInstance? (identity) → false
  Config-->>Config: add listenerA (store interested keys/prefixes)

  Initializer->>Config: addChangeListener(listenerB, keysB, prefixesB)
  Config-->>Config: containsListenerInstance? (identity) → false
  Config-->>Config: add listenerB

  Config->>SourceA: notify changeEvent
  SourceA-->>SourceA: onChange -> clear cached names
  SourceA-->>Config: handler invoked (listenerA or listenerB depending on instance)

  Config->>SourceB: notify changeEvent
  SourceB-->>SourceB: onChange -> clear cached names
  SourceB-->>Config: handler invoked (distinct listener instances both get events)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Two hops, two ears, two listeners awake,
Same label, new heartbeat—no more mistake.
Cache clears its fur with a jubilant shake,
Dual contexts now sing, no stale paths to break.
The rabbit applauds and nibbles a cake.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: deduplicate config listeners by identity' accurately and concisely describes the main change—switching from equals-based to identity-based listener deduplication.
Linked Issues check	✅ Passed	All objectives from issue #88 are met: listener deduplication and removal now use identity (==) instead of equals-based comparison, enabling both same-name CachedCompositePropertySource instances to receive ConfigChangeEvent notifications.
Out of Scope Changes check	✅ Passed	All changes are directly related to the linked issue: modifications to addChangeListener and removeChangeListener for identity-based deduplication, addition of regression tests, and CHANGES.md update.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java (1)

103-112: Pre-existing TOCTOU in addChangeListener — not blocking but worth noting.

containsListenerInstance and m_listeners.add are not performed atomically. Under concurrent registration of the same listener instance, both threads could pass the identity check and add it twice. This race existed before this PR (with contains/add), so it's not a regression, but if you ever tighten this, a small synchronized block around the check-and-add would close the window.

apollo-client/src/test/java/com/ctrip/framework/apollo/internals/AbstractConfigTest.java (1)

278-301: Minor: SettableFuture.set throws if called twice before reset.

If onChange were invoked more than once between resets, the second changeFuture.set(...) would throw IllegalStateException. This is fine for the current controlled tests, but consider using changeCount as the sole invariant for multi-fire scenarios if you ever extend these tests.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java (1)

113-123: ⚠️ Potential issue | 🟠 Major

m_interestedKeys/m_interestedKeyPrefixes removal still uses equals, inconsistent with identity-based m_listeners.

When two listeners are equal-but-not-identical (the exact scenario this PR fixes), m_interestedKeys.remove(listener) on line 115 uses equals-based lookup on the ConcurrentHashMap, so removing either listener deletes the shared map entry for both. The surviving listener then has no entry in m_interestedKeys, causing isConfigChangeListenerInterested to treat it as interested in all keys — a silent broadening of its subscription scope.

The same issue applies to addChangeListener: m_interestedKeys.put(listener2, ...) overwrites the entry keyed by listener1 (since ConcurrentHashMap uses equals), so listener1's interested-key set is silently replaced by listener2's.

Consider switching these maps to an IdentityHashMap (wrapped with Collections.synchronizedMap for thread safety) to match the identity semantics used for m_listeners.

Proposed fix sketch

-  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<>());

And in removeChangeListener, move the map removals inside the identity check so only the matched instance's entries are cleaned up:

   public boolean removeChangeListener(ConfigChangeListener listener) {
-    m_interestedKeys.remove(listener);
-    m_interestedKeyPrefixes.remove(listener);
     for (ConfigChangeListener addedListener : m_listeners) {
       if (addedListener == listener) {
+        m_interestedKeys.remove(addedListener);
+        m_interestedKeyPrefixes.remove(addedListener);
         return m_listeners.remove(addedListener);
       }
     }
     return false;
   }

🧹 Nitpick comments (1)

apollo-client/src/test/java/com/ctrip/framework/apollo/internals/AbstractConfigTest.java (1)

128-156: Good regression test that clearly validates the fix.

The precondition assertions on lines 140-141 are excellent — they explicitly verify the conditions (assertNotSame + assertEquals) that trigger the bug.

Minor robustness note: Thread.sleep(100) on line 152 is inherently racy. The existing tests in this class use SettableFuture with a timeout for deterministic waiting. Consider adopting the same pattern here for consistency and CI stability.

[Copilot]

Pull request overview

This PR addresses a Spring Cloud bootstrap dual-context edge case where change listeners that are different instances but equals()-equal (e.g., Spring PropertySource equality-by-name) could be incorrectly de-duplicated, leading to missed notifications and stale cached property names.

Changes:

• Switch AbstractConfig change-listener de-duplication from equals() to instance identity (==).
• Update listener removal logic to attempt identity-based removal.
• Add a regression test covering two same-name CachedCompositePropertySource listeners and update CHANGES.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java	Changes listener registration/removal logic to use identity semantics.
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/AbstractConfigTest.java	Adds regression test for two equals()-equal but distinct listener instances both receiving events.
CHANGES.md	Documents the fix in release notes.

Comments suppressed due to low confidence (1)

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

• m_interestedKeys/m_interestedKeyPrefixes are still keyed by ConfigChangeListener in a ConcurrentMap, so they still use equals/hashCode. For listeners where equals is not identity (e.g., Spring PropertySource equality-by-name), registering two distinct instances will collide and overwrite each other’s interested-keys/prefixes, and later lookups/removals won’t be identity-safe. To truly switch to identity semantics, store per-listener metadata with identity keys (e.g., wrap the listener in an identity-based key, or replace the separate maps with a single registration object that holds listener + filters).

  public void addChangeListener(ConfigChangeListener listener, Set<String> interestedKeys, Set<String> interestedKeyPrefixes) {
    if (!containsListenerInstance(listener)) {
      m_listeners.add(listener);
      if (interestedKeys != null && !interestedKeys.isEmpty()) {
        m_interestedKeys.put(listener, Sets.newHashSet(interestedKeys));
      }
      if (interestedKeyPrefixes != null && !interestedKeyPrefixes.isEmpty()) {
        m_interestedKeyPrefixes.put(listener, Sets.newHashSet(interestedKeyPrefixes));
      }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.