diff --git a/packages/engine.io-client/lib/socket.ts b/packages/engine.io-client/lib/socket.ts index 47303ab6d..3399d9367 100644 --- a/packages/engine.io-client/lib/socket.ts +++ b/packages/engine.io-client/lib/socket.ts @@ -348,6 +348,13 @@ export class SocketWithoutUpgrade extends Emitter< * 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; + /** + * Timestamp of the last packet received from the server, of any type. Used + * by the ping timeout check as a cheap liveness signal, so a server that is + * clearly still sending data isn't considered dead just because its next + * ping happens to be delayed (e.g. behind a backlog on its own end). + */ + private _lastPacketTime: number = Date.now(); private clearTimeoutFn: typeof clearTimeout; private readonly _beforeunloadEventListener: () => void; private readonly _offlineEventListener: () => void; @@ -603,6 +610,7 @@ export class SocketWithoutUpgrade extends Emitter< // Socket is live - any packet counts this.emitReserved("heartbeat"); + this._lastPacketTime = Date.now(); switch (packet.type) { case "open": @@ -662,13 +670,45 @@ export class SocketWithoutUpgrade extends Emitter< const delay = this._pingInterval + this._pingTimeout; this._pingTimeoutTime = Date.now() + delay; this._pingTimeoutTimer = this.setTimeoutFn(() => { - this._onClose("ping timeout"); + this._checkPingTimeout(delay); }, delay); if (this.opts.autoUnref) { this._pingTimeoutTimer.unref(); } } + /** + * Called when the ping timeout timer fires. Rather than closing the + * connection unconditionally, this checks whether any packet has been + * received recently enough that the server is clearly still alive, and if + * so, extends the wait instead of giving up - mirroring the same check on + * the server side (see `Socket#checkPingTimeout` in the `engine.io` + * package), which needed the equivalent client-side change to actually take + * effect: without it, a client that stopped hearing new pings (because the + * server's own next ping was delayed while it waited on a slow pong) would + * disconnect on its own regardless of what the server decided. + * + * @param delay - the ping timeout duration to apply, in milliseconds + * @private + */ + private _checkPingTimeout(delay: number) { + const elapsedSinceLastPacket = Date.now() - this._lastPacketTime; + + if (elapsedSinceLastPacket < delay) { + const remaining = delay - elapsedSinceLastPacket; + this._pingTimeoutTime = Date.now() + remaining; + this._pingTimeoutTimer = this.setTimeoutFn(() => { + this._checkPingTimeout(delay); + }, remaining); + if (this.opts.autoUnref) { + this._pingTimeoutTimer.unref(); + } + return; + } + + this._onClose("ping timeout"); + } + /** * Called on `drain` event * @@ -755,6 +795,14 @@ export class SocketWithoutUpgrade extends Emitter< const hasExpired = Date.now() > this._pingTimeoutTime; if (hasExpired) { + const delay = this._pingInterval + this._pingTimeout; + const elapsedSinceLastPacket = Date.now() - this._lastPacketTime; + + if (elapsedSinceLastPacket < delay) { + this._pingTimeoutTime = Date.now() + (delay - elapsedSinceLastPacket); + return false; + } + debug("throttled timer detected, scheduling connection close"); this._pingTimeoutTime = 0; diff --git a/packages/engine.io-client/test/socket.js b/packages/engine.io-client/test/socket.js index 621c89500..38043742d 100644 --- a/packages/engine.io-client/test/socket.js +++ b/packages/engine.io-client/test/socket.js @@ -293,8 +293,10 @@ describe("Socket", function () { socket.on("open", () => { expect(socket._hasPingExpired()).to.be(false); - // simulate a throttled timer + // simulate a throttled timer with no recent packet either socket._pingTimeoutTime = Date.now() - 1; + socket._lastPacketTime = + Date.now() - (socket._pingInterval + socket._pingTimeout) - 1; expect(socket._hasPingExpired()).to.be(true); @@ -308,5 +310,21 @@ describe("Socket", function () { done(); }); }); + + it("does not close if a packet was recently received", (done) => { + const socket = new Socket(); + + socket.on("open", () => { + // simulate a throttled timer + socket._pingTimeoutTime = Date.now() - 1; + // simulate a recently received packet + socket._lastPacketTime = Date.now(); + + expect(socket._hasPingExpired()).to.be(false); + + socket.close(); + done(); + }); + }); }); }); diff --git a/packages/engine.io/lib/socket.ts b/packages/engine.io/lib/socket.ts index c2efe8ebb..6c372eec5 100644 --- a/packages/engine.io/lib/socket.ts +++ b/packages/engine.io/lib/socket.ts @@ -58,6 +58,13 @@ export class Socket extends EventEmitter { private cleanupFn: any[] = []; private pingTimeoutTimer; private pingIntervalTimer; + /** + * Timestamp of the last packet received from the client, of any type. Used by + * the ping timeout check as a cheap liveness signal: a client that is clearly + * still sending data shouldn't be dropped just because its next expected pong + * happens to be delayed behind other traffic. + */ + private lastPacketTime: number = Date.now(); /** * This is the session identifier that the client will use in the subsequent HTTP requests. It must not be shared with @@ -157,6 +164,7 @@ export class Socket extends EventEmitter { // export packet event debug(`received packet ${packet.type}`); this.emit("packet", packet); + this.lastPacketTime = Date.now(); switch (packet.type) { case "ping": @@ -227,17 +235,44 @@ export class Socket extends EventEmitter { */ private resetPingTimeout() { clearTimeout(this.pingTimeoutTimer); - this.pingTimeoutTimer = setTimeout( - () => { - if (this.readyState === "closed") return; - this.onClose("ping timeout"); - }, + const timeout = this.protocol === 3 ? this.server.opts.pingInterval + this.server.opts.pingTimeout - : this.server.opts.pingTimeout, + : this.server.opts.pingTimeout; + this.pingTimeoutTimer = setTimeout( + () => this.checkPingTimeout(timeout), + timeout, ); } + /** + * Called when the ping timeout timer fires. Instead of closing + * unconditionally, checks whether any packet (not just the expected pong) + * was received recently enough to extend the wait instead. + * + * Unlike a previous, reverted fix, this doesn't touch any timer on the + * common per-packet path (only a timestamp), and never reschedules + * `pingIntervalTimer`, so the ping schedule itself is unaffected. + * + * @param timeout - the ping timeout duration to apply, in milliseconds + * @private + */ + private checkPingTimeout(timeout: number) { + if (this.readyState === "closed") return; + + const elapsedSinceLastPacket = Date.now() - this.lastPacketTime; + + if (elapsedSinceLastPacket < timeout) { + this.pingTimeoutTimer = setTimeout( + () => this.checkPingTimeout(timeout), + timeout - elapsedSinceLastPacket, + ); + return; + } + + this.onClose("ping timeout"); + } + /** * Attaches handlers for the given transport. * diff --git a/packages/engine.io/test/server.js b/packages/engine.io/test/server.js index bbfa86068..659fcaddf 100644 --- a/packages/engine.io/test/server.js +++ b/packages/engine.io/test/server.js @@ -970,6 +970,101 @@ describe("server", () => { }); }); + it("should not trigger a ping timeout on the server if other packets are received while waiting for the pong", (done) => { + // pingInterval/pingTimeout are kept short and distinct so a bug here + // reliably shows up as a premature close within the test's own window, + // without needing to wait out multiple full cycles. + const opts = { allowUpgrades: false, pingInterval: 50, pingTimeout: 30 }; + const engine = listen(opts, (port) => { + const socket = new ClientSocket(`ws://localhost:${port}`); + let serverClosed = false; + let messageInterval; + + engine.on("connection", (conn) => { + conn.on("close", () => { + serverClosed = true; + }); + }); + + socket.on("open", () => { + // This test is scoped to the server's own decision, so the client's + // independent ping timeout watchdog (covered separately below) is + // neutralized here - otherwise the client would eventually give up + // waiting for a next ping of its own accord (since the server never + // gets to send one without a pong to resume its own cycle from), + // which would mask whether the server's check is actually correct. + socket._resetPingTimeout = () => {}; + + // Suppress the client's automatic "ping" -> "pong" reply (simulating + // a pong that is delayed or lost behind other traffic), while still + // dispatching every other packet type as usual. + socket[sendPacketMethod] = ( + (original) => (type, data, options, fn) => { + if (type === "pong") return; + return original(type, data, options, fn); + } + )(socket[sendPacketMethod].bind(socket)); + + // Keep sending "message" packets well within pingTimeout (30ms) of + // each other, so the connection should look alive to the server + // even though no pong ever arrives. + messageInterval = setInterval(() => socket.send("still here"), 10); + }); + + setTimeout(() => { + clearInterval(messageInterval); + expect(serverClosed).to.be(false); + socket.close(); + engine.httpServer.close(); + done(); + }, 250); // several multiples of pingInterval + pingTimeout + }); + }); + + it("should not trigger a ping timeout on the client if other packets are received while waiting for the next ping", (done) => { + // Symmetric counterpart of the test above: here the *server's* ping + // schedule is what's stalled (simulating it never getting a pong to + // resume its own cycle from), while both ends keep exchanging ordinary + // "message" packets. The client should not give up on its own accord + // just because no fresh ping arrives. + const opts = { allowUpgrades: false, pingInterval: 50, pingTimeout: 30 }; + const engine = listen(opts, (port) => { + const socket = new ClientSocket(`ws://localhost:${port}`); + let clientClosed = false; + let clientMessageInterval; + + socket.on("open", () => { + clientMessageInterval = setInterval( + () => socket.send("client alive"), + 10, + ); + }); + socket.on("close", () => { + clientClosed = true; + }); + + engine.on("connection", (conn) => { + const originalSendPacket = conn.sendPacket.bind(conn); + conn.sendPacket = (type, data, options, fn) => { + if (type === "ping") return; + return originalSendPacket(type, data, options, fn); + }; + const serverMessageInterval = setInterval(() => { + if (conn.readyState === "open") conn.send("still here"); + }, 10); + conn.on("close", () => clearInterval(serverMessageInterval)); + }); + + setTimeout(() => { + clearInterval(clientMessageInterval); + expect(clientClosed).to.be(false); + socket.close(); + engine.httpServer.close(); + done(); + }, 250); + }); + }); + it("should trigger when server closes a client", (done) => { const engine = listen({ allowUpgrades: false }, (port) => { const socket = new ClientSocket(`ws://localhost:${port}`);