fix(eio): expose EventEmitter interface on WebSocket upgrade response - #5526
Open
nabsei wants to merge 2 commits into
Open
fix(eio): expose EventEmitter interface on WebSocket upgrade response#5526nabsei wants to merge 2 commits into
nabsei wants to merge 2 commits into
Conversation
Author
|
Just checking in on this one — happy to make any changes if something's needed, or let me know if it's not a good fit. No rush either way. |
WebSocketResponse (the fake response object passed to middlewares during a WebSocket upgrade) did not implement EventEmitter, so any middleware calling res.on(...), e.g. pino-http listening for "close" to know when the request finished, crashed with "res.on is not a function". Make WebSocketResponse extend EventEmitter and emit "close" when the underlying socket closes, and "finish" when end() is called, mirroring the events a real http.ServerResponse emits. Fixes socketio#5072
The previous commit only patched WebSocketResponse in server.ts (used by the plain Node.js HTTP server). userver.ts (used with EIO_WS_ENGINE=uws) has its own separate response class, ResponseWrapper, which didn't get the same fix, so the pino-http regression test failed specifically under the uws engine. Two changes were needed, not just extending EventEmitter: unlike the Node.js HTTP server, where the same TCP socket persists across a WebSocket upgrade (so listening for its "close" event works directly), uWebSockets.js invalidates the HTTP response handle once res.upgrade() succeeds, and res.onAborted() never fires afterwards. The ResponseWrapper now also gets threaded through the WebSocket's user data on upgrade, so it can be told to emit "close" from the `close` handler on the `.ws()` route once the actual WebSocket connection ends. Verified: full engine.io suite passes under both EIO_WS_ENGINE=uws (178 passing, 0 failing, up from 177/1) and the default ws engine (175 passing on server.js/engine.io.js/middlewares.js, no regressions). prettier --check passes.
nabsei
force-pushed
the
fix/eio-websocket-response-eventemitter
branch
from
July 27, 2026 12:17
10c0eed to
facee04
Compare
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
1 participant
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
Fixes #5072. During a WebSocket upgrade,
engine.ioapplies middlewares against aWebSocketResponseobject that only implements a small subset ofhttp.ServerResponse(setHeader/getHeader/removeHeader/write/writeHead/end). It does not implementEventEmitter, so any middleware that callsres.on(...), for examplepino-http, which listens for"close"to know when the request finished, crashes withTypeError: res.on is not a function.New behavior
WebSocketResponsenow extendsEventEmitterand emits"close"when the underlying socket closes, and"finish"whenend()is called, mirroring the events a realhttp.ServerResponseemits. Added a regression test intest/middlewares.jsthat registers ares.on("close", ...)listener during a WebSocket upgrade and asserts it fires.Other information (e.g. related issues)
Parts of this fix (root-cause analysis and implementation) were produced with the help of Claude Code. I reviewed and understood the change before opening this PR.
Verification performed:
middlewares.js/server.js/engine.io.jssuite (175 passing, 2 pre-existing unrelated pending), no regressions.pino-httppackage against an unpatched build (crashes withres.on is not a function), then confirmed the same script completes cleanly with this fix applied.prettier --checkpasses on the changed files.