FWD » Core Development » Runtime Infrastructure

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

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.