add some tests

Author: darrachequesne

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");

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();
+      });
+    });
+  });
 });

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.
    *

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 {

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;
+        });
+      });
+    });
+  });
 });