================================================================================================ PROPOSAL - [MAJOR] performance - WebClientProtocol.startKeepAlivePing Backlog "draining" heuristic tears down healthy, actively-receiving sessions ================================================================================================ REVIEW FINDING (abridged) "the keep-alive tears down a healthy, actively-receiving session whose outbound backlog does not shrink between two consecutive samples. draining is queued > 0 && queued < lastQueueSize against the single previous sample, so a deque that is flat or growing because production outruns a slow link is never 'draining'; after MAX_BACKLOG_STUCK_TICKS the tick falls through and enqueues MSG_SERVER_PING at the *tail* of that same FIFO [...] so once the backlog delay exceeds the deadline the echo can no longer arrive in time. [...] because sendMessage polls before writing, a single large in-flight message samples as queued == 0, so stuckTicks never engages and teardown comes after only 3 ticks (~90 s)." FILES src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java src/com/goldencode/p2j/ui/client/driver/web/PushMessagesWorker.java ------------------------------------------------------------------------------------------------ 1. ROOT CAUSE ------------------------------------------------------------------------------------------------ Two independent defects in the same block (WebClientProtocol.java:1694-1720): (a) Queue depth is the wrong observable. It is a *level*, not a rate, and the code infers progress from a level difference against a single previous sample. A queue that is flat or growing is indistinguishable from a wedged one, even though a flat queue under continuous production is the normal steady state of a slow link. Worse, the level is blind to the write actually in progress: sendMessage() polls the message off the deque before writing (PushMessagesWorker.java:304), so a single large in-flight message samples as queued == 0, stuckTicks resets, and the peer gets no grace at all. (b) The ping shares the FIFO it is supposed to probe. sendBinaryMessage() -> pushMessage() -> offerLast() puts MSG_SERVER_PING behind the entire backlog. Once the queue delay exceeds dataIdleMs the echo *cannot* arrive in time, no matter how healthy the peer is: the probe is self-defeating. On the web CHUI path there is no other inbound traffic to fall back on - ChuiWebSimulator pushes one-way MSG_DRAW per screen flush, and this same revision removed the client's periodic outbound ping (ping() became checkServerSilence(), which sends nothing). Together they contradict the method's own documented contract that "comparing against the previous depth is what keeps a genuinely slow but progressing burst alive indefinitely". ------------------------------------------------------------------------------------------------ 2. PROPOSED FIX ------------------------------------------------------------------------------------------------ 2.1 Measure the rate, not the level ------------------------------------ Delete the lastQueueSize / stuckTicks / draining logic entirely and use the monotonic transmitted-message counter added in proposal-issue-major-functional-1 (PushMessagesWorker. getTransmittedCount()). It is a true progress signal: it advances on every completed write, including the in-flight one that queue depth cannot see, and it freezes on a wedged write. getQueueSize() is kept, but demoted to diagnostics only - it appears in the log lines below and nowhere in the decision. (If it ends up unused after this change, remove it and revision entry 009 in PushMessagesWorker.java with it, rather than leaving an unused public accessor.) 2.2 Send the ping out of band ------------------------------ Give the keep-alive a queue-jumping push so a backlog cannot delay the probe. MSG_SERVER_PING is stateless and is answered by an echo the client handler treats independently of every other message (p2j.socket.js case types.MSG_SERVER_PING), so reordering it ahead of drawing traffic is safe. PushMessagesWorker: + /** + * Add a message at the *head* of the FIFO queue and notify the push thread. + *

+ * Reserved for the keep-alive heartbeat. A probe queued behind a large backlog cannot be + * answered inside its own deadline, which would make the heartbeat report a healthy peer as + * dead; the heartbeat is stateless and independently handled by the client, so overtaking the + * pending output is safe. Do not use it for anything order-sensitive. + * + * @param message + * A binary or a text message. + */ + public void pushPriorityMessage(Object message) + { + if (running) + { + messages.offerFirst(message); + + // notify + synchronized (messages) + { + messages.notify(); + } + } + } WebClientProtocol, next to the other send helpers: + /** + * Send the keep-alive heartbeat, ahead of any pending output. + *

+ * Called from the keep-alive timer thread. The instance lock is taken here so that the + * {@code pushWorker} read observes the same discipline as every other access site. + */ + private void sendKeepAlivePing() + { + synchronized (lock) + { + if (pushWorker != null) + { + pushWorker.pushPriorityMessage(ByteBuffer.wrap(new byte[] { MSG_SERVER_PING })); + } + } + } 2.3 Composed tick body ----------------------- This is the final form of the TimerTask after this proposal, proposal-issue-major-functional-1 (outbound grace) and proposal-issue-minor-security-1 (absolute cap) are applied together: pingTimer = new Timer("websocket-keep-alive", true); pingTimer.schedule(new TimerTask() { /** * The transmitted-message count seen at the previous tick. Owned by this task instance * rather than the enclosing protocol, so that a stale tick of a cancelled timer cannot * perturb the counters of the timer that replaced it. */ private long lastTransmitted; /** * Close the session if the page has stopped echoing the heartbeat, otherwise send one. */ @Override public void run() { try { if (!session.isOpen()) { return; } PushMessagesWorker worker = pushWorker; long transmitted = (worker != null) ? worker.getTransmittedCount() : 0L; int queued = (worker != null) ? worker.getQueueSize() : 0; // progress is measured as completed writes, not as queue depth: a depth is a level, // not a rate, and it is blind to the message already polled off the queue and being // written. A completed write is the same evidence Jetty's notIdle() used to act on, // and it stops dead on a half-open socket. boolean progressing = transmitted != lastTransmitted; lastTransmitted = transmitted; long quietMs = (System.nanoTime() - lastDataNanos) / 1_000_000L; if (quietMs > dataIdleMs) { // outbound progress postpones the deadline, but never removes it: the peer's // kernel accepting our frames does not prove its page still runs code if (progressing && quietMs <= graceLimitMs) { if (LOG.isLoggable(Level.FINE)) { LOG.fine(String.format( "Heartbeat quiet for %d ms (deadline=%d ms) but output is still being " + "delivered (queued=%d); waiting up to %d ms.", quietMs, dataIdleMs, queued, graceLimitMs)); } sendKeepAlivePing(); return; } // either the peer is unreachable or its page no longer runs any code; in both // cases our own pings have kept Jetty's idle timeout from ever reclaiming this LOG.severe(String.format( "No websocket message received for %d ms (deadline=%d ms, grace limit=%d ms, " + "queued=%d, delivering=%b), the peer is gone or its page is no longer " + "running; closing the session!", quietMs, dataIdleMs, graceLimitMs, queued, progressing)); session.close(SESSION_HEARTBEAT_LOST, "client heartbeat stopped", Callback.NOOP); return; } // queue-jumping, so a backlog cannot delay the probe past its own deadline sendKeepAlivePing(); } catch (Throwable t) { // must never propagate: an uncaught exception silently kills the timer thread LOG.warning("Keep-alive ping could not be sent!", t); } } }, interval, interval); MAX_BACKLOG_STUCK_TICKS becomes unused and must be removed; its javadoc is superseded by MAX_BACKLOG_GRACE_MULTIPLIER (proposal-issue-minor-security-1). The method javadoc paragraph claiming "comparing against the previous depth is what keeps a genuinely slow but progressing burst alive indefinitely" must be rewritten - the new mechanism keeps such a burst alive for graceLimitMs, not indefinitely, and that is the honest statement. ------------------------------------------------------------------------------------------------ 3. PERFORMANCE NOTES ------------------------------------------------------------------------------------------------ - No new per-message cost: one volatile long increment per successful send, on the pushworker thread only. - No new allocation on the tick path beyond the existing 1-byte ping buffer. - offerFirst() on ConcurrentLinkedDeque is the same cost as offerLast(). - The scrolling-report / VT100-stream case in the finding no longer closes the session at all: the ping overtakes the backlog, the echo arrives within one RTT, and lastDataNanos is refreshed normally. The entire undelivered backlog is therefore no longer discarded by stopPushWorker(). ------------------------------------------------------------------------------------------------ 4. RELATIONSHIP TO OTHER PROPOSALS ------------------------------------------------------------------------------------------------ proposal-issue-major-functional-1 adds getTransmittedCount() consumed here proposal-issue-minor-security-1 adds graceLimitMs / MAX_BACKLOG_GRACE_MULTIPLIER used here proposal-issue-major-functional-2 supplies dataIdleMs derived from the idle tolerance proposal-issue-minor-functional-3 supplies SESSION_HEARTBEAT_LOST proposal-issue-minor-functional-4 the sendKeepAlivePing() helper also removes the unsynchronized pushWorker read on the send path; the remaining read in the tick still wants the field made volatile ------------------------------------------------------------------------------------------------ 5. VERIFICATION ------------------------------------------------------------------------------------------------ 1. Web CHUI, scrolling report over a throttled link (tc/netem, e.g. 256 kbit + 300 ms), mouse and keyboard idle for >6 ticks. Expected before: session.close(SHUTDOWN) at ~180 s. Expected after: session survives, MSG_SERVER_PING echoes arrive between MSG_DRAW frames. 2. Web GUI graphics-heavy repaint producing a single multi-megabyte in-flight message across >3 ticks. Expected before: teardown at ~90 s (queued samples as 0). After: no teardown. 3. Flat-but-nonempty queue (steady production at exactly link capacity) for 10 ticks: no teardown, no FINE grace logging (the echo keeps arriving). 4. Confirm ordering is unaffected: capture the frames and check MSG_SERVER_PING interleaving does not break MSG_DRAW / draw-hash sequences.