somak2kai commented on PR #10631: URL: https://github.com/apache/rocketmq/pull/10631#issuecomment-5021314560
> ## Review by github-manager-bot > ### Summary > Fixes data race and TOCTOU inconsistency in `DefaultLitePullConsumerImpl.taskTable` by wrapping the iterate-remove-then-add sequence in `synchronized(taskTable)` blocks across `updatePullTask`, `updateAssignPullTask`, and `removePullTask`. > > ### Findings > * **[Critical]** `DefaultLitePullConsumerImplTaskTableRaceTest.java:107` — The test method `testUpdatePullTaskDoesNotDoubleScheduleSameQueue` calls `consumer.updateAssignQueueAndStartPullTask(topic, Collections.emptySet(), mqDivided)` directly, but this method does not exist in the diff (nor in the current codebase). The diff only modifies `updatePullTask`, `updateAssignPullTask`, and `removePullTask`. **This will cause a compilation error.** Please either: (a) add the missing method, or (b) update the test to use the existing `updatePullTask` method via reflection, consistent with the other two test methods. > * **[Info]** `DefaultLitePullConsumerImpl.java:222,467,725` — Synchronizing on `this.taskTable` (a `ConcurrentHashMap` instance) is functionally correct since the reference is stable, but slightly unconventional. A dedicated `private final Object taskTableLock = new Object()` would make the locking intent clearer and decouple the monitor from the map semantics. This is a minor style preference, not a blocker. > * **[Info]** The coarse-grained `synchronized` blocks serialize all task table mutations. Since rebalance is not a hot path, this is acceptable. No performance concern in practice. > > ### Positive Aspects > * ✅ Core fix correctly addresses the root cause: the non-atomic "iterate+remove, then containsKey+put" sequence is now atomic > * ✅ All three mutation paths (`updatePullTask`, `updateAssignPullTask`, `removePullTask`) are consistently synchronized > * ✅ Test design using `CyclicBarrier` for concurrent reproduction is solid > * ✅ `removePullTask` synchronization prevents cross-topic taskTable corruption during concurrent add/remove > * ✅ Minimal, focused changes — no unnecessary refactoring > > ### Verdict > The synchronization approach is correct and well-targeted. However, the test file has a compilation error that must be fixed before merge. Once `updateAssignQueueAndStartPullTask` is resolved, this should be ready to merge. > > _Automated review by github-manager-bot_ wait what compilation error? pls help -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
