Heartbeat timeout is no longer reset on packet

[streunerlein]

Describe the bug
We have a high volume of data incoming from our sockets and they get disconnected at PingInterval+Timeout s with a ping timeout because the server has not received a pong, however it has received other messages from these sockets.

Note how some connections drop after ~40seconds and reconnect.

There was a fix for this in socketio/engine.io@be7b4e7 which got reverted in 5359bae

While I understand the "wastefulness" of creating timers on every packet, I still think the point of the socket being very much alive if there is data coming from it is valid (even when the pong packages is coming through timely).

Is there a chance to revert the revert?

To Reproduce

Please fill the following code example:

Socket.IO server version: 4.x

Send a lot of packages from the client, watch them disconnect after ~40 seconds even when data is coming from them.

Expected behavior
Connections should not be considered stale if there are still packets coming.

[idirdev]

Ran into something similar in a high-throughput setup. The core of the problem is that the heartbeat timer only resets when a pong is received, not when any data arrives from the client. So if the client is sending tons of data but the pong packet gets delayed or lost in the noise, the server considers the connection dead even though it's clearly alive.

The fix in engine.io (commit be7b4e7) that reset the heartbeat on any incoming packet made sense conceptually, but the revert (5359bae) happened because resetting timers on every packet is expensive with high message volume (clearTimeout + setTimeout on every single message adds up).

A middle-ground approach that avoids the timer churn: instead of resetting the timer on every packet, just track when the last packet arrived and check that timestamp when the heartbeat timer fires:

let lastPacketTime = Date.now();

// on any incoming packet:
lastPacketTime = Date.now();

// when heartbeat timer fires:
if (Date.now() - lastPacketTime < pingTimeout) {
  // client sent data recently, its alive
  resetHeartbeat();
} else {
  disconnect("ping timeout");
}

You just update a timestamp (cheap) and check it when the existing timer fires (also cheap). No timer churn.

As a workaround for now, you could increase pingInterval and pingTimeout on the server to give the pong more time to arrive, or increase them substantially if your transport is reliable enough.

[streunerlein]

Thanks for your thoughts. I see why it was reverted, makes sense and I really like the idea of just storing timestamps. What would be needed to make this land in socket.io?

[darrachequesne]

Hi! I'm currently reviewing #5524.

I'm wondering whether it would make sense to call timer.refresh() on each incoming packet, something like:

private onPacket(packet: Packet) {
    // ...
   
+    if (this.pingTimeoutTimer) {
+      this.pingTimeoutTimer.refresh();
+    }

    switch (packet.type) {
      case "pong":
        if (this.transport.protocol === 3) {
          this.onError(new Error("invalid heartbeat direction"));
          return;
        }
        debug("got pong");
        clearTimeout(this.pingTimeoutTimer);
+       this.pingTimeoutTimer = null;

        this.pingIntervalTimer.refresh();
        this.emit("heartbeat");
        break;

// ...

This way:

• no more ping timeout disconnections under heavy usage
• no timer creation on every incoming message

I'm going to run some benchmarks with that change.

What do you think?

[darrachequesne]

I went ahead and implemented the suggestion above: da008a5

Released in engine.io@6.6.11. Could you please check?