From b9252e207413a850db7e4f0f0ef7dd2ef0ed26da Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Mon, 11 Apr 2022 16:58:12 +0200 Subject: [PATCH] feat: add details to the "close" event The close event will now include additional details to help debugging if anything has gone wrong. Example when a payload is over the maxHttpBufferSize value in HTTP long-polling mode: ```js socket.on("close", (reason, details) => { console.log(reason); // "transport error" // in that case, details is an error object console.log(details.message); "xhr post error" console.log(details.description); // 413 (the HTTP status of the response) // details.context refers to the XMLHttpRequest object console.log(details.context.status); // 413 console.log(details.context.responseText); // "" }); ``` Note: the error object was already included before this commit and is kept for backward compatibility Example when a payload is over the maxHttpBufferSize value with WebSockets: ```js socket.on("close", (reason, details) => { console.log(reason); // "transport close" // in that case, details is a plain object console.log(details.description); // "websocket connection closed" // details.context is a CloseEvent object console.log(details.context.code); // 1009 (which means "Message Too Big") console.log(details.context.reason); // "" }); ``` Example within a cluster without sticky sessions: ```js socket.on("close", (reason, details) => { console.log(details.context.status); // 400 console.log(details.context.responseText); // '{"code":1,"message":"Session ID unknown"}' }); ``` Note: we could also print some warnings in development for the "usual" errors: - CORS error - HTTP 400 with multiple nodes - HTTP 413 with maxHttpBufferSize but that would require an additional step when going to production (i.e. setting NODE_ENV variable to "production"). This is open to discussion! Related: - https://github.com/socketio/socket.io/issues/3946 - https://github.com/socketio/socket.io/issues/1979 - https://github.com/socketio/socket.io-client/issues/1518 --- lib/socket.ts | 15 ++--- lib/transport.ts | 64 +++++++++++++++------ lib/transports/polling-xhr.ts | 26 ++++++--- lib/transports/polling.ts | 8 +-- lib/transports/websocket.ts | 10 +++- test/socket.js | 104 ++++++++++++++++++++++++++++++++-- 6 files changed, 181 insertions(+), 46 deletions(-) diff --git a/lib/socket.ts b/lib/socket.ts index 41c5a5e07..22ae36fa0 100644 --- a/lib/socket.ts +++ b/lib/socket.ts @@ -5,6 +5,7 @@ import parseuri from "parseuri"; import debugModule from "debug"; // debug() import { Emitter } from "@socket.io/component-emitter"; import { protocol } from "engine.io-parser"; +import { CloseDetails } from "./transport"; const debug = debugModule("engine.io-client:socket"); // debug() @@ -230,7 +231,7 @@ interface SocketReservedEvents { upgrading: (transport) => void; upgrade: (transport) => void; upgradeError: (err: Error) => void; - close: (reason: string, desc?: Error) => void; + close: (reason: string, description?: CloseDetails | Error) => void; } export class Socket extends Emitter<{}, {}, SocketReservedEvents> { @@ -365,7 +366,9 @@ export class Socket extends Emitter<{}, {}, SocketReservedEvents> { } if (this.hostname !== "localhost") { this.offlineEventListener = () => { - this.onClose("transport close"); + this.onClose("transport close", { + description: "network connection lost" + }); }; addEventListener("offline", this.offlineEventListener, false); } @@ -471,9 +474,7 @@ export class Socket extends Emitter<{}, {}, SocketReservedEvents> { .on("drain", this.onDrain.bind(this)) .on("packet", this.onPacket.bind(this)) .on("error", this.onError.bind(this)) - .on("close", () => { - this.onClose("transport close"); - }); + .on("close", reason => this.onClose("transport close", reason)); } /** @@ -890,7 +891,7 @@ export class Socket extends Emitter<{}, {}, SocketReservedEvents> { * * @api private */ - private onClose(reason: string, desc?) { + private onClose(reason: string, description?: CloseDetails | Error) { if ( "opening" === this.readyState || "open" === this.readyState || @@ -921,7 +922,7 @@ export class Socket extends Emitter<{}, {}, SocketReservedEvents> { this.id = null; // emit close event - this.emitReserved("close", reason, desc); + this.emitReserved("close", reason, description); // clean buffers after, so users can still // grab the buffers on `close` event diff --git a/lib/transport.ts b/lib/transport.ts index 965cff4db..73984eff0 100644 --- a/lib/transport.ts +++ b/lib/transport.ts @@ -1,14 +1,42 @@ -import { decodePacket } from "engine.io-parser"; -import { DefaultEventsMap, Emitter } from "@socket.io/component-emitter"; +import { decodePacket, Packet, RawData } from "engine.io-parser"; +import { Emitter } from "@socket.io/component-emitter"; import { installTimerFunctions } from "./util.js"; import debugModule from "debug"; // debug() import { SocketOptions } from "./socket.js"; const debug = debugModule("engine.io-client:transport"); // debug() +class TransportError extends Error { + public readonly type = "TransportError"; + + constructor( + reason: string, + readonly description: any, + readonly context: any + ) { + super(reason); + } +} + +export interface CloseDetails { + description: string; + context?: CloseEvent | XMLHttpRequest; +} + +interface TransportReservedEvents { + open: () => void; + error: (err: TransportError) => void; + packet: (packet: Packet) => void; + close: (details?: CloseDetails) => void; + poll: () => void; + pollComplete: () => void; + drain: () => void; +} + export abstract class Transport extends Emitter< - DefaultEventsMap, - DefaultEventsMap + {}, + {}, + TransportReservedEvents > { protected opts: SocketOptions; protected supportsBinary: boolean; @@ -37,17 +65,17 @@ export abstract class Transport extends Emitter< /** * Emits an error. * - * @param {String} str + * @param {String} reason + * @param description + * @param context - the error context * @return {Transport} for chaining * @api protected */ - protected onError(msg, desc) { - const err = new Error(msg); - // @ts-ignore - err.type = "TransportError"; - // @ts-ignore - err.description = desc; - super.emit("error", err); + protected onError(reason: string, description: any, context?: any) { + super.emitReserved( + "error", + new TransportError(reason, description, context) + ); return this; } @@ -102,7 +130,7 @@ export abstract class Transport extends Emitter< protected onOpen() { this.readyState = "open"; this.writable = true; - super.emit("open"); + super.emitReserved("open"); } /** @@ -111,7 +139,7 @@ export abstract class Transport extends Emitter< * @param {String} data * @api protected */ - protected onData(data) { + protected onData(data: RawData) { const packet = decodePacket(data, this.socket.binaryType); this.onPacket(packet); } @@ -121,8 +149,8 @@ export abstract class Transport extends Emitter< * * @api protected */ - protected onPacket(packet) { - super.emit("packet", packet); + protected onPacket(packet: Packet) { + super.emitReserved("packet", packet); } /** @@ -130,9 +158,9 @@ export abstract class Transport extends Emitter< * * @api protected */ - protected onClose() { + protected onClose(details?: CloseDetails) { this.readyState = "closed"; - super.emit("close"); + super.emitReserved("close", details); } protected abstract doOpen(); diff --git a/lib/transports/polling-xhr.ts b/lib/transports/polling-xhr.ts index 92eba06f7..917002b1c 100755 --- a/lib/transports/polling-xhr.ts +++ b/lib/transports/polling-xhr.ts @@ -7,6 +7,8 @@ import { installTimerFunctions, pick } from "../util.js"; import { DefaultEventsMap, Emitter } from "@socket.io/component-emitter"; import { Polling } from "./polling.js"; import { SocketOptions } from "../socket.js"; +import { RawData } from "engine.io-parser"; +import { CloseDetails } from "../transport"; const debug = debugModule("engine.io-client:polling-xhr"); // debug() @@ -83,8 +85,8 @@ export class XHR extends Polling { data: data }); req.on("success", fn); - req.on("error", err => { - this.onError("xhr post error", err); + req.on("error", (xhrStatus, context) => { + this.onError("xhr post error", xhrStatus, context); }); } @@ -97,14 +99,20 @@ export class XHR extends Polling { debug("xhr poll"); const req = this.request(); req.on("data", this.onData.bind(this)); - req.on("error", err => { - this.onError("xhr poll error", err); + req.on("error", (xhrStatus, context) => { + this.onError("xhr poll error", xhrStatus, context); }); this.pollXhr = req; } } -export class Request extends Emitter { +interface RequestReservedEvents { + success: () => void; + data: (data: RawData) => void; + error: (err: number | Error, context: XMLHttpRequest) => void; +} + +export class Request extends Emitter<{}, {}, RequestReservedEvents> { private readonly opts: { xd; xs } & SocketOptions; private readonly method: string; private readonly uri: string; @@ -230,7 +238,7 @@ export class Request extends Emitter { * @api private */ onSuccess() { - this.emit("success"); + this.emitReserved("success"); this.cleanup(); } @@ -240,7 +248,7 @@ export class Request extends Emitter { * @api private */ onData(data) { - this.emit("data", data); + this.emitReserved("data", data); this.onSuccess(); } @@ -249,8 +257,8 @@ export class Request extends Emitter { * * @api private */ - onError(err) { - this.emit("error", err); + onError(err: number | Error) { + this.emitReserved("error", err, this.xhr); this.cleanup(true); } diff --git a/lib/transports/polling.ts b/lib/transports/polling.ts index c8fe7e247..00749d4b1 100644 --- a/lib/transports/polling.ts +++ b/lib/transports/polling.ts @@ -75,7 +75,7 @@ export abstract class Polling extends Transport { debug("polling"); this.polling = true; this.doPoll(); - this.emit("poll"); + this.emitReserved("poll"); } /** @@ -93,7 +93,7 @@ export abstract class Polling extends Transport { // if its a close packet, we close the ongoing requests if ("close" === packet.type) { - this.onClose(); + this.onClose({ description: "transport closed by the server" }); return false; } @@ -108,7 +108,7 @@ export abstract class Polling extends Transport { if ("closed" !== this.readyState) { // if we got data we're not polling this.polling = false; - this.emit("pollComplete"); + this.emitReserved("pollComplete"); if ("open" === this.readyState) { this.poll(); @@ -153,7 +153,7 @@ export abstract class Polling extends Transport { encodePayload(packets, data => { this.doWrite(data, () => { this.writable = true; - this.emit("drain"); + this.emitReserved("drain"); }); }); } diff --git a/lib/transports/websocket.ts b/lib/transports/websocket.ts index e24774752..a85cca860 100644 --- a/lib/transports/websocket.ts +++ b/lib/transports/websocket.ts @@ -91,7 +91,7 @@ export class WS extends Transport { : new WebSocket(uri) : new WebSocket(uri, protocols, opts); } catch (err) { - return this.emit("error", err); + return this.emitReserved("error", err); } this.ws.binaryType = this.socket.binaryType || defaultBinaryType; @@ -111,7 +111,11 @@ export class WS extends Transport { } this.onOpen(); }; - this.ws.onclose = this.onClose.bind(this); + this.ws.onclose = closeEvent => + this.onClose({ + description: "websocket connection closed", + context: closeEvent + }); this.ws.onmessage = ev => this.onData(ev.data); this.ws.onerror = e => this.onError("websocket error", e); } @@ -168,7 +172,7 @@ export class WS extends Transport { // defer to next tick to allow Socket to clear writeBuffer nextTick(() => { this.writable = true; - this.emit("drain"); + this.emitReserved("drain"); }, this.setTimeoutFn); } }); diff --git a/test/socket.js b/test/socket.js index 8014d1877..c0594101a 100644 --- a/test/socket.js +++ b/test/socket.js @@ -1,14 +1,15 @@ const expect = require("expect.js"); -const eio = require("../"); +const { Socket } = require("../"); const { isIE11, isAndroid, isEdge, isIPad } = require("./support/env"); const FakeTimers = require("@sinonjs/fake-timers"); +const { repeat } = require("./util"); describe("Socket", function() { this.timeout(10000); describe("filterUpgrades", () => { it("should return only available transports", () => { - const socket = new eio.Socket({ transports: ["polling"] }); + const socket = new Socket({ transports: ["polling"] }); expect(socket.filterUpgrades(["polling", "websocket"])).to.eql([ "polling" ]); @@ -17,7 +18,7 @@ describe("Socket", function() { }); it("throws an error when no transports are available", done => { - const socket = new eio.Socket({ transports: [] }); + const socket = new Socket({ transports: [] }); let errorMessage = ""; socket.on("error", error => { errorMessage = error; @@ -39,7 +40,7 @@ describe("Socket", function() { it("uses window timeout by default", done => { const clock = FakeTimers.install(); - const socket = new eio.Socket({ transports: [] }); + const socket = new Socket({ transports: [] }); let errorMessage = ""; socket.on("error", error => { errorMessage = error; @@ -54,7 +55,7 @@ describe("Socket", function() { it.skip("uses custom timeout when provided", done => { const clock = FakeTimers.install(); - const socket = new eio.Socket({ + const socket = new Socket({ transports: [], useNativeTimers: true }); @@ -80,4 +81,97 @@ describe("Socket", function() { }, 1); }); }); + + describe("close", () => { + it("provides details when maxHttpBufferSize is reached (polling)", done => { + const socket = new Socket({ transports: ["polling"] }); + socket.on("open", () => { + socket.send(repeat("a", 101)); // over the maxHttpBufferSize value of the server + }); + + socket.on("error", err => { + expect(err).to.be.an(Error); + expect(err.type).to.eql("TransportError"); + expect(err.message).to.eql("xhr post error"); + expect(err.description).to.eql(413); + // err.context is a XMLHttpRequest object + expect(err.context.readyState).to.eql(4); + expect(err.context.responseText).to.eql(""); + }); + + socket.on("close", (reason, details) => { + expect(reason).to.eql("transport error"); + expect(details).to.be.an(Error); + done(); + }); + }); + + it("provides details when maxHttpBufferSize is reached (websocket)", done => { + const socket = new Socket({ transports: ["websocket"] }); + socket.on("open", () => { + socket.send(repeat("a", 101)); // over the maxHttpBufferSize value of the server + }); + + socket.on("close", (reason, details) => { + if (isIE11) { + expect(reason).to.eql("transport error"); + expect(details).to.be.an(Error); + } else { + expect(reason).to.eql("transport close"); + expect(details.description).to.eql("websocket connection closed"); + // details.context is a CloseEvent object + expect(details.context.code).to.eql(1009); // "Message Too Big" (see https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent/code) + expect(details.context.reason).to.eql(""); + // note: details.context.wasClean is false in the browser, but true in Node.js + } + done(); + }); + }); + + it("provides details when the session ID is unknown (polling)", done => { + const socket = new Socket({ + transports: ["polling"], + query: { sid: "abc" } + }); + + socket.on("error", err => { + expect(err).to.be.an(Error); + expect(err.type).to.eql("TransportError"); + expect(err.message).to.eql("xhr poll error"); + expect(err.description).to.eql(400); + // err.context is a XMLHttpRequest object + expect(err.context.readyState).to.eql(4); + expect(err.context.responseText).to.eql( + '{"code":1,"message":"Session ID unknown"}' + ); + }); + + socket.on("close", (reason, details) => { + expect(reason).to.eql("transport error"); + expect(details).to.be.an(Error); + done(); + }); + }); + + it("provides details when the session ID is unknown (websocket)", done => { + const socket = new Socket({ + transports: ["websocket"], + query: { sid: "abc" } + }); + + socket.on("error", err => { + expect(err).to.be.an(Error); + expect(err.type).to.eql("TransportError"); + expect(err.message).to.eql("websocket error"); + // err.description is a generic Event + expect(err.description.type).to.be("error"); + }); + + socket.on("close", (reason, details) => { + expect(reason).to.eql("transport error"); + expect(details).to.be.an(Error); + done(); + }); + }); + }); });