lhotari commented on PR #4730: URL: https://github.com/apache/bookkeeper/pull/4730#issuecomment-4878839966
**Together with the `maxReadsInProgressLimit` default change from 0 → 10000 in this same PR, this change can permanently deadlock the entire read path of a bookie.** The root cause is an interaction between this change and the server-side backpressure feature from #1410 (extended to the v2 protocol by #3324). ### Background: the #1410 admission design blocks a Netty event loop thread `maxReadsInProgressLimit` is enforced in [`BookieRequestProcessor.onReadRequestStart`](https://github.com/apache/bookkeeper/blob/4a012a2a1e984333c65834caddb1639158ce6bac/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java#L236-L254): ```java if (!readsSemaphore.tryAcquire()) { ... channel.config().setAutoRead(false); ... readsSemaphore.acquireUninterruptibly(); // blocks the calling thread channel.config().setAutoRead(true); ... } ``` It is called from the read processor constructors/factories ([`ReadEntryProcessor.create`](https://github.com/apache/bookkeeper/blob/4a012a2a1e984333c65834caddb1639158ce6bac/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java#L54), [`ReadEntryProcessorV3` constructor](https://github.com/apache/bookkeeper/blob/4a012a2a1e984333c65834caddb1639158ce6bac/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java#L57)), which [`processReadRequest`](https://github.com/apache/bookkeeper/blob/4a012a2a1e984333c65834caddb1639158ce6bac/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java#L676-L699) runs on the Netty IO thread *before* submitting the request to the read thread pool. When `acquireUninterruptibly()` blocks, an entire event loop is parked. Every channel registered on that loop is affected: no requests are decoded (reads or writes, v2 or v3), no responses are flushed, no write futures com plete, no future listeners run, and no channel closes are processed. That is the latent hazard in this design: **releasing a permit requires event-loop progress (flushing a response), while acquiring a permit can block the event loop.** It stayed latent for years because `maxReadsInProgressLimit` defaulted to 0, so the semaphore was never created. ### What changed in this PR Two things: 1. The [`getMaxReadsInProgressLimit()`](https://github.com/apache/bookkeeper/blob/4a012a2a1e984333c65834caddb1639158ce6bac/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java#L1089-L1091) default changed from 0 to 10000, arming the semaphore and the autoRead gate on every deployment. Since [`readWorkerThreadsThrottlingEnabled`](https://github.com/apache/bookkeeper/blob/4a012a2a1e984333c65834caddb1639158ce6bac/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java#L2135-L2137) also defaults to `true`, the throttled response path is active by default as well. 2. The permit is now released only from the `writeAndFlush` future listener in [`sendResponseAndWait`](https://github.com/apache/bookkeeper/blob/4a012a2a1e984333c65834caddb1639158ce6bac/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java#L155-L186): ```java ChannelFuture future = channel.writeAndFlush(response); future.addListener((ChannelFutureListener) f -> { ... processor.onReadRequestFinish(); // the only release }); ``` Unlike the sibling [`sendResponse()`](https://github.com/apache/bookkeeper/blob/4a012a2a1e984333c65834caddb1639158ce6bac/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java#L84-L110) used by the non-throttled path — which handles non-writable channels (wait up to `waitTimeoutOnBackpressureMillis`, blacklist, or drop the response) and always lets [`sendReadReqResponse`](https://github.com/apache/bookkeeper/blob/4a012a2a1e984333c65834caddb1639158ce6bac/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java#L73-L82) release the permit synchronously — `sendResponseAndWait` has no non-writable-channel handling. If the remote peer stops draining responses (TCP zero window), `writeAndFlush` parks the message in the `ChannelOutboundBuffer` and the returned future can stay pending indefinitely: the listener never fires, and the permit is never released. Before this PR, the blocking `future.get()` capped un-flushed responses at `numReadWorkerThreads`. Now the number of in-flight un-flushed responses is bounded only by the semaphore itself — whose release depends on those very flushes completing. ### The deadlock 1. The semaphore exhausts under load, by either route: **(a) slow reads** — permits are acquired at ingest, before the request is submitted to the read thread pool, so the executor backlog alone reaches the limit. With defaults the semaphore is the binding constraint: 10000 permits vs. `numReadWorkerThreads` (8) × `maxPendingReadRequestPerThread` (10000) = 80000 queue slots, so it trips long before `ETOOMANYREQUESTS` load shedding would. **(b) slow clients** — permits are held by responses stuck in the outbound buffers of open-but-non-writable channels, whose write futures never complete. 2. The next read arriving on each event loop fails `tryAcquire()` and parks that loop in `acquireUninterruptibly()`. 3. Read worker threads keep completing reads, but `writeAndFlush` from an off-loop thread only enqueues a flush task on the response channel's (parked) event loop. Neither route can release a permit anymore: flush tasks are frozen on parked loops, pending futures never complete, and even client disconnects can no longer be processed. 4. Once every IO thread has parked, no permit can ever be released, and the entire request path stalls — including writes, since parked loops can't decode any request type. `READ_ENTRY_IN_PROGRESS` stays pinned at `maxReadsInProgressLimit` with read throughput at zero until the bookie is restarted, and the bookie can still *appear* healthy: the HTTP admin/metrics endpoints are served from separate threads and don't exercise the data path. Reproducing should be straightforward: set `maxReadsInProgressLimit` to a small value, connect a v2-protocol client, issue more concurrent reads than the limit, and have the client stop reading from its socket. A sustained cache-missing read backlog reaches the limit the same way without any client misbehavior. ### Why this change doesn't achieve its own goal either The aim of this PR was to stop read worker threads from blocking in `future.get()` (the p99 spike). But with the semaphore now on by default, the blocking moves somewhere worse: under permit exhaustion the Netty IO threads block, stalling every channel on their event loops and making permit release impossible. A latency spike is traded for a permanent, unrecoverable read-path deadlock. Note that the throttle's own designed trigger condition — a sustained read backlog — is by itself sufficient to set up the deadlock; no client misbehavior is required. Scope notes: - The permit leak is specific to the v2 wire protocol (the `sendReadReqResponse` throttle branch). The v3 path releases the permit synchronously after a non-blocking write ([`ReadEntryProcessorV3.sendResponse`](https://github.com/apache/bookkeeper/blob/4a012a2a1e984333c65834caddb1639158ce6bac/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java#L339-L346)) and cannot leak this way — a pure-v3 overload parks event loops only transiently and recovers, because v3 releases don't depend on event-loop progress. However, the semaphore and the blocking admission are shared: once v2 permits are stuck, v3 reads also park event loops, and every channel on a parked loop stalls regardless of protocol or operation type. Note that Pulsar brokers use the v2 protocol by default (`bookkeeperUseV2WireProtocol=true`). - Affected: master, released 4.18.0, and branch-4.17 — the pending 4.17.4 release would ship this. ### Mitigations and fix direction Operators can break the loop with either `maxReadsInProgressLimit=0` (the previous default) or `readWorkerThreadsThrottlingEnabled=false` (routes responses through `sendResponse()`, which always releases the permit). For a proper fix, I think two things are needed: 1. **Never block an event loop on the semaphore.** Minimal version: move the acquire off the Netty thread, e.g. into the processor's execution on the read worker (keeping acquire/release strictly paired — the current `RejectedExecutionException` path releases a permit acquired at creation). Better: make admission asynchronous — on `tryAcquire` failure, disable autoRead and queue the request with a timeout, replying `ETOOMANYREQUESTS` on timeout, so no thread ever blocks. Pulsar has a non-blocking semaphore implementation that fits this shape: [`AsyncSemaphore`](https://github.com/apache/pulsar/blob/master/pulsar-common/src/main/java/org/apache/pulsar/common/semaphore/AsyncSemaphore.java) / [`AsyncSemaphoreImpl`](https://github.com/apache/pulsar/blob/master/pulsar-common/src/main/java/org/apache/pulsar/common/semaphore/AsyncSemaphoreImpl.java) — `CompletableFuture`-based acquire with a timeout, a bounded waiter queue, a cancellation hook (e.g. for closed channels), and idempo tent permit release, which makes it safe to wire the release into both the write-future listener and a channel-close listener. Being ASF/Apache-2.0 code, it could be adapted into BookKeeper. 2. **Guarantee `onReadRequestFinish()` runs exactly once even if the write future never completes** — non-writable-channel handling like `sendResponse()` has, and/or a channel-close/timeout-bound release. Until both are in place, I'd suggest reverting the `maxReadsInProgressLimit` default to 0 — in particular for the pending 4.17.4 release. -- 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]
