Skip to content

fix(eio,eio-client): handle close packets consistently across transports#5525

Open
GiHoon1123 wants to merge 1 commit into
socketio:mainfrom
GiHoon1123:fix-5286-websocket-close-packet
Open

fix(eio,eio-client): handle close packets consistently across transports#5525
GiHoon1123 wants to merge 1 commit into
socketio:mainfrom
GiHoon1123:fix-5286-websocket-close-packet

Conversation

@GiHoon1123

Copy link
Copy Markdown

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 close is 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 both engine.io and engine.io-client, rather than duplicating polling's own override:

  • On the server, this also covers the uWebSockets.js-based WebSocket transport for free, since it relies on the same base method and doesn't override onData().
  • On the client, the close details now match the polling transport's existing wording ("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 called onClose() 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:

  • The WebTransport transport has the same underlying gap (it decodes and dispatches packets directly, bypassing onData() entirely), but it isn't mentioned in the issue and is disabled by default. Happy to follow up separately if maintainers want it addressed.
  • This PR only reproduces the specific direction described in the issue (server sending a close packet, and the reverse). Nothing further intentionally deferred beyond WebTransport above.

Test plan

  • New test: server closes the transport upon receiving a close packet from a WebSocket client
  • New test: client closes the transport upon receiving a close packet from the server (matches the issue's reproduction), including asserting the close details match polling's wording
  • Verified regression coverage by reverting each fix individually (git stash on just that file) and confirming the corresponding new test fails without it, then restoring and reconfirming it passes
  • Verified empirically (not just via the readyState field) that the underlying raw WebSocket connection is actually closed on both the server and the client side, not just marked closed at the Engine.IO layer
  • Existing engine.io suite 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-published engine.io-client-v3 package rather than the engine.io-client package touched by this PR
  • Existing engine.io-client suite passes

🤖 Generated with Claude Code

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent behavior of close packet processing in websocket and polling transports

1 participant