fix(eio,eio-client): handle close packets consistently across transports#5525
Open
GiHoon1123 wants to merge 1 commit into
Open
fix(eio,eio-client): handle close packets consistently across transports#5525GiHoon1123 wants to merge 1 commit into
GiHoon1123 wants to merge 1 commit into
Conversation
The polling transport already closed the connection upon receiving a
"close" packet, but the WebSocket transport silently ignored it and kept
the connection open, on both the server and the client. This is
inconsistent with the Engine.IO protocol, where "close" is a formal packet
type that any conformant implementation may send over any transport.
Add the same check to the shared base Transport#onData(), on both sides
(this also covers the uWebSockets.js-based server transport, which relies
on the same base method). On the client, match the polling transport's
existing close details ("transport closed by the server") for consistency.
Both fixes call the transport's real close procedure (not just the
internal "closed" state), so the underlying WebSocket connection is
actually torn down instead of being left open once the Engine.IO layer
considers it closed.
Fixes socketio#5286
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.
Summary
Fixes #5286.
The polling transport already closes the connection when it receives a "close" packet, but the WebSocket transport silently ignored it and kept the connection running - on both the server and the client. This is inconsistent with the Engine.IO protocol, where
closeis a formal packet type (independent of the underlying WebSocket connection's own close frame), so any conformant implementation may send it over any transport.Approach
Added the same check to the shared base
Transport#onData()in bothengine.ioandengine.io-client, rather than duplicating polling's own override:onData()."transport closed by the server"), for consistency between transports.Both fixes call the transport's real close procedure (
close()/doClose()+onClose()), not just the internal state transition - an earlier version of this fix calledonClose()directly (mirroring polling's own shortcut), but that leaves the underlying WebSocket connection open indefinitely, since polling's shortcut is safe only because each poll is a short-lived HTTP request that Node's HTTP server cleans up on its own, which doesn't apply to a persistent WebSocket connection.Out of scope
While investigating, two related gaps were found but intentionally left out of this PR to keep it focused on the reported issue:
WebTransporttransport has the same underlying gap (it decodes and dispatches packets directly, bypassingonData()entirely), but it isn't mentioned in the issue and is disabled by default. Happy to follow up separately if maintainers want it addressed.WebTransportabove.Test plan
git stashon just that file) and confirming the corresponding new test fails without it, then restoring and reconfirming it passesengine.iosuite passes for both protocol v4 and legacy v3 (EIO_CLIENT=3) - the client-side test is skipped under legacy v3 mode, since that mode exercises the separately-publishedengine.io-client-v3package rather than theengine.io-clientpackage touched by this PRengine.io-clientsuite passes🤖 Generated with Claude Code