fix the concurrent issue in SpringValueRegistry.scanAndClean #95
Conversation
WalkthroughThe pull request addresses a concurrent modification issue in the Changes
Assessment against linked issues
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apollo-client/src/main/java/com/ctrip/framework/apollo/spring/property/SpringValueRegistry.java (2)
93-94: Consider adding debug logging for removed entries.Adding debug logging when entries are removed would help with troubleshooting and monitoring the cleanup process.
// clear unused spring values + if (logger.isDebugEnabled()) { + logger.debug("Removing invalid SpringValue: {}", springValue.getValue()); + } springValueIterator.remove();
87-96: Consider batch processing to reduce lock contention.For better performance under high load, consider processing entries in batches to reduce lock holding time. This would be especially beneficial when dealing with large numbers of entries.
Multimap<String, SpringValue> springValues = iterator.next(); + List<Entry<String, SpringValue>> invalidEntries = new ArrayList<>(); synchronized (springValues) { Iterator<Entry<String, SpringValue>> springValueIterator = springValues.entries() .iterator(); while (springValueIterator.hasNext()) { Entry<String, SpringValue> springValue = springValueIterator.next(); if (!springValue.getValue().isTargetBeanValid()) { - // clear unused spring values - springValueIterator.remove(); + invalidEntries.add(springValue); } } + // Batch remove invalid entries + if (!invalidEntries.isEmpty()) { + invalidEntries.forEach(entry -> springValues.remove(entry.getKey(), entry.getValue())); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGES.md(1 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/spring/property/SpringValueRegistry.java(1 hunks)
🔇 Additional comments (2)
CHANGES.md (1)
15-15: LGTM! Clear and well-documented changelog entry.
The changelog entry accurately describes the fix and includes the correct PR reference.
apollo-client/src/main/java/com/ctrip/framework/apollo/spring/property/SpringValueRegistry.java (1)
87-96: LGTM! Correct synchronization implementation.
The synchronization implementation properly addresses the concurrent modification issue by:
- Synchronizing on the springValues object during iteration
- Maintaining a minimal synchronization scope
- Preserving the interrupt check
What's the purpose of this PR
fix the concurrent issue in SpringValueRegistry.scanAndClean
Which issue(s) this PR fixes:
Fixes apolloconfig/apollo#5296, apolloconfig/apollo#4355
Brief changelog
scanAndCleanFollow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
New Features
Bug Fixes
Documentation