Skip to content

Commit 0de5452

Browse files
committed
fix(eio): make the upgrade response an EventEmitter
WebSocketResponse exposes a subset of http.ServerResponse so that Express middlewares can be applied to an upgrade request, but it was a plain object. Any middleware that observes the response lifecycle — pino-http and most other request loggers do, via res.on('close') — threw "TypeError: res.on is not a function" and killed the upgrade. Extend EventEmitter and emit "finish" then "close" once the handshake has been written, so those middlewares run to completion instead of crashing. statusCode is set to 101 so a logger reports the handshake rather than an undefined status.
1 parent 9738333 commit 0de5452

2 files changed

Lines changed: 44 additions & 1 deletion

File tree

‎packages/engine.io/lib/server.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -685,11 +685,17 @@ export abstract class BaseServer extends EventEmitter {
685685
*
686686
* @see https://nodejs.org/api/http.html#class-httpserverresponse
687687
*/
688-
class WebSocketResponse {
688+
class WebSocketResponse extends EventEmitter {
689+
/**
690+
* The status code of the handshake response written by the "ws" package.
691+
*/
692+
public statusCode = 101;
693+
689694
constructor(
690695
readonly req,
691696
readonly socket: Duplex,
692697
) {
698+
super();
693699
// temporarily store the response headers on the req object (see the "headers" event)
694700
req[kResponseHeaders] = {};
695701
}
@@ -875,6 +881,11 @@ export class Server extends BaseServer {
875881

876882
// delegate to ws
877883
this.ws.handleUpgrade(engineRequest, socket, head, (websocket) => {
884+
// the handshake response has been written, so middlewares which track the
885+
// response lifecycle (loggers, metrics) can now complete
886+
res.emit("finish");
887+
res.emit("close");
888+
878889
this.onWebSocket(engineRequest, socket, websocket);
879890
});
880891
};

‎packages/engine.io/test/middlewares.js‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,38 @@ describe("middlewares", () => {
249249
});
250250
});
251251

252+
it("should support middlewares that track the response lifecycle (websocket)", function (done) {
253+
// the uWebSockets.js server has its own response wrapper, see ResponseWrapper in lib/userver.ts
254+
if (process.env.EIO_WS_ENGINE === "uws") {
255+
return this.skip();
256+
}
257+
const engine = listen((port) => {
258+
let responseClosed = false;
259+
260+
engine.use((req, res, next) => {
261+
// response loggers such as pino-http register listeners on the response
262+
res.on("close", () => {
263+
responseClosed = true;
264+
});
265+
next();
266+
});
267+
268+
const socket = new WebSocket(
269+
`ws://localhost:${port}/engine.io/?EIO=4&transport=websocket`,
270+
);
271+
272+
socket.on("open", () => {
273+
expect(responseClosed).to.be(true);
274+
275+
socket.close();
276+
if (engine.httpServer) {
277+
engine.httpServer.close();
278+
}
279+
done();
280+
});
281+
});
282+
});
283+
252284
it("should fail on errors (polling)", (done) => {
253285
const engine = listen((port) => {
254286
engine.use((req, res, next) => {

0 commit comments

Comments
 (0)