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