fix(eio): expose EventEmitter interface on WebSocket upgrade response#5526
Open
nabsei wants to merge 1 commit into
Open
fix(eio): expose EventEmitter interface on WebSocket upgrade response#5526nabsei wants to merge 1 commit into
nabsei wants to merge 1 commit into
Conversation
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 Co-authored-by: Claude <noreply@anthropic.com>
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
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 — credited via the
Co-authored-bytrailer on the commit. 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.