================================================================================ PROPOSAL major-functional-2 onConnect discards the whole push backlog, but the GUI driver never resyncs ================================================================================ Branch : 11645a (task #11645), trunk base r16695 Files : src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java src/com/goldencode/p2j/ui/client/driver/web/PushMessagesWorker.java src/com/goldencode/p2j/ui/client/gui/driver/web/GuiWebDriver.java Severity : MAJOR (functional) - confirmed in the challenge pass Category : regression introduced by this branch 1. PROBLEM -------------------------------------------------------------------------------- WebClientProtocol.onConnect:480-486 now calls pushWorker.discardQueued(), throwing away all pending server-to-browser output. The in-code justification is: "initRemoteClient below resyncs" That is true for CHUI and false for GUI. GuiWebDriver.initRemoteClient() is literally a no-op: GuiWebDriver.java:1033-1036 // no-op Only ChuiWebSimulator.initRemoteClient() resyncs (clearCache(), sendColorPalette(), sendSwitchMode(), triggerRepaint(true) - ChuiWebSimulator.java:200-215). So on the GUI driver every draw batch produced during an outage is silently dropped and nothing regenerates it. The canvas is left permanently stale. 2. EVIDENCE / TRIGGER -------------------------------------------------------------------------------- Trigger: a half-open socket. The browser's silence watchdog (checkLostPings / onServerSilenceExpired -> attemptToRestoreConnection() -> connect() -> new WebSocket(url)) reconnects before Jetty delivers onClose for the dead session, so onConnect takes its session-replacement branch: WebClientProtocol.java:464 if (this.session != null && this.session.isOpen()) with pushWorker != null and a full backlog - the worker is parked in latch.await() on the dead socket while producers keep enqueuing up to the 32 MB high-water mark. Two facts make this specifically bad rather than merely lossy: a) The discard has effect ONLY on this path. On a first connect pushWorker is null; after a real onClose, stopPushWorker() nulls it. So on every other reconnect discardQueued() is a no-op. The change therefore takes effect exactly where it is unsafe. b) The page was never reloaded, so the browser sends MSG_PING_PONG, not MSG_PAGE_LOADED (p2j.socket.js:5762). Nothing else requests a repaint. The canvas is intact and stale, and there is no path that regenerates the lost drawing operations. Pre-change behaviour: the backlog survived and drained onto the new socket via startPushWorker -> setCurrentSession (which clears sessionInvalid and releases the worker parked in getCurrentSession); failed sends were re-queued at the head (PushMessagesWorker.java:659-668). 3. ROOT CAUSE -------------------------------------------------------------------------------- The discard was added to protect against cross-generation MSG_PARTIAL tails arriving on a new connection. But that hazard is already handled elsewhere: - queuePartial's generation check (WebClientProtocol.java:1063-1067) refuses to enqueue a piece whose generation no longer matches; - the client calls resetChunkTransfers() in handleOpenEvent, and collectChunk drops any piece arriving without PARTIAL_FLAG_FIRST. So discarding *whole* messages buys no safety that is not already provided, while costing all pending GUI output. Note also that pre-change the queue held only whole, self-contained messages - streamed transfers went through session.sendPartialBinary under sendLock, bypassing the queue - so replaying the retained backlog was well-formed. Routing MSG_PARTIAL through the same queue is what created the appearance of a new hazard. 4. PROPOSED FIX -------------------------------------------------------------------------------- RECOMMENDED - drop the discard, keep the generation guard. Remove the pushWorker.discardQueued() call from onConnect and restore the pre-change retain-and-replay behaviour, relying on the mechanisms in section 3 for partial-transfer safety. Concretely, in onConnect's session-replacement branch: - delete the discardQueued() call and the misleading "initRemoteClient below resyncs" comment; - replace the comment with one stating why the backlog is retained: whole messages are connection-agnostic, and stale partial pieces are already excluded by the generation check in queuePartial plus resetChunkTransfers() on the client. Keep discardQueued() as a method only if another caller needs it; otherwise remove it so it cannot be reintroduced casually. If it is kept, its accounting race must be fixed first - see proposal-major-performance-1, which is a prerequisite for any retained use of this method. ALTERNATIVE A - keep the discard, make the GUI actually resync. Implement GuiWebDriver.initRemoteClient() to force a full repaint (invalidate all windows and retransmit). This is the semantically complete option but it is considerably larger, it makes every stale-session reconnect pay a full-screen repaint, and drawing ops are not the only queued state (font definitions, streamed images, cursor state) - so a repaint alone may not be sufficient. Only worth doing if a resync is wanted for its own sake. ALTERNATIVE B - discard selectively. Tag each queued message with the connectionGeneration at push time and have discardQueued(generation) remove only stale MSG_PARTIAL pieces, retaining whole messages. This preserves the stated intent with no output loss, at the cost of one int per queue entry. Reasonable if the team wants belt-and-braces on top of the existing generation guard. Recommendation: the RECOMMENDED option. It is the smallest change, restores known- good behaviour, and the hazard it removes protection against is already covered twice over. ALTERNATIVE B is the fallback if reviewers want an explicit discard retained. 5. RELATED DEFECT THAT MUST BE SETTLED TOGETHER -------------------------------------------------------------------------------- discardQueued() also has an accounting race and re-queues one stale message after clearing the deque (see proposal-major-performance-1). If the RECOMMENDED option is taken, that defect becomes unreachable from onConnect but the method remains broken; fix or remove it in the same change so it is not reintroduced later. 6. RISK -------------------------------------------------------------------------------- Low-to-moderate. Removing the discard restores the behaviour that shipped before this branch, so the risk profile is "back to known state" rather than new. Watch for: a stale message at the head of the queue being written to the new socket ahead of anything the reconnect sends. That is pre-existing behaviour and was correct before, but confirm it still is now that MSG_PARTIAL shares the queue - specifically that a partial piece enqueued under the old generation cannot be at the head when the new session starts draining. The generation check is at enqueue time, so a piece enqueued just before the bump can still be queued; verify the client's orphan-piece drop absorbs it (it should - the piece will lack PARTIAL_FLAG_FIRST from the new page's point of view after resetChunkTransfers()). 7. TEST PLAN -------------------------------------------------------------------------------- 1. GUI web client. Force a half-open socket (drop packets on the websocket port with the process still alive, or SIGSTOP the browser tab's network) long enough for the client watchdog to reconnect but short of Jetty's idle close. Drive drawing activity throughout. Expect after reconnect: the canvas is current, no missing regions. 2. Same scenario on the CHUI web client. Expect: unchanged behaviour (it resyncs via triggerRepaint either way). 3. Reconnect while a streamed image (defineImageStreamed) is mid-transfer. Expect: no corrupt image, no orphaned transfer, either a clean re-send or a dropped transfer - but never a partially applied one. 4. Reconnect while a custom font is being streamed. Expect: no hung drawing thread (this is the MINOR queuePartial/createFont finding; verify it did not get worse). 5. Confirm queuedBytes returns to 0 after activity settles, both before and after a reconnect (guards against the accounting skew in performance-1).