Skip to content

Commit

Permalink
fix(sio-client): do not send a packet on an expired connection (#5134)
Browse files Browse the repository at this point in the history
When a laptop is suspended or a phone is locked, the timer that is used
to check the liveness of the connection is paused and is not able to
detect that the heartbeat has failed.

Previously, emitting a message after resuming the page would lose the
message. The status of the timer will now be checked before sending the
message, so that it gets buffered and sent upon reconnection.

Note: we could also have used the Page Visibility API or a custom
setTimeout() method based on setInterval(), but this would not be as
reliable as the current solution.

Reference: https://developer.mozilla.org/en-US/docs/Web/API/Page_Visibility_API

Related: #5135
  • Loading branch information
tjenkinson authored and darrachequesne committed Sep 18, 2024
1 parent 7a23dde commit 8adcfbf
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 8 deletions.
35 changes: 34 additions & 1 deletion packages/engine.io-client/lib/socket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
CookieJar,
createCookieJar,
defaultBinaryType,
nextTick,
} from "./globals.node.js";
import debugModule from "debug"; // debug()

Expand Down Expand Up @@ -320,6 +321,11 @@ export class SocketWithoutUpgrade extends Emitter<
private _pingTimeout: number = -1;
private _maxPayload?: number = -1;
private _pingTimeoutTimer: NodeJS.Timer;
/**
* 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;
Expand Down Expand Up @@ -630,9 +636,11 @@ export class SocketWithoutUpgrade extends Emitter<
*/
private _resetPingTimeout() {
this.clearTimeoutFn(this._pingTimeoutTimer);
const delay = this._pingInterval + this._pingTimeout;
this._pingTimeoutTime = Date.now() + delay;
this._pingTimeoutTimer = this.setTimeoutFn(() => {
this._onClose("ping timeout");
}, this._pingInterval + this._pingTimeout);
}, delay);
if (this.opts.autoUnref) {
this._pingTimeoutTimer.unref();
}
Expand Down Expand Up @@ -710,6 +718,31 @@ export class SocketWithoutUpgrade extends Emitter<
return this.writeBuffer;
}

/**
* Checks whether the heartbeat timer has expired but the socket has not yet been notified.
*
* 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
*/
/* private */ _hasPingExpired() {
if (!this._pingTimeoutTime) return 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 hasExpired;
}

/**
* Sends a message.
*
Expand Down
26 changes: 26 additions & 0 deletions packages/engine.io-client/test/socket.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
});
});
11 changes: 4 additions & 7 deletions packages/socket.io-client/lib/socket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -440,16 +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();

This comment has been minimized.

Copy link
@tjenkinson

tjenkinson Sep 18, 2024

Author Contributor

@darrachequesne

    const pingExpired = this.io.engine?._hasPingExpired?.() ?? false;
    const isConnected = this.connected && !pingExpired;

is a bit safer in case there's a version mismatch and it's an older engine without the fn?

This comment has been minimized.

Copy link
@darrachequesne

darrachequesne Sep 19, 2024

Member

Good catch! We'll release [email protected], and make socket.io-client depend on it. (we use tilde range so it should be OK)


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) {
} else if (isConnected) {
this.notifyOutgoingListeners(packet);
this.packet(packet);
} else {
Expand Down
29 changes: 29 additions & 0 deletions packages/socket.io-client/test/socket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

This comment has been minimized.

Copy link
@tjenkinson

tjenkinson Sep 18, 2024

Author Contributor

great test!

expect(value).to.eql("123");

success(done, socket);
});
});

socket.io.once("reconnect", () => {
hasReconnected = true;
});
});
});
});
});

0 comments on commit 8adcfbf

Please sign in to comment.