lizhimins opened a new pull request, #10828:
URL: https://github.com/apache/rocketmq/pull/10828
## What is the purpose of the change
Fixes #10827.
An orderly (fifo / PopKv) retry carrying the same attemptId is an idempotent
reentrant request, but `PopConsumerService.popAsync` fails fast with an empty
response when the group@topic lock is contended. The retry then suspends in
long polling, burns the only reentrant opportunity, times out, and the client
rotates to a new attemptId — permanently losing reentrancy and blocking the
queue head (up to one invisibleTime, or ~3h when proxy autoRenew keeps
extending nextVisibleTime). See the issue for the full failure chain, verified
step by step against code in a real incident (a patrol system consuming orderly
messages through Proxy intermittently hung for a long time).
## Brief description of the change
Introduce `tryLockForPop` in `PopConsumerService`: fifo requests with a
non-empty attemptId spin-retry `tryLock` until the lock is acquired instead of
failing fast; all other requests keep the fail-fast behavior.
Correctness rationale:
- The lock holder always releases on pop completion (`whenComplete` unlock),
with the lock service's 2-minute expiry sweep as the worst-case backstop, so
the spin cannot wait forever.
- Same-attemptId contention is rare and the wait is normally milliseconds,
so busy-wait cost is negligible.
- Once the lock is acquired, the existing re-pop path runs (checkBlock
passes for the same attemptId, consumedCount is not incremented), fully
preserving the reentrant semantics.
- The wakeUp long-polling re-execution path benefits as well.
A lock-free read-only replay of the previous OrderInfo batch was evaluated
first; it works but is much larger, and further analysis showed the existing
re-pop path is already self-consistent (invalidating the receipt handle of the
lost delivery is normal at-least-once semantics) — the real defect is only that
lock contention leaves the retry empty, hence this minimal fix (+26 lines, no
config switch).
## Does this pull request affect any existing functionality?
No. Only fifo requests carrying a non-empty attemptId change behavior (spin
instead of empty response on lock contention); all other requests keep
fail-fast.
## Verification
- New unit tests `PopConsumerServiceLockRetryTest`: spin until the lock is
acquired and the pop path proceeds; non-fifo keeps fail-fast. All 17 existing
`PopConsumerServiceTest` cases pass, checkstyle clean, JDK 8 compile verified.
- Real-environment verification (k8s micro setup, broker image with this
fix):
- End-to-end: first pop FOUND/2 unacked → same-attemptId re-pop FOUND/2
within milliseconds → ack succeeds → fresh-attemptId pop finds the queue
drained.
- Contention probe: 5 rounds × 16 concurrent same-attemptId pops, all 80
returned the same batch with zero empty responses; broker logs confirmed all 16
requests per round executed inside the lock, i.e. the contention window was
genuinely exercised.
--
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]
