================================================================================ PROPOSAL major-performance-1 discardQueued breaks the queued-bytes accounting and leaks a stale message ================================================================================ Branch : 11645a (task #11645), trunk base r16695 Files : src/com/goldencode/p2j/ui/client/driver/web/PushMessagesWorker.java src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java Severity : reported MAJOR, downgraded to MINOR in the challenge pass (impact is bounded, see section 3); highest-ranked performance finding Category : defect introduced by this branch 1. PROBLEM -------------------------------------------------------------------------------- PushMessagesWorker.discardQueued() (:332-341) mutates the message deque and the queuedBytes counter with no mutual exclusion against the worker thread's in-flight send. onConnect holds WebClientProtocol.lock; the worker holds only sendLock. Disjoint monitors, so there is no exclusion at all. Two consequences: (a) NEGATIVE ACCOUNTING. sendMessage captures long msgSize = sizeOf(message); // :556 ... releaseCapacity(msgSize); // :672 -> addAndGet(-msgSize) If queuedBytes.set(0L) (:335) lands between those two points, the counter settles at -msgSize. Nothing floors it - the only writers are +sizeOf (:226, :317), -msgSize (:352) and set(0) - so the offset persists until the next discardQueued. Both awaitCapacity variants (:249, :256, :282, :289) compare queuedBytes.get() against positive limits, so the effective high- and low-water marks are raised by the offset. The mirror case also exists: pushMessage does addAndGet(+size) at :226 BEFORE offerLast at :227, so an offer landing after clear() leaks the bytes positive, permanently shrinking headroom until the next discard. (b) A STALE MESSAGE SURVIVES THE DISCARD. On the send-failure path, messages.offerFirst(message); // :666 runs AFTER messages.clear(). So one message built for the previous connectionGeneration survives, sits at the head of the queue, and is the first thing written to the fresh socket - ahead of the resync. The comment at :665 ("its bytes remain accounted for") is false once a discard intervened. This is precisely the invariant queuePartial's generation check (WebClientProtocol.java:1063-1067) exists to enforce. Trigger: onConnect's session-replacement branch firing while the push worker is inside sendMessage - the same page-reload / half-open-socket race the discardQueued() call was added for. The window is wide and is the expected state: onConnect closes the old session at :463 while the worker is parked in latch.await() on that half-open socket. The counter survives the reconnect because startPushWorker (:2017) reuses the existing worker and the replaced session's onClose takes the session != this.session branch (:809), skipping stopPushWorker(). 2. WHY IT IS NOT WORSE THAN IT LOOKS -------------------------------------------------------------------------------- Recorded so the fix is not over-engineered. The original report claimed the skew is "permanently negative and cumulative across reconnects". It is not: - There is exactly one worker thread, so at most one msgSize can race per discard. - Every subsequent discardQueued re-bases the counter with an absolute set(0). - Magnitude is bounded: streamed transfers - the only large payloads - are chunked to PARTIAL_PAYLOAD_SIZE = 64 KB (WebClientProtocol.java:324), and ordinary drawing / hoverable batches are KB-scale. So the 32 MB guard is perturbed by tens of KB, not progressively raised. Consequence (b) is also largely absorbed by the peer: collectChunk drops an orphaned piece whose PARTIAL_FLAG_FIRST is unset ("Discarding orphaned message piece", p2j.socket.js:3205-3212) and handleOpenEvent calls resetChunkTransfers() (:5712). Residual exposure is a stale non-partial message rendered ahead of the resync, or a stale FIRST piece opening a transfer that never completes. That is why this is MINOR, not MAJOR. It is still worth fixing: the counter is the only memory guard on the push queue, and a silently wrong counter is the kind of defect that only shows up as an OOM under load months later. 3. RELATIONSHIP TO proposal-major-functional-2 -------------------------------------------------------------------------------- functional-2 recommends removing the discardQueued() call from onConnect entirely, because the GUI never resyncs after it. If that is done, this defect becomes unreachable from onConnect - but the method stays broken and will bite the next caller. Fix or remove it in the same change. Settle functional-2 first, then apply whichever branch below matches the decision. 4. PROPOSED FIX -------------------------------------------------------------------------------- IF discardQueued() IS REMOVED (functional-2 recommended option): Delete the method along with its only call site, so the broken accounting cannot be reintroduced. Nothing further needed. IF discardQueued() IS RETAINED (functional-2 alternative B, or a future caller): Fix 1 - drain and subtract instead of set(0). Only ever subtract what was actually removed, which fixes both the negative skew and the positive leak in one stroke: Object msg; while ((msg = messages.poll()) != null) { queuedBytes.addAndGet(-sizeOf(msg)); } The in-flight message the worker already polled is never in the deque, so its bytes are never subtracted here - and the worker's own releaseCapacity(msgSize) remains correct. The pushMessage(+size before offerLast) ordering also becomes harmless: an offer that lands after the drain leaves both the entry and its bytes present, which is consistent. Fix 2 - do not re-queue a message from a superseded generation. Give the worker the generation it polled under and drop the message on the failure path when the generation has moved on: if (!msgSent) { if (generation == callbacks.currentGeneration()) { messages.offerFirst(message); // bytes still accounted } else { releaseCapacity(msgSize); // superseded: drop and account } } If threading a generation through is unwelcome, a cheaper equivalent is a volatile discardEpoch incremented by discardQueued(); the worker captures it before the send and skips the offerFirst if it changed. Either way the rule is: a message must not survive a discard. Fix 3 - correct the comment at :665, which currently asserts an invariant the code does not maintain. Fix 4 (assertion, optional but cheap) - after the drain, assert or log at FINE when queuedBytes.get() is negative. The counter should never be negative; making that visible turns any future recurrence into a log line instead of a mystery. 5. ADJACENT CHEAP WIN - NOT PART OF THIS DEFECT -------------------------------------------------------------------------------- Surfaced while verifying the same code path, one-line fix, worth folding into the same commit: PARTIAL_PAYLOAD_SIZE is 64 * 1024 (WebClientProtocol.java:324), so each piece is 6 + 65536 = 65542 bytes on the wire. Jetty 12.0.34 defaults DEFAULT_MAX_FRAME_SIZE = 65536 with DEFAULT_AUTO_FRAGMENT = true, and FWD calls neither setMaxFrameSize nor setAutoFragment. Every piece therefore crosses the frame boundary by exactly 6 bytes and is auto-fragmented into a full 65536-byte frame plus a 6-byte continuation frame - roughly 500 extra near-empty frames per 4K image. Fix: make the payload (64 * 1024) - PARTIAL_HEADER_SIZE so a piece is exactly one frame. Separately, the choice of 64 KB itself is worth revisiting: the pre-change path used ChunkTransferDaemon.getChunkSizeCached() (1 MB default) with a single reused buffer, and nothing forces 64 KB - the server's setMaxBinaryMessageSize is inbound-only and defaults to -1, the browser's maxBinaryMessage governs only client-to-server pieces, and collectChunk has no per-piece cap. A larger piece reduces the per-piece cost (a lock acquisition on the same monitor as GuiWebEmulatedWindow.offer, two CAS ops, a deque node, a monitor notify, a latch + Callback + park/unpark round trip) by the same factor. The stated constraint - a piece must stay well under the echo deadline of 3 ping intervals - is met by a far larger piece: 1 MB only breaches a 90 s deadline below ~93 KB/s. That is a tuning decision, not a defect; raise it with the team rather than changing it unilaterally. 6. RISK -------------------------------------------------------------------------------- Low. Fix 1 is strictly more conservative than set(0) - it cannot subtract bytes that were not removed. Fix 2 changes what happens to one message on a failure path that only runs during a reconnect. Watch for: sizeOf(msg) must be the same function used at enqueue time, or the drain introduces its own skew. Verify there is exactly one sizeOf implementation. 7. TEST PLAN -------------------------------------------------------------------------------- 1. Instrument queuedBytes (a FINE log or a JMX counter) and confirm it returns to exactly 0 when activity settles - before any reconnect, after a clean reconnect, and after a half-open-socket reconnect during heavy drawing. 2. Force the race deliberately: hold the worker in sendMessage (breakpoint or an injected delay on the dead socket) and drive onConnect's replacement branch. Confirm the counter is non-negative and consistent afterwards. 3. Confirm no stale pre-reconnect message is rendered on the new socket ahead of the resync (drive a distinguishable draw op just before the outage). 4. Load test: sustained large-image streaming across repeated forced reconnects; watch for the high-water guard still engaging at the configured 32 MB. 5. If the adjacent frame-size fix is taken: confirm on the wire (browser devtools frame view or a Jetty frame log) that a piece is one frame, not two.