[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
… 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.
wang-jiahua
force-pushed
the
fix/response-table-race-condition
branch
from
3c89a5a to
c100a62
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10576 +/- ##
=============================================
- Coverage 48.26% 48.18% -0.09%
+ Complexity 13433 13415 -18
=============================================
Files 1378 1378
Lines 100817 100817
Branches 13040 13041 +1
=============================================
- Hits 48660 48579 -81
- Misses 46211 46274 +63
- Partials 5946 5964 +18 ☔ 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
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.