FWD » Core Development » Runtime Infrastructure

================================================================================

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).