Skip to content

feat: trigger reconnect on emit if not connected #162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/event_hub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ export class EventHub {
* @return {Boolean}
*/
isConnected(): boolean {
return this._socketIo?.isConnected() || false;
return this._socketIo?.socket.connected || false;
}

/**
Expand Down
15 changes: 9 additions & 6 deletions source/simple_socketio.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export default class SimpleSocketIOClient {
private packetQueue: string[] = [];
private reconnectionAttempts: number = 0;
private reconnecting: boolean = false;
initializing: Promise<void>;

// Added socket object with connected, open reconnect and transport properties to match current API
// The old socket-io client uses both a connected and open property that are interchangeable
Expand Down Expand Up @@ -129,7 +130,7 @@ export default class SimpleSocketIOClient {
this.heartbeatTimeoutMs = heartbeatTimeoutMs;
this.apiUser = apiUser;
this.apiKey = apiKey;
this.initializeWebSocket();
this.initializing = this.initializeWebSocket();
}
/**
* Fetches the session ID from the ftrack server.
Expand Down Expand Up @@ -235,10 +236,10 @@ export default class SimpleSocketIOClient {
}
this.reconnecting = false;
this.reconnectionAttempts = 0; // Reset reconnection attempts
this.handleEvent("connect", {});
this.flushPacketQueue();
// Set connected property to true
this.socket.connected = true;
this.handleEvent("connect", {});
this.flushPacketQueue();
}
/**
* Handles WebSocket closing
Expand Down Expand Up @@ -313,10 +314,11 @@ export default class SimpleSocketIOClient {
const dataString = eventData ? `:::${JSON.stringify(payload)}` : "";
const packet = `${PACKET_TYPES.event}${dataString}`;

if (this.webSocket?.readyState === WebSocket.OPEN) {
if (this.isConnected()) {
this.webSocket.send(packet);
} else {
this.packetQueue.push(packet);
this.reconnect();
}
}
/**
Expand Down Expand Up @@ -404,15 +406,16 @@ export default class SimpleSocketIOClient {
* @private
* @param randomizedDelay
*/
private attemptReconnect(): void {
private async attemptReconnect(): Promise<void> {
await this.initializing;
// Check if already connected or if active reconnection attempt ongoing.
if (this.socket.connected || this.reconnecting) {
return;
}
this.reconnecting = true;
this.reconnectionAttempts++;
this.initializeWebSocket();
this.reconnectTimeout = undefined;
this.initializing = this.initializeWebSocket();
this.reconnect();
}
/**
Expand Down
35 changes: 30 additions & 5 deletions test/simple_socketio.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -410,19 +410,43 @@ describe("Tests using SimpleSocketIOClient", () => {
`${PACKET_TYPES.event}${expectedDataString}`,
);
});
test("emit triggers a reconnect if not connected", () => {
client.webSocket = createWebSocketMock();
client.webSocket.readyState = WebSocket.CLOSED;

const reconnectSpy = vi.spyOn(client, "reconnect");

const eventName = "testEvent";
const eventData = { foo: "bar" };

client.emit(eventName, eventData);

expect(reconnectSpy).toHaveBeenCalledTimes(1);

const expectedPayload = {
name: eventName,
args: [eventData],
};
const expectedDataString = `:::${JSON.stringify(expectedPayload)}`;
expect(client.packetQueue).toContainEqual(
`${PACKET_TYPES.event}${expectedDataString}`,
);

reconnectSpy.mockRestore();
});
describe("Reconnection tests", () => {
test("attemptReconnect method initialises websocket again", () => {
test("attemptReconnect method initialises websocket again", async () => {
client.initializeWebSocket = vi.fn();

client.attemptReconnect();
await client.attemptReconnect();

expect(client.initializeWebSocket).toHaveBeenCalledTimes(1);
});

test("attemptReconnect method increments attempts count", () => {
test("attemptReconnect method increments attempts count", async () => {
const initialAttempts = client.reconnectionAttempts;

client.attemptReconnect();
await client.attemptReconnect();

expect(client.reconnectionAttempts).toBe(initialAttempts + 1);
});
Expand Down Expand Up @@ -456,7 +480,7 @@ describe("Tests using SimpleSocketIOClient", () => {
// Reconnect should not be called yet
expect(client.attemptReconnect).toHaveBeenCalledTimes(0);
});
test("reconnect method exponentially increase delay for every attempt, stopping at the max value", () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it would be helpful with a test that ensures that multiple calls to reconnect during the connection phase does not lead to multiple reconnection attempts in case of failure, just one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already implemented ⭐

test("reconnect method exponentially increase delay for every attempt, stopping at the max value", async () => {
const originalRandom = Math.random;
Math.random = vi.fn().mockReturnValue(1);
vi.useFakeTimers();
Expand All @@ -468,6 +492,7 @@ describe("Tests using SimpleSocketIOClient", () => {
const expectedMaxDelay = expectedMinDelay * 1.5;
client.reconnecting = false; // Since it never gets to the actual fail state triggered by
client.reconnect();
await client.initializing;
vi.advanceTimersByTime(expectedMaxDelay + 1);
expect(client.attemptReconnect).toHaveBeenCalledTimes(i + 1);
}
Expand Down