fuyou001 commented on PR #10828: URL: https://github.com/apache/rocketmq/pull/10828#issuecomment-5215485515
Thanks for fixing the lost-reentrancy path described in #10827. I traced the current head through POP entry, long-poll wakeup, ACK/change-invisible, lock cleanup, request lifetime, and persisted OrderInfo. The direction is reasonable, but the new synchronous wait couples several P1 correctness and availability risks, so I do not think the busy-spin implementation is safe to merge yet. ### P1: the unbounded spin can exhaust the shared Pull executor and outlive the request `PopConsumerService.tryLockForPop` spins at lines 493-495 before `popAsync` can return a Future. Network POP and long-poll wakeup execute on `pullMessageExecutor`, which is also shared by PULL, PEEK, POP_LITE, NOTIFICATION, and POLLING_INFO; Local Proxy invokes the processor synchronously on its caller thread. A burst of same-attemptId retries can therefore occupy the complete shared pool while continuously calling `Thread.yield()`. The loop has no request deadline, channel-active, interruption, cancellation, or Broker-stop check. The timeout check in `PopMessageProcessor` happens only before entering `popAsync`. Consequently a request can expire or disconnect, later acquire the lock, and still rewrite OrderInfo/popTime. Multiple stale retries can also invalidate the receipt returned by a live retry. The premise that every holder releases is not true on all existing paths: after acquisition, a deleted or consume-disabled subscription group returns at `PopConsumerService.java:367-370` before the `whenComplete` unlock is installed, and a synchronous setup exception can reach the catch/return at lines 466-470 with the same result. Please replace the worker-thread busy-spin with bounded, cancellable asynchronous acquisition or a lock-release notification path. Enforce the original request deadline and channel/service state, and put every successful acquisition under one ownership guard. ### P1: timeout cleanup is not a safe backstop and can break mutual exclusion The new comment at `PopConsumerService.java:488-490` relies on `PopConsumerLockService.removeTimeout()`. That method removes an entry after two minutes even when its `locked` flag is true. The spinner then creates and acquires a new `TimedLock` for the same key while the old holder can still be inside the critical section. There is a second ownership problem: `unlock(key)` looks up the current map entry instead of releasing the instance acquired by the caller. The concrete sequence is: 1. holder A owns old lock L1; 2. cleanup removes locked L1; 3. waiter B creates and acquires L2, so A and B now overlap; 4. A executes `unlock(key)`, finds L2, and releases the lock owned by B; 5. a third request can enter concurrently with B. This can concurrently mutate OrderInfo and consumerOffset across POP, ACK, and ChangeInvisible. The existing lock test even verifies removal while `locked=true`, but does not test ownership after replacement. Please do not use removal of a held entry as lock handoff. Fix the leak paths and only remove safely idle entries, or introduce owner/epoch/fencing semantics so an old holder cannot commit or unlock a replacement. Add a regression with old holder + timeout cleanup + waiter + old unlock + third contender, asserting that the critical-section concurrency never exceeds one. ### P1: waiting consumes the invisible time before delivery `PopMessageProcessor` captures `beginTimeMills` before calling `popAsync`, and `PopConsumerContext` stores that value as final `popTime` before `tryLockForPop` waits. After the lock is eventually acquired, the same old value is persisted into OrderInfo and returned in the receipt. FIFO blocking calculates visibility as `popTime + invisibleTime`. If the wait approaches or exceeds `invisibleTime`, the returned message has a shortened or already-expired lease. Once the lock is released, a different attemptId can immediately pass `checkBlock` and read the same queue head while the first client is still processing it. After bounded acquisition, recheck the request deadline and establish popTime at the actual delivery epoch before reading/updating OrderInfo. Add a test that holds the lock longer than invisibleTime and verifies that the returned receipt remains invisible for the full configured duration from actual delivery. ### P1: a stored matching string does not prove an active retry for the target queue `QueueLevelConsumerManager.isAttemptIdMatched` scans every queue under topic/group and checks only the stored string. It does not receive the requested queueId, inspect ACK bits or next-visible-time, or revalidate after acquiring the lock. OrderInfo remains after all messages are ACKed or visibility expires, is cleaned only after up to 24 hours, and is persisted/restored across Broker restart. Therefore fully-ACKed, expired, restarted stale IDs, and an ID stored only on another queue can all enter the unbounded wait. The check is also outside the lock, so the current holder can ACK or replace the OrderInfo while the waiter is spinning. Please make the predicate queue-aware and state-aware: for an explicit queue, require a matching active unacked delivery on that queue; only `queueId=-1` should scan queues. Revalidate under the acquired lock before reading messages. Regression coverage should include all-ACKed, expired, explicit-queue mismatch, state replacement while waiting, and persisted active versus stale OrderInfo. ### Suggested validation The current tests pass, but `PopConsumerServiceLockRetryTest` only makes a mock `tryLock` succeed on call 51. It does not exercise a real holder, request expiry/cancellation, shared-executor saturation, timeout cleanup, receipt validity, or state changes while waiting. Those scenarios should be covered before relying on the new waiting path. -- 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]
