+ * Unlike {@link #pushMessage} this bounds the transfer by piece count rather than by the shared + * byte budget, so a single large transfer cannot materialize in the queue in its entirety. + *
+ * IMPORTANT: call this WITHOUT holding {@code WebClientProtocol.lock}. + * + * @param message + * The piece to enqueue. + */ + public void pushChunk(Object message) + { + try + { + chunkPermits.acquire(); + } + catch (InterruptedException e) + { + Thread.currentThread().interrupt(); + return; + } + + pushMessage(message); + } The permit must be released wherever a chunk leaves the queue - both after a successful send and on the failure/teardown paths - or a stalled socket wedges the producer permanently. The cleanest place is beside the existing releaseCapacity(msgSize) call in the send loop, conditional on the message being a chunk. That needs the worker to recognise a chunk; the least invasive way is to wrap the piece in a tiny marker type rather than inspecting msg[0]: + /** A streamed transfer piece; carries a permit that is released once the piece is written. */ + static final class Chunk + { + final ByteBuffer buffer; + + Chunk(ByteBuffer buffer) { this.buffer = buffer; } + } with sizeOf() and the send path unwrapping it, and releaseCapacity's neighbourhood doing chunkPermits.release() for a Chunk. stopPushing() must drain and release outstanding permits too. Then in WebClientProtocol.queueChunk, swap the call: - pushWorker.pushMessage(ByteBuffer.wrap(msg, 0, CHUNK_HEADER_SIZE + payloadLen)); + pushWorker.pushChunk(ByteBuffer.wrap(msg, 0, CHUNK_HEADER_SIZE + payloadLen)); and note that the acquire must happen outside synchronized (lock) - move it to sit with the existing awaitSendCapacity() call at the top of queueChunk rather than inside the block, for the same deadlock reason that comment already gives. At MAX_INFLIGHT_CHUNKS = 4 the peak is ~256 KiB of live buffers instead of the whole payload, and the heartbeat still interleaves freely because pushMessageFirst neither acquires a permit nor waits behind one. 2.2 Cheaper alternative if 2.1 is judged too much machinery ------------------------------------------------------------- Give the streamed path its own, much lower byte threshold rather than a permit count: in queueChunk, replace awaitSendCapacity() with a variant that parks while queuedBytes exceeds, say, 1 MB rather than the 32 MB high-water mark. This needs no marker type and no permit accounting - just a second threshold parameter on awaitCapacity - but it is approximate, because queuedBytes is shared with ordinary sends, so a busy draw batch can make the streamed producer park early. That is harmless (it only throttles a background transfer) and this option is a legitimate 80% fix at 20% of the complexity. Recommendation: take 2.2 unless a measurement shows the transfer footprint actually matters, and record 2.1 as the design if it ever does. The finding is a design-intent regression, not a production symptom, and 2.1 adds a marker type and permit-lifecycle obligations to a class that is already the most concurrency-sensitive in this area. 2.3 Correct the stale javadoc uncovered alongside this -------------------------------------------------------- GuiWebSocket.defineImageStreamed's javadoc parenthetical still says "the send blocks until the whole message is on the wire". The *requirement* it justifies still holds - sendBinaryMessageStreamed drains the InputStream on the calling thread, which is what keeps the FWD context available for the underlying chunk pulls - but the stated reason has not been true since the rewrite. Reword to say the body is consumed on the calling thread, without claiming the send is synchronous. ------------------------------------------------------------------------------------------------ 3. RELATIONSHIP TO OTHER PROPOSALS ------------------------------------------------------------------------------------------------ proposal-issue-critical-functional-1 changes queueChunk's signature and adds a generation check. If both are taken, apply that one first; the permit acquire belongs next to awaitSendCapacity(), before the generation check, and an abandoned transfer must release any permit it holds. Note the interaction explicitly: with 2.1, a transfer abandoned by the generation check returns from queueChunk having acquired a permit it never queues. That permit must be released on that path or repeated reconnects will exhaust the semaphore and hang the next transfer. ------------------------------------------------------------------------------------------------ 4. RISK AND VERIFICATION ------------------------------------------------------------------------------------------------ Risk: moderate for 2.1 - a permit leak on any path that removes a chunk from the queue without releasing (teardown, discardQueued from the critical proposal, an exception in the send loop) turns into a hung producer, which is worse than the footprint it fixes. Low for 2.2, which cannot deadlock because it reuses the existing timed-wait capacity mechanism. Verification 1. Baseline the peak: attach a heap sampler (or log queuedBytes at each queueChunk) while pushing a maximized-window background image. Expected pre-fix: queuedBytes tracks the full payload. Post-fix (2.1): it plateaus at ~4 pieces; (2.2): at ~1 MB. 2. Confirm the transfer still completes and renders correctly, and time it - the fix must not measurably slow the transfer, since the consumer was never the bottleneck's cause. 3. Confirm the heartbeat still goes out during a large transfer (server log shows pings at the configured cadence throughout), i.e. the permit gate did not starve pushMessageFirst. 4. For 2.1 specifically: kill the link mid-transfer, let the session tear down and reconnect, and repeat five times. Confirm the next transfer still starts - this is the permit-leak test and is the one that matters. 5. Confirm custom font loading (the other sendBinaryMessageStreamed caller) is unaffected.