FWD » Core Development » Runtime Infrastructure

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

PROPOSAL - [MAJOR] functional - p2j.socket.js handleCloseEvent

resetChunkTransfers() is unreachable on every client-initiated close

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

REVIEW FINDING (abridged)

   "resetChunkTransfers() is only reachable on the this == ws path [...] closeWebSocketSafely calls

   ws.close(...) and then sets ws = null synchronously while the close event is delivered

   asynchronously afterwards [...] the incomplete chunkTransfers entries and their chunkBytes

   accounting survive into the next socket permanently."

FILES

   src/com/goldencode/p2j/ui/client/driver/web/res/p2j.socket.js

------------------------------------------------------------------------------------------------

1. ROOT CAUSE

------------------------------------------------------------------------------------------------

handleCloseEvent (p2j.socket.js:5868) opens with

   if (this != ws)

   {

      ... log ...

      if (!ws) { socketOpenedAt = 0; }

      return;                                    // <-- returns here

   }

   ...

   socketOpenedAt = 0;

   resetChunkTransfers();                        // <-- :5889, only on the this == ws path

   attemptToRestoreConnection();

closeWebSocketSafely (:6231) does

   ws.close(ceCodes.Normal_Closure, "Closing websocket due to " + reason);

   ws = null;

synchronously, and never detaches ws.onclose. The close event is delivered later, by which time

this is the dying socket and ws is null (or already its replacement), so the handler takes the early

return and the chunk state is never cleared.

That the branch already special-cases exactly this situation for a different variable - the

socketOpenedAt = 0 with its comment "a client-initiated close nulled ws before this event landed" -

shows the path is known-live and that the new cleanup was simply added to the wrong side of it.

Reachability is not in question: handleMessageEvent zeroes lostPings on every inbound message, so

checkLostPings can only fire once pieces have *stopped* arriving - i.e. a stalled link during a

large image or font push, which is exactly when a transfer is incomplete. onServerSilenceExpired

and createWebSocket's own closeWebSocketSafely reach the same path.

Two consequences:

   (1) An unconditional leak. The pieces are never freed and chunkBytes never decreases for them, so

       repeated mid-transfer reconnects ratchet it upward until the 64 MB MAX_CHUNK_BYTES cap trips

       inside collectChunk - which then wipes the pieces of the *in-flight, healthy* transfer too

       and lets its remainder assemble into a tail-only message. A leak in the failure path thus

       corrupts an unrelated later transfer.

   (2) A same-id join across the reconnect. onClose calls stopPushWorker() while holding lock, and

       stopPushing() joins for up to WAIT_TO_DIE = 5000 ms; a producer woken from awaitCapacity()

       blocks on lock inside queueChunk for that window, during which the browser's reconnect can

       bring onConnect onto the same unfair monitor. If onConnect wins, the producer's next

       queueChunk succeeds on the new socket with the old transfer id, and the pieces the dead

       worker dropped become a silent gap in the middle of a message that still reassembles under a

       valid leading type byte.

------------------------------------------------------------------------------------------------

2. PROPOSED FIX

------------------------------------------------------------------------------------------------

Moving the call is necessary but NOT sufficient - see 2.3.

2.1 Clear the chunk state where the socket is actually abandoned

------------------------------------------------------------------

closeWebSocketSafely is the single funnel for client-initiated closes, and it is the point at which

the page decides the pieces can never be completed. Put the cleanup there:

      function closeWebSocketSafely(reason)

      {

         try

         {

            if (ws && ws.readyState != wsStates.CLOSED && ws.readyState != wsStates.CLOSING &&

                ws.readyState != wsStates.CONNECTING)

            {

               ws.close(ceCodes.Normal_Closure, "Closing websocket due to " + reason);

               ws = null;

   +           // the close event will land with ws already nulled and take handleCloseEvent's

   +           // this != ws early return, so the pieces have to be dropped here

   +           resetChunkTransfers();

            }

         }

Keep the existing call in handleCloseEvent (:5889): it covers the transport-death path, where

closeWebSocketSafely never runs.

Note the guard in closeWebSocketSafely skips a CONNECTING socket, so a close requested while the

socket is still opening leaves ws non-null and the cleanup unrun - correct, because that socket

never carried any pieces.

2.2 Also clear it on a fresh connection, as a backstop

--------------------------------------------------------

Any path that reaches a new socket without either cleanup having run still starts clean:

   in handleOpenEvent (:5669), with the other per-socket resets

           connected = true;

           lostPings = 0;

   +       resetChunkTransfers();

           socketOpenedAt = (new Date()).getTime();

This is cheap and makes the invariant "a new socket begins with no partial transfers"

unconditional, rather than dependent on which of three close paths ran.

2.3 REQUIRED companion: reject a reassembly that does not start at a first piece

----------------------------------------------------------------------------------

Consequence (2) above is not addressed by clearing state - it is the opposite problem. After a

reset, the surviving suffix of the old transfer arrives at an *empty* table and starts a brand-new

reassembly, which is precisely the arbitrary-first-byte hazard. The remedy is the CHUNK_FLAG_FIRST

check specified in proposal-issue-critical-functional-1 section 2.2:

   if (!parts)

   {

      if (!first) { drop and log; return null; }

      parts = chunkTransfers[id] = [];

   }

Do not land 2.1/2.2 as a standalone "fix" for this finding: on their own they convert a corrupt

message into a differently corrupt message. The pair (2.1 + first-piece flag) is what makes the

close path safe.

------------------------------------------------------------------------------------------------

3. RELATIONSHIP TO OTHER PROPOSALS

------------------------------------------------------------------------------------------------

   proposal-issue-critical-functional-1   supplies CHUNK_FLAG_FIRST and the server-side generation

                                          check.  Hard prerequisite for 2.3; its section 2.3

                                          (discardQueued on session replacement) also removes the

                                          leak's main source.

   proposal-issue-minor-functional-1      the MAX_CHUNK_BYTES abandon path that consequence (1)

                                          feeds into; fixing this leak makes that cap far harder to

                                          reach in practice.

------------------------------------------------------------------------------------------------

4. RISK AND VERIFICATION

------------------------------------------------------------------------------------------------

Risk: low. resetChunkTransfers() is idempotent and touches only the two chunk variables, so calling

it from three places is harmless. The only judgement call is 2.1's placement inside the readyState

guard rather than outside it; outside would also clear when a CONNECTING socket is abandoned, which

is equally correct but marginally more surprising to read.

Verification

   1. Instrument collectChunk to log chunkBytes on every piece. Start a large image transfer, kill

      the link mid-transfer so checkLostPings closes the socket, let it reconnect, and repeat five

      times. Expected: chunkBytes returns to 0 after each cycle. Pre-fix it climbs monotonically.

   2. Repeat until the pre-fix build crosses MAX_CHUNK_BYTES and logs "Abandoning incomplete message

      transfers"; confirm the fixed build never reaches it.

   3. With the first-piece check in place, confirm the page logs "Discarding orphaned message piece"

      rather than dispatching, for any suffix that does slip through on the new socket.

   4. Confirm the ordinary transport-death path (server process killed) still clears state, i.e.

      the handleCloseEvent call was not removed by mistake.

   5. Confirm a normal logout still works - closeWebSocketSafely is on that path too, and the added

      call must not disturb the "logout" reason string that WebClientProtocol.onClose matches on.