Repository navigation
fix(sio-client): discard a volatile packet when the namespace is not connected - #5621
Open
mark1russell7 wants to merge 2 commits into
Open
mark1russell7 wants to merge 2 commits into
mark1russell7 wants to merge 2 commits into
Conversation
…connected A volatile packet was discarded only when the transport was not writable. During the handshake of a namespace (for example a second namespace on a connected Manager, or right after a reconnection), the transport is writable but the socket is not connected yet, so the packet was buffered and sent once connected. The documentation of the volatile flag says the event "will be dropped when this socket is not ready to send messages".
…able again Right after the CONNECT packet of the namespace, the transport is not writable, so the volatile packet was discarded even without the fix. The test now uses a namespace that delays its acknowledgement, and emits the packet once the transport is writable again.
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.
The kind of change this PR does introduce
Current behavior
A volatile packet is discarded only when the transport is not writable. When the transport is writable and the socket is not connected yet (the handshake of a namespace on a connected Manager, or after a reconnection), the packet goes into
sendBufferand the client sends it after theconnectevent.The documentation of the
volatileflag says that the event "will be dropped when this socket is not ready to send messages".New behavior
The client discards a volatile packet when the transport is not writable or the socket is not connected.
A test is added in
test/socket.ts("should discard a volatile packet when the namespace is not connected yet"). It uses a new namespace/delayedintest/support/server.ts, whose middleware answers after 100 ms. The test checks that the transport is writable and the socket is not connected before it emits.The tests ran in GitHub Actions (Node.js 24.22.2 on Linux): the new tests with the
libfolder of upstream main (before), then with this change (after), thennpm testof socket.io-client with this change:The failures before the change:
Other information (e.g. related issues)
Closes #5614
This PR and #5622 add tests at the same place in
test/socket.ts. When one is merged, I will update the other.This PR was prepared with AI assistance (Claude). The tests above were run before and after the change, with the results above.
🤖 Generated with Claude Code