Re: [PR] fix: prevent chunked producer self-deadlock when totalChunks > MaxPendingMessages [pulsar-client-go]

["n24q02m (via GitHub)" <gi...@apache.org>]

n24q02m commented on PR #1540:
URL: https://github.com/apache/pulsar-client-go/pull/1540#issuecomment-5952729554

   Thanks for the detailed review — the partial-delivery argument convinced us, and we're not going to defend the per-chunk acquisition loop.
   
   We implemented the batch acquire you proposed and measured it against the regression test. Short version: on its own it does not close #1448, so we'd like to check the direction with you before pushing another iteration.
   
   ### What we measured
   
   Implementation as described in your sketch: `AcquireN` / `TryAcquireN` on the internal semaphore, `reserveSemaphore` taking all `totalChunks` permits in one atomic call before the message enters the event loop, and the per-chunk acquire removed from `internalSend`. `TestSendChunkedMessageWithMoreChunksThanMaxPendingMessages` (11 chunks of 1000 bytes), Pulsar 3.0.11 standalone, `DisableBatching: true`:
   
   | `MaxPendingMessages` | result |
   | --- | --- |
   | 2 (< totalChunks) | FAIL — `context deadline exceeded` after 30s |
   | 11 (= totalChunks) | PASS in 0.05s |
   | 1000 | PASS in 0.05s |
   
   The same test against `cc1033e` (the per-chunk code in this branch) **passes in 0.13s at `MaxPendingMessages: 2`** — the value the test itself sets.
   
   So the batch acquire regresses this regression test at its own configured value, and only starts passing once `MaxPendingMessages` is raised to at least the chunk count. That is the opposite of the direction we need, and it's why we'd rather not push it as-is.
   
   One plausible reading of the boundary: at `MaxPendingMessages: 2` the per-chunk acquire keeps at most 2 chunks in flight, so chunk *k+2* is only dispatched after chunk *k* is acked. With the batch acquire nothing throttles in-flight chunks, and something else becomes the bound at the same `totalChunks` threshold — i.e. the semaphore is not the only limit on a chunk group.
   
   ### What we do know about the remaining hang
   
   At timeout (goroutine dump plus producer warnings, `MaxPendingMessages: 2`):
   
   - the event loop is parked in `runEventsLoop`'s `select` — idle, not blocked in `writeData`, `PutUnsafe`, or the semaphore;
   - no goroutine is blocked in the semaphore or the pending queue;
   - no `sequenceId` mismatch (`local < remote` / `local > remote`) is logged;
   - exactly one ack is dropped as `Got ack ledgerId:11 entryId:2 for timed out msg`, i.e. `p.pendingQueue.Peek()` returned nothing when it arrived.
   
   That is consistent with every chunk being dispatched and acked while the chunk group's callback still never fires, but we haven't isolated it yet.
   
   ### Two corrections to the sketch
   
   1. **The boundary should be `>=`, not `>`.** With `current > s.maxPermits` and two concurrent callers at `current == maxPermits`, both pass the check and both CAS, so `permits` overshoots by `2n`; repeated bursts have no bound. `current >= s.maxPermits` matches the existing `TryAcquire` in the same file and bounds the overshoot to roughly `maxPermits + n` under contention. That isn't a hard limit either — it's still a throttle, like `TryReserveMemory`.
   
   2. **The release side needs `reservedSemaphore`.** `sendRequest` already carries a `reservedSemaphore` field; if `reserveSemaphore` acquires `n` permits, it must record `n` there and each chunk's completion path must decrement it, otherwise every error path that calls `sr.done()` without sending all chunks leaks `n-1` permits permanently. In the batch design the parent's `done()` is the branch that historically doesn't run at all after dispatch.
   
   ### Question
   
   Is there prior art we should be following for throttling a chunk group as a unit — in particular, whether the bound should be the chunk count at all, or whether the chunks should be admitted against the connection's pending-message budget rather than the producer's publish semaphore? We're happy to keep the test at `MaxPendingMessages: 11` and iterate in whatever direction you prefer.


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org