From dbeb1e5ce9ed76aab84234917c0bb94c2dd6fd92 Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Wed, 18 Sep 2024 10:33:00 +0200 Subject: [PATCH] add some tests --- packages/engine.io-client/lib/socket.ts | 43 +++++++++++------------- packages/engine.io-client/test/socket.js | 26 ++++++++++++++ packages/socket.io-client/lib/manager.ts | 11 ------ packages/socket.io-client/lib/socket.ts | 14 +++----- packages/socket.io-client/test/socket.ts | 29 ++++++++++++++++ 5 files changed, 79 insertions(+), 44 deletions(-) diff --git a/packages/engine.io-client/lib/socket.ts b/packages/engine.io-client/lib/socket.ts index c6e3be091d..dec52864d5 100644 --- a/packages/engine.io-client/lib/socket.ts +++ b/packages/engine.io-client/lib/socket.ts @@ -10,9 +10,9 @@ import { CookieJar, createCookieJar, defaultBinaryType, + nextTick, } from "./globals.node.js"; import debugModule from "debug"; // debug() -import { nextTick } from "./globals.js"; const debug = debugModule("engine.io-client:socket"); // debug() @@ -321,8 +321,11 @@ export class SocketWithoutUpgrade extends Emitter< private _pingTimeout: number = -1; private _maxPayload?: number = -1; private _pingTimeoutTimer: NodeJS.Timer; - private _pingTimeoutTime: number | null = null; - private _scheduledDisconnectFromIsResponsive = false; + /** + * The expiration timestamp of the {@link _pingTimeoutTimer} object is tracked, in case the timer is throttled and the + * callback is not fired on time. This can happen for example when a laptop is suspended or when a phone is locked. + */ + private _pingTimeoutTime = Infinity; private clearTimeoutFn: typeof clearTimeout; private readonly _beforeunloadEventListener: () => void; private readonly _offlineEventListener: () => void; @@ -577,7 +580,6 @@ export class SocketWithoutUpgrade extends Emitter< // Socket is live - any packet counts this.emitReserved("heartbeat"); - this._resetPingTimeout(); switch (packet.type) { case "open": @@ -588,6 +590,7 @@ export class SocketWithoutUpgrade extends Emitter< this._sendPacket("pong"); this.emitReserved("ping"); this.emitReserved("pong"); + this._resetPingTimeout(); break; case "error": @@ -633,8 +636,6 @@ export class SocketWithoutUpgrade extends Emitter< */ private _resetPingTimeout() { this.clearTimeoutFn(this._pingTimeoutTimer); - if (this._pingInterval <= 0 || this._pingTimeout <= 0) return; - const delay = this._pingInterval + this._pingTimeout; this._pingTimeoutTime = Date.now() + delay; this._pingTimeoutTimer = this.setTimeoutFn(() => { @@ -718,29 +719,28 @@ export class SocketWithoutUpgrade extends Emitter< } /** - * Returns `true` if the connection is responding to heartbeats. + * Checks whether the heartbeat timer has expired but the socket has not yet been notified. * - * If heartbeats are disabled this will always return `true`. + * Note: this method is private for now because it does not really fit the WebSocket API, but if we put it in the + * `write()` method then the message would not be buffered by the Socket.IO client. * * @return {boolean} + * @private */ - public isResponsive() { - if (this.readyState === "closed") return false; - if (this._pingTimeoutTime === null) return true; + /* private */ _hasPingExpired() { + if (!this._pingTimeoutTime) return true; - const responsive = Date.now() < this._pingTimeoutTime; - if (!responsive && !this._scheduledDisconnectFromIsResponsive) { - debug( - "`isResponsive` detected missed ping so scheduling connection close" - ); - this._scheduledDisconnectFromIsResponsive = true; + const hasExpired = Date.now() > this._pingTimeoutTime; + if (hasExpired) { + debug("throttled timer detected, scheduling connection close"); + this._pingTimeoutTime = 0; nextTick(() => { this._onClose("ping timeout"); }, this.setTimeoutFn); } - return responsive; + return hasExpired; } /** @@ -752,9 +752,6 @@ export class SocketWithoutUpgrade extends Emitter< * @return {Socket} for chaining. */ public write(msg: RawData, options?: WriteOptions, fn?: () => void) { - // this will schedule a disconnection on next tick if heartbeat missed - this.isResponsive(); - this._sendPacket("message", msg, options, fn); return this; } @@ -768,7 +765,8 @@ export class SocketWithoutUpgrade extends Emitter< * @return {Socket} for chaining. */ public send(msg: RawData, options?: WriteOptions, fn?: () => void) { - return this.write(msg, options, fn); + this._sendPacket("message", msg, options, fn); + return this; } /** @@ -895,7 +893,6 @@ export class SocketWithoutUpgrade extends Emitter< // clear timers this.clearTimeoutFn(this._pingTimeoutTimer); - this._pingTimeoutTime = null; // stop event from firing again for transport this.transport.removeAllListeners("close"); diff --git a/packages/engine.io-client/test/socket.js b/packages/engine.io-client/test/socket.js index a7037f6e94..9e87e36170 100644 --- a/packages/engine.io-client/test/socket.js +++ b/packages/engine.io-client/test/socket.js @@ -270,4 +270,30 @@ describe("Socket", function () { }); }); }); + + describe("throttled timer", () => { + it("checks the state of the timer", (done) => { + const socket = new Socket(); + + expect(socket._hasPingExpired()).to.be(false); + + socket.on("open", () => { + expect(socket._hasPingExpired()).to.be(false); + + // simulate a throttled timer + socket._pingTimeoutTime = Date.now() - 1; + + expect(socket._hasPingExpired()).to.be(true); + + // subsequent calls should not trigger more 'close' events + expect(socket._hasPingExpired()).to.be(true); + expect(socket._hasPingExpired()).to.be(true); + }); + + socket.on("close", (reason) => { + expect(reason).to.eql("ping timeout"); + done(); + }); + }); + }); }); diff --git a/packages/socket.io-client/lib/manager.ts b/packages/socket.io-client/lib/manager.ts index 5c66acc107..35bae86ae6 100644 --- a/packages/socket.io-client/lib/manager.ts +++ b/packages/socket.io-client/lib/manager.ts @@ -492,17 +492,6 @@ export class Manager< this._close(); } - /** - * Returns `true` if the connection is responding to heartbeats. - * - * If heartbeats are disabled this will always return `true`. - * - * @return {boolean} - */ - isResponsive() { - return this.engine.isResponsive ? this.engine.isResponsive() : true; - } - /** * Writes a packet. * diff --git a/packages/socket.io-client/lib/socket.ts b/packages/socket.io-client/lib/socket.ts index 219aebe3d1..973bafb32f 100644 --- a/packages/socket.io-client/lib/socket.ts +++ b/packages/socket.io-client/lib/socket.ts @@ -440,19 +440,13 @@ export class Socket< packet.id = id; } - const isTransportWritable = - this.io.engine && - this.io.engine.transport && - this.io.engine.transport.writable; + const isTransportWritable = this.io.engine?.transport?.writable; + const isConnected = this.connected && !this.io.engine?._hasPingExpired(); - const discardPacket = - this.flags.volatile && (!isTransportWritable || !this.connected); + const discardPacket = this.flags.volatile && !isTransportWritable; if (discardPacket) { debug("discard packet as the transport is not currently writable"); - } else if ( - this.connected && - (this.flags.volatile || this.io.isResponsive()) - ) { + } else if (isConnected) { this.notifyOutgoingListeners(packet); this.packet(packet); } else { diff --git a/packages/socket.io-client/test/socket.ts b/packages/socket.io-client/test/socket.ts index 2f067b1ea9..9374453850 100644 --- a/packages/socket.io-client/test/socket.ts +++ b/packages/socket.io-client/test/socket.ts @@ -787,4 +787,33 @@ describe("socket", () => { }); }); }); + + describe("throttled timer", () => { + it("should buffer the event and send it upon reconnection", () => { + return wrap((done) => { + let hasReconnected = false; + + const socket = io(BASE_URL, { + forceNew: true, + reconnectionDelay: 10, + }); + + socket.once("connect", () => { + // @ts-expect-error simulate a throttled timer + socket.io.engine._pingTimeoutTime = Date.now() - 1; + + socket.emit("echo", "123", (value) => { + expect(hasReconnected).to.be(true); + expect(value).to.eql("123"); + + success(done, socket); + }); + }); + + socket.io.once("reconnect", () => { + hasReconnected = true; + }); + }); + }); + }); });