[ISSUE #10575] Fix race condition between scanResponseTable and processResponseCommand - #10576
[ISSUE #10575] Fix race condition between scanResponseTable and processResponseCommand#10576wang-jiahua wants to merge 1 commit into
Conversation
3c89a5a to
c100a62
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10576 +/- ##
=============================================
- Coverage 48.58% 48.50% -0.09%
+ Complexity 13683 13655 -28
=============================================
Files 1381 1381
Lines 101464 101464
Branches 13187 13188 +1
=============================================
- Hits 49298 49211 -87
- Misses 46171 46239 +68
- Partials 5995 6014 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR fixes a race condition between scanResponseTable() and processResponseCommand() in NettyRemotingAbstract. The fix is correct and well-targeted.
Findings
-
[Info]
NettyRemotingAbstract.java:470— Good fix. ChangingresponseTable.get(opaque)+ laterresponseTable.remove(opaque)to a singleresponseTable.remove(opaque)eliminates the TOCTOU (time-of-check-to-time-of-use) race window. SinceresponseTableis aConcurrentHashMap,remove()is atomic and returns the value in one operation. -
[Info]
NettyRemotingAbstract.java:565-570— Correct defensive approach. Switching from iterator-basedit.remove()toresponseTable.remove(key)with a null check ensures that ifprocessResponseCommandalready consumed the entry,scanResponseTablewill not double-release theResponseFuture. This prevents potential resource corruption from duplicaterelease()calls. -
[Info] Codecov flags 1 uncovered line — this is the
removed == nullbranch, which only triggers under the exact race condition this PR fixes. The branch is correct but inherently difficult to cover in unit tests due to its timing-dependent nature.
Assessment
| Dimension | Verdict |
|---|---|
| Correctness | ✅ Race condition properly eliminated via atomic remove |
| Performance | ✅ No regression; ConcurrentHashMap.remove() is O(1) |
| Tests | |
| Compatibility | ✅ No API changes; internal implementation only |
LGTM. Clean, minimal fix for a real concurrency bug.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Defensive fix with proper validation and test coverage. LGTM.
Automated review by github-manager-bot
… processResponseCommand processResponseCommand used get+remove (non-atomic) to retrieve ResponseFuture from responseTable. scanResponseTable used iterator.remove(). If the response arrived between scanResponseTable's remove and processResponseCommand's get, the response would be logged as 'not matched any request' and silently dropped. The callback would fire with timeout instead of success, even though the broker had successfully processed the request. Fix: Use ConcurrentHashMap.remove(key) in both methods. This is atomic: either processResponseCommand gets the future (and executes success callback), or scanResponseTable gets it (and executes timeout callback), but never both and never neither.
c100a62 to
9188a91
Compare
Which Issue(s) This PR Fixes
Fixes #10575
Brief Description
processResponseCommandusedresponseTable.get(opaque)+responseTable.remove(opaque)(non-atomic) to retrieve ResponseFuture.scanResponseTableusediterator.remove(). If the response arrived betweenscanResponseTable's remove andprocessResponseCommand's get, the response was logged asnot matched any requestand silently dropped.Fix: Use
ConcurrentHashMap.remove(key)(atomic) in both methods. EitherprocessResponseCommandgets the future (success callback) orscanResponseTablegets it (timeout callback), but never both and never neither.How Did You Implement It
processResponseCommand: Changed
responseTable.get(opaque)+responseTable.remove(opaque)to a singleresponseTable.remove(opaque)call.scanResponseTable: Changed
it.remove()toresponseTable.remove(next.getKey())and added a null check on the return value.How to Verify It
Reproduction: 256 threads, 50ms timeout, 1MB commitlog, single send thread pool. In 2 minutes, 776
not matched any requestwarnings appeared in clientremoting.log— all withcode=0(SEND_OK) and valid msgId/queueOffset. After applying this fix, the warnings should disappear.Existing tests:
NettyRemotingAbstractTest(5 tests) all pass.