fix: deduplicate config listeners by identity#121
fix: deduplicate config listeners by identity#121nobodyiam merged 2 commits intoapolloconfig:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughReplaces equality-based listener deduplication in AbstractConfig with identity-based checks so distinct listener instances with the same name can be registered, removed, and notified independently; adds unit tests validating multi-instance notifications and removal; updates CHANGES.md with a changelog entry. Changes
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(listenerA)? -> false
Config-->>Config: add listenerA and record interested keys/prefixes
Initializer->>Config: addChangeListener(listenerB, keysB, prefixesB)
Config-->>Config: containsListenerInstance(listenerB)? -> false
Config-->>Config: add listenerB
Config->>SourceA: fire changeEvent
SourceA-->>SourceA: onChange -> clear cached names
SourceA-->>Config: notify listeners (listenerA and/or listenerB as applicable)
Config->>SourceB: fire changeEvent
SourceB-->>SourceB: onChange -> clear cached names
SourceB-->>Config: notify listeners (both distinct instances receive events)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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_interestedKeyPrefixesremoval still usesequals, inconsistent with identity-basedm_listeners.When two listeners are equal-but-not-identical (the exact scenario this PR fixes),
m_interestedKeys.remove(listener)on line 115 usesequals-based lookup on theConcurrentHashMap, so removing either listener deletes the shared map entry for both. The surviving listener then has no entry inm_interestedKeys, causingisConfigChangeListenerInterestedto 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 bylistener1(since ConcurrentHashMap usesequals), so listener1's interested-key set is silently replaced by listener2's.Consider switching these maps to an
IdentityHashMap(wrapped withCollections.synchronizedMapfor thread safety) to match the identity semantics used form_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 useSettableFuturewith a timeout for deterministic waiting. Consider adopting the same pattern here for consistency and CI stability.
nobodyiam
force-pushed
the
codex/fix-88-listener-identity
branch
from
427c8d8 to
6d32daa
Compare
There was a problem hiding this comment.
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
AbstractConfigchange-listener de-duplication fromequals()to instance identity (==). - Update listener removal logic to attempt identity-based removal.
- Add a regression test covering two same-name
CachedCompositePropertySourcelisteners and updateCHANGES.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_interestedKeyPrefixesare still keyed byConfigChangeListenerin aConcurrentMap, so they still useequals/hashCode. For listeners whereequalsis not identity (e.g., SpringPropertySourceequality-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.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java
Outdated
Show resolved
Hide resolved
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/AbstractConfigTest.java
Show resolved
Hide resolved
nobodyiam
force-pushed
the
codex/fix-88-listener-identity
branch
from
6d32daa to
ad4aa96
Compare
nobodyiam
force-pushed
the
codex/fix-88-listener-identity
branch
from
ad4aa96 to
85b3cf4
Compare
There was a problem hiding this comment.
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
============================================
+ Coverage 68.68% 71.01% +2.32%
- Complexity 1503 1634 +131
============================================
Files 212 224 +12
Lines 6396 6727 +331
Branches 647 679 +32
============================================
+ Hits 4393 4777 +384
+ Misses 1673 1600 -73
- Partials 330 350 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
What's the purpose of this PR
Fixes a listener registration edge case in Spring Cloud bootstrap dual-context initialization.
When two
CachedCompositePropertySourcelisteners share the same name (ApolloBootstrapPropertySources),AbstractConfig#addChangeListenerusedList.contains(equals-based) to de-duplicate listeners.Because Spring
PropertySourcedefines equality by name, the second listener instance could be skipped,which leaves the active context listener unsubscribed and
CachedCompositePropertySource#namesstale after key add/delete.This PR switches listener de-duplication/removal to instance identity (
==) semantics and adds a regression testcovering two same-name
CachedCompositePropertySourcelisteners.Which issue(s) this PR fixes:
Fixes #88
Brief changelog
AbstractConfig#addChangeListenerto deduplicate by object identity instead ofequalsAbstractConfig#removeChangeListenerto remove by object identityCachedCompositePropertySourcelisteners both receiving change eventsCHANGES.mdFollow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Test run details
mvn -pl apollo-client clean testmvn -pl apollo-client -Dtest=AbstractConfigTest,CachedCompositePropertySourceTest,ApolloApplicationContextInitializerTest testSummary by CodeRabbit
Bug Fixes
Tests