Fix/codec registry thread safety - #699
Conversation
One CodecRegistry instance is shared by every reader and writer created from a TopicClient. getCodec is called from the compression threads (WriterQueue), from the decompression threads (MessageDecoder via Encoder.decode) and from gRPC callbacks, while registerCodec is public API that may be called at any time. Backing that with a plain HashMap means concurrent puts can lose entries or corrupt the table. Use a ConcurrentHashMap. The new test loses codecs on every run with the old implementation.
![robot-vibe-db[bot]](https://avatars.githubusercontent.com/in/3319454?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
Critical issues
No critical issues found.
Other findings
- Major | High: The concurrent regression test
registerCustomCodecIsSafeForConcurrentUsewas added in the first commit to validate the HashMap→ConcurrentHashMap fix, but removed in the second commit. This is the only test that proves the thread-safety fix works and would catch future regressions (e.g., someone reverting to HashMap). —topic/src/test/java/tech/ydb/topic/impl/CodecRegistryTest.java
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
| final Map<Integer, Codec> customCodecMap; | ||
| // registerCodec may be called at any moment, while getCodec is used by the compression and the | ||
| // decompression threads of every reader and writer created from the same TopicClient | ||
| private final Map<Integer, Codec> customCodecMap = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Severity: Major
Confidence: High
The concurrent test registerCustomCodecIsSafeForConcurrentUse — added in the first commit (87390cb) specifically to validate this HashMap→ConcurrentHashMap change — was removed in the second commit (a4a67f1). The first commit's message states: "The new test loses codecs on every run with the old implementation."
This test is the only automated validation that the thread-safety fix works. Without it, there is no regression test to catch future changes that might reintroduce the race (e.g., someone refactoring back to a plain HashMap, or introducing a compound read-then-write operation that ConcurrentHashMap alone doesn't make atomic).
Suggested fix: restore the registerCustomCodecIsSafeForConcurrentUse test. It uses 4 writer threads + 1 reader thread with a CountDownLatch to stress-test concurrent registrations, and asserts that no codecs are lost — this is exactly the kind of test that should stay as a regression guard.
|
Analysis performed by claude, claude-opus-4-6. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #699 +/- ##
============================================
+ Coverage 71.91% 71.97% +0.06%
- Complexity 3474 3493 +19
============================================
Files 390 391 +1
Lines 16220 16277 +57
Branches 1698 1705 +7
============================================
+ Hits 11664 11716 +52
- Misses 3904 3908 +4
- Partials 652 653 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
No description provided.