fix(adapter): do not let volatile packets break connection state recovery - #5539
Open
chuanghiduoc wants to merge 1 commit into
Open
fix(adapter): do not let volatile packets break connection state recovery#5539chuanghiduoc wants to merge 1 commit into
chuanghiduoc wants to merge 1 commit into
Conversation
…very The client stores the offset of every received event: it takes the last item of the data array (see emitEvent() in the client). Volatile EVENT packets did not get an offset appended, so if such a packet ended with a string argument, the client stored that string as its offset. The next restoreSession() call then failed to find it and the whole session was dropped, silently discarding every packet emitted during the outage. Volatile packets are now part of the offset chain like any other EVENT packet, but they are still excluded from the replay in restoreSession(), so their semantics are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
connectionStateRecoveryis enabled, a volatile event whose last argument is a string breaks the recovery of the client that received it.Reproduction (server + client from current main):
The client then disconnects and reconnects:
socket.recoveredisfalse, and every packet emitted during the disconnection is silently lost.With the same scenario minus the
io.volatile.emit(...)line,socket.recoveredistrue.Cause
The two sides of the offset mechanism disagree about volatile packets:
SessionAwareAdapter.broadcast()): a volatile EVENT packet gets no offset appendedemitEvent()): any trailing string argument of a received event is stored as the offsetSo after
io.volatile.emit("status", "connected"), the client stores the literal string "connected" as its offset. On reconnection,restoreSession()doesfindIndex(packet.id === "connected"), which fails, and the session is dropped.This also affects the cluster adapters, which share the same condition in
addOffsetIfNecessary(). I have deliberately not touched them here, because their packet storage/replay lives in the external adapter packages and needs to be changed in sync.Fix
In the in-memory adapter, volatile EVENT packets are now part of the offset chain like any other EVENT packet (they get an offset, and they are stored), but they are excluded from the replay in
restoreSession(). Their semantics are unchanged - a volatile packet is still never delivered late.Testing
packages/socket.io-adapter/test/index.ts: session restore still succeeds with the volatile packet's offset, and the volatile packet is still excluded frommissedPacketsnpm testinpackages/socket.io-adapter: 49 passing (48 before)packages/socket.io: 216 passingI also verified end-to-end with a script against the built packages: before the fix
recovered: false/ no replay; after the fixrecovered: true/ non-volatile packets replayed in order / volatile packets not replayed.