fix(concurrency): refine synchronization in GroovySemanticDB #1044
Conversation
Consistent synchronization on index lists to prevent races between buildIndexes and removeFromIndexes. Atomic map removal when lists become empty.
📝 WalkthroughWalkthroughSingle-file refactoring of GroovySemanticDB cache statistics and indexing structures to improve thread-safety. Replaces non-thread-safe primitive Long counters with AtomicLong, switches indexing from synchronizedList to CopyOnWriteArrayList, and updates all related operations to use atomic operations and proper synchronization patterns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Summary of ChangesHello @albertocavalcante, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the concurrency and thread safety within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the thread safety of GroovySemanticDB by introducing atomic counters for cache statistics and using concurrent collections for symbol and occurrence indexes. These changes are well-aligned with the goal of refining synchronization for a concurrent environment. My review includes a couple of suggestions to remove redundant synchronized blocks when adding to CopyOnWriteArrayList, which will simplify the code and improve performance without compromising thread safety.
| val list = symbolIndex.computeIfAbsent(symbol.symbol) { CopyOnWriteArrayList() } | ||
| synchronized(list) { | ||
| list.add(uri to symbol) | ||
| } |
There was a problem hiding this comment.
The synchronized(list) block is redundant here. CopyOnWriteArrayList is a thread-safe collection, and its add method is already internally synchronized. You can simplify this operation and make it more efficient by removing the explicit synchronized block.
| val list = symbolIndex.computeIfAbsent(symbol.symbol) { CopyOnWriteArrayList() } | |
| synchronized(list) { | |
| list.add(uri to symbol) | |
| } | |
| symbolIndex.computeIfAbsent(symbol.symbol) { CopyOnWriteArrayList() }.add(uri to symbol) |
| val list = occurrenceIndex.computeIfAbsent(occurrence.symbol) { CopyOnWriteArrayList() } | ||
| synchronized(list) { | ||
| list.add(uri to occurrence) | ||
| } |
There was a problem hiding this comment.
Similar to the symbolIndex update, the synchronized(list) block here is redundant because CopyOnWriteArrayList.add() is already thread-safe. Removing the unnecessary synchronization will simplify the code and avoid the overhead of double-locking.
| val list = occurrenceIndex.computeIfAbsent(occurrence.symbol) { CopyOnWriteArrayList() } | |
| synchronized(list) { | |
| list.add(uri to occurrence) | |
| } | |
| occurrenceIndex.computeIfAbsent(occurrence.symbol) { CopyOnWriteArrayList() }.add(uri to occurrence) |
There was a problem hiding this comment.
Pull request overview
This PR tightens the concurrency behavior of GroovySemanticDB, focusing on making cache statistics updates thread-safe and refining how symbol/occurrence indexes are synchronized.
Changes:
- Replaced non-atomic cache hit/miss counters with
AtomicLongand updated all usages to be thread-safe. - Updated
getStatistics()to read snapshot values from the atomic counters for consistent cache metrics. - Switched symbol and occurrence index list implementations to
CopyOnWriteArrayListand adjusted index-building logic accordingly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val list = symbolIndex.computeIfAbsent(symbol.symbol) { CopyOnWriteArrayList() } | ||
| synchronized(list) { | ||
| list.add(uri to symbol) | ||
| } |
There was a problem hiding this comment.
Switching the index lists to CopyOnWriteArrayList while still wrapping all mutations and reads in synchronized(list) combines two synchronization mechanisms and incurs copy-on-write overhead on every add/remove without an accompanying gain in thread-safety. Since these lists are updated whenever documents are (re)indexed, this can negatively impact performance; consider either reverting to a simple mutable list guarded by synchronized(list) or simplifying the locking to rely on CopyOnWriteArrayList semantics with a different strategy for the compound "if empty then remove" operations.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
semantics/core/src/main/kotlin/com/github/albertocavalcante/gvy/semantics/db/GroovySemanticDB.kt (1)
196-240:⚠️ Potential issue | 🟠 MajorFix race condition where list can be removed from index while being mutated.
computeIfAbsent()returns a list reference that another thread can remove from the map viaremoveFromIndexesbefore theadd()executes. Subsequent mutations then occur on a list no longer in the map, silently losing index entries. UseConcurrentHashMap.compute()to mutate the list and map atomically within a single operation.🔧 Suggested atomic update pattern
private fun buildIndexes(uri: URI, doc: SemanticDocument) { // Index symbols doc.symbols.forEach { symbol -> - val list = symbolIndex.computeIfAbsent(symbol.symbol) { CopyOnWriteArrayList() } - synchronized(list) { - list.add(uri to symbol) - } + symbolIndex.compute(symbol.symbol) { _, list -> + val updated = list ?: CopyOnWriteArrayList() + updated.add(uri to symbol) + updated + } } // Index occurrences doc.occurrences.forEach { occurrence -> - val list = occurrenceIndex.computeIfAbsent(occurrence.symbol) { CopyOnWriteArrayList() } - synchronized(list) { - list.add(uri to occurrence) - } + occurrenceIndex.compute(occurrence.symbol) { _, list -> + val updated = list ?: CopyOnWriteArrayList() + updated.add(uri to occurrence) + updated + } } } private fun removeFromIndexes(uri: URI, doc: SemanticDocument) { // Remove symbols from index and cache doc.symbols.forEach { symbol -> - symbolIndex[symbol.symbol]?.let { list -> - synchronized(list) { - list.removeIf { it.first == uri } - if (list.isEmpty()) { - symbolIndex.remove(symbol.symbol, list) - cacheLock.write { symbolCache.remove(symbol.symbol) } - } - } - } + var removed = false + symbolIndex.computeIfPresent(symbol.symbol) { _, list -> + list.removeIf { it.first == uri } + if (list.isEmpty()) { + removed = true + null + } else { + list + } + } + if (removed) { + cacheLock.write { symbolCache.remove(symbol.symbol) } + } } // Remove occurrences from index doc.occurrences.forEach { occurrence -> - occurrenceIndex[occurrence.symbol]?.let { list -> - synchronized(list) { - list.removeIf { it.first == uri } - if (list.isEmpty()) { - occurrenceIndex.remove(occurrence.symbol, list) - } - } - } + occurrenceIndex.computeIfPresent(occurrence.symbol) { _, list -> + list.removeIf { it.first == uri } + if (list.isEmpty()) null else list + } } }
|
Summary
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.