From 67ab69606a2418fc2da510c2036a24ba942456c2 Mon Sep 17 00:00:00 2001 From: David Krauser Date: Fri, 9 May 2025 15:39:10 -0400 Subject: [PATCH] Handle network connectivity changes in websocket (#30788) This commit introduces listeners for network changes that will: - Test the websocket if we get an offline event to check if we have disconnected. - Re-connect the websocket immediately if we get an online event, and we are disconnected. Additionally when a ping fails, we now immediately call the onclose() callback instead of waiting for the system to trigger it when the broken websocket closes. This allows us to re-connect much more quickly (since we don't have to wait for the broken websocket to get cleaned up by the system). --- ...post_body_additional_content.test.tsx.snap | 8 +- webapp/platform/client/src/websocket.test.ts | 207 +++++++++++++++++- webapp/platform/client/src/websocket.ts | 203 ++++++++++++----- 3 files changed, 353 insertions(+), 65 deletions(-) diff --git a/webapp/channels/src/components/post_view/post_body_additional_content/__snapshots__/post_body_additional_content.test.tsx.snap b/webapp/channels/src/components/post_view/post_body_additional_content/__snapshots__/post_body_additional_content.test.tsx.snap index cda0cedefe..fef1216ada 100644 --- a/webapp/channels/src/components/post_view/post_body_additional_content/__snapshots__/post_body_additional_content.test.tsx.snap +++ b/webapp/channels/src/components/post_view/post_body_additional_content/__snapshots__/post_body_additional_content.test.tsx.snap @@ -110,7 +110,6 @@ exports[`PostBodyAdditionalContent with a normal link Should render the plugin c "conn": null, "connectFailCount": 0, "connectionId": "", - "connectionUrl": null, "errorCallback": null, "errorListeners": Set {}, "eventCallback": null, @@ -119,6 +118,8 @@ exports[`PostBodyAdditionalContent with a normal link Should render the plugin c "messageListeners": Set {}, "missedEventCallback": null, "missedMessageListeners": Set {}, + "offlineHandler": null, + "onlineHandler": null, "pingInterval": null, "postedAck": false, "reconnectCallback": null, @@ -128,6 +129,7 @@ exports[`PostBodyAdditionalContent with a normal link Should render the plugin c "responseSequence": 1, "serverHostname": "", "serverSequence": 0, + "waitingForPong": false, } } /> @@ -168,7 +170,6 @@ exports[`PostBodyAdditionalContent with a normal link Should render the plugin c "conn": null, "connectFailCount": 0, "connectionId": "", - "connectionUrl": null, "errorCallback": null, "errorListeners": Set {}, "eventCallback": null, @@ -177,6 +178,8 @@ exports[`PostBodyAdditionalContent with a normal link Should render the plugin c "messageListeners": Set {}, "missedEventCallback": null, "missedMessageListeners": Set {}, + "offlineHandler": null, + "onlineHandler": null, "pingInterval": null, "postedAck": false, "reconnectCallback": null, @@ -186,6 +189,7 @@ exports[`PostBodyAdditionalContent with a normal link Should render the plugin c "responseSequence": 1, "serverHostname": "", "serverSequence": 0, + "waitingForPong": false, } } /> diff --git a/webapp/platform/client/src/websocket.test.ts b/webapp/platform/client/src/websocket.test.ts index 98785fb237..935943320a 100644 --- a/webapp/platform/client/src/websocket.test.ts +++ b/webapp/platform/client/src/websocket.test.ts @@ -3,13 +3,52 @@ import WebSocketClient from './websocket'; -// Define some WebSocket globals that aren't defined in node +// Define some browser globals that aren't defined in node if (typeof WebSocket === 'undefined') { (global as any).WebSocket = { CONNECTING: 0, OPEN: 1, CLOSING: 2, CLOSED: 3, }; } +// Mock window and navigator if they're not defined +if (typeof window === 'undefined') { + const eventHandlers: {[key: string]: Array<(event: Event) => void>} = {}; + + // Create a mock window object with working event handlers + (global as any).window = { + addEventListener: jest.fn((event: string, handler: (event: Event) => void) => { + if (!eventHandlers[event]) { + eventHandlers[event] = []; + } + eventHandlers[event].push(handler); + }), + removeEventListener: jest.fn((event: string, handler: (event: Event) => void) => { + if (eventHandlers[event]) { + const index = eventHandlers[event].indexOf(handler); + if (index !== -1) { + eventHandlers[event].splice(index, 1); + } + } + }), + dispatchEvent: jest.fn((event: Event) => { + const handlers = eventHandlers[event.type] || []; + handlers.forEach((handler) => handler(event)); + return true; + }), + }; +} + +// Mock Event class if it's not defined +if (typeof Event === 'undefined') { + (global as any).Event = class MockEvent { + type: string; + + constructor(type: string) { + this.type = type; + } + }; +} + class MockWebSocket { readonly binaryType: BinaryType = 'blob'; readonly bufferedAmount: number = 0; @@ -286,7 +325,7 @@ describe('websocketclient', () => { }, minWebSocketRetryTime: 1, reconnectJitterRange: 1, - clientPingInterval: 1, + clientPingInterval: 10, }); let numPings = 0; @@ -321,14 +360,16 @@ describe('websocketclient', () => { if (mockWebSocket.onclose) { mockWebSocket.onclose(); } - if ((mockWebSocket.close as jest.Mock).mock.calls.length > 2) { - client.close(); + if (jest.mocked(mockWebSocket.close).mock.calls.length === 3) { + setTimeout(() => { + client.close(); + }, 1); } }); client.initialize('mock.url'); - jest.advanceTimersByTime(30); + jest.advanceTimersByTime(100); client.close(); @@ -418,4 +459,160 @@ describe('websocketclient', () => { jest.useRealTimers(); }); + + test('should add network event listener on initialize', () => { + // Mock window.addEventListener + const originalAddEventListener = window.addEventListener; + const originalRemoveEventListener = window.removeEventListener; + + const addEventListenerMock = jest.fn(); + const removeEventListenerMock = jest.fn(); + + window.addEventListener = addEventListenerMock; + window.removeEventListener = removeEventListenerMock; + + // Create client + const mockWebSocket = new MockWebSocket(); + const client = new WebSocketClient({ + newWebSocketFn: (url: string) => { + mockWebSocket.url = url; + return mockWebSocket; + }, + }); + + // No listeners should be added on construction + expect(addEventListenerMock).not.toHaveBeenCalled(); + + // Initialize should add listeners + client.initialize('mock.url'); + + // Verify event listeners are added on initialize + expect(addEventListenerMock).toHaveBeenCalledWith('online', expect.any(Function)); + + // Clean up + client.close(); + + // Verify event listeners are removed + expect(removeEventListenerMock).toHaveBeenCalledWith('online', expect.any(Function)); + + // Restore mocks + window.addEventListener = originalAddEventListener; + window.removeEventListener = originalRemoveEventListener; + }); + + test('should reconnect when network comes online', () => { + jest.useFakeTimers(); + + var connected = true; + const mockWebSocket = new MockWebSocket(); + const newWebSocketFn = jest.fn((url: string) => { + mockWebSocket.url = url; + + // selectively simulate the network being down + setTimeout(() => { + if (!connected && mockWebSocket.onclose) { + mockWebSocket.close(); + } + }, 1); + + return mockWebSocket; + }); + + // Use a small minWebSocketRetryTime to speed up the test + const client = new WebSocketClient({ + newWebSocketFn, + minWebSocketRetryTime: 100, + maxWebSocketRetryTime: 1000, + }); + + // Initialize the client + client.initialize('mock.url'); + mockWebSocket.open(); + expect(newWebSocketFn).toHaveBeenCalledTimes(1); + + // Simulate network going offline + mockWebSocket.close(); + connected = false; + + // Connection should be closed + expect(mockWebSocket.readyState).toBe(WebSocket.CLOSED); + + // Wait a very long time to max out retry timeout + jest.advanceTimersByTime(10000); + + // Reset the mock to track the next retry + // which should be quicker than the max + newWebSocketFn.mockClear(); + + // Simulate network coming back online + connected = true; + const onlineEvent = new Event('online'); + window.dispatchEvent(onlineEvent); + + // Should not reconnect immediately (should wait for the timeout) + expect(newWebSocketFn).not.toHaveBeenCalled(); + + // Advance timers to trigger the reconnect + jest.advanceTimersByTime(110); + + // Should have reconnected after the timeout + expect(newWebSocketFn).toHaveBeenCalledTimes(1); + + // Clean up + client.close(); + + // Reset timers + jest.useRealTimers(); + }); + + test('should send ping when network goes offline', () => { + jest.useFakeTimers(); + + const mockWebSocket = new MockWebSocket(); + const client = new WebSocketClient({ + newWebSocketFn: (url: string) => { + mockWebSocket.url = url; + setTimeout(() => { + if (mockWebSocket.onopen) { + mockWebSocket.open(); + } + }, 1); + return mockWebSocket; + }, + clientPingInterval: 300, + }); + + let numPings = 0; + mockWebSocket.send = (evt) => { + const msg = JSON.parse(evt); + if (msg.action !== 'ping') { + return; + } + numPings++; + }; + + const openSpy = jest.spyOn(mockWebSocket, 'open'); + const closeSpy = jest.spyOn(mockWebSocket, 'close'); + + client.initialize('mock.url'); + jest.advanceTimersByTime(10); + + expect(mockWebSocket.readyState).toBe(WebSocket.OPEN); + expect(openSpy).toBeCalledTimes(1); + expect(closeSpy).toBeCalledTimes(0); + expect(numPings).toBe(1); + + // Simulate network going offline + const offlineEvent = new Event('offline'); + window.dispatchEvent(offlineEvent); + jest.advanceTimersByTime(10); + + client.close(); + + expect(openSpy).toBeCalledTimes(1); + expect(closeSpy).toBeCalledTimes(1); + expect(numPings).toBe(2); + + jest.useRealTimers(); + }); }); diff --git a/webapp/platform/client/src/websocket.ts b/webapp/platform/client/src/websocket.ts index 1a20161a0c..6fa1d15f66 100644 --- a/webapp/platform/client/src/websocket.ts +++ b/webapp/platform/client/src/websocket.ts @@ -34,7 +34,6 @@ export default class WebSocketClient { private config: WebSocketClientConfig; private conn: WebSocket | null; - private connectionUrl: string | null; // responseSequence is the number to track a response sent // via the websocket. A response will always have the same sequence number @@ -89,12 +88,17 @@ export default class WebSocketClient { private postedAck: boolean; private pingInterval: ReturnType | null; + private waitingForPong: boolean; + // reconnectTimeout is used for automatic reconnect after socket close private reconnectTimeout: ReturnType | null; + // Network event handlers + private onlineHandler: (() => void) | null = null; + private offlineHandler: (() => void) | null = null; + constructor(config?: Partial) { this.conn = null; - this.connectionUrl = null; this.responseSequence = 1; this.serverSequence = 0; this.connectFailCount = 0; @@ -105,12 +109,13 @@ export default class WebSocketClient { this.reconnectTimeout = null; this.config = {...defaultWebSocketClientConfig, ...config}; this.pingInterval = null; + this.waitingForPong = false; } // on connect, only send auth cookie and blank state. // on hello, get the connectionID and store it. // on reconnect, send cookie, connectionID, sequence number. - initialize(connectionUrl = this.connectionUrl, token?: string, postedAck?: boolean) { + initialize(connectionUrl: string, token?: string, postedAck?: boolean) { if (this.conn) { return; } @@ -135,6 +140,61 @@ export default class WebSocketClient { this.postedAck = postedAck; } + // Setup network event listener + // Remove existing listeners if any + if (this.onlineHandler) { + window.removeEventListener('online', this.onlineHandler); + } + if (this.offlineHandler) { + window.removeEventListener('offline', this.offlineHandler); + } + + this.onlineHandler = () => { + // If we're already connected, don't need to do anything + if (this.conn && this.conn.readyState === WebSocket.OPEN) { + return; + } + + console.log('network online event received, scheduling reconnect'); // eslint-disable-line no-console + + // Set a timer to reconnect after a delay to avoid rapid connection attempts + this.clearReconnectTimeout(); + this.reconnectTimeout = setTimeout( + () => { + this.reconnectTimeout = null; + this.initialize(connectionUrl, token, this.postedAck); + }, + this.config.minWebSocketRetryTime, + ); + }; + + this.offlineHandler = () => { + // If we've detected a full disconnection, don't need to do anything more + if (this.conn && this.conn.readyState !== WebSocket.OPEN) { + return; + } + + console.log('network offline event received, checking connection'); // eslint-disable-line no-console + + // If we haven't detected a full disconnection, + // send a ping immediately to test the socket + // + // NOTE: There is a potential race condition here with the regular ping interval. + // If we send this ping close to when the interval check occurs (e.g., at 29.5s of the 30s interval), + // the server might not have enough time to respond before the interval executes. + // When the interval runs, it will see we're still waiting for a pong and close the connection, + // even though the network might be fine and the server just needs a bit more time to respond. + // This race condition is rare and the impact is just an unnecessary reconnect, + // so we accept this limitation to keep the implementation simple. + this.waitingForPong = true; + this.ping(() => { + this.waitingForPong = false; + }); + }; + + window.addEventListener('online', this.onlineHandler); + window.addEventListener('offline', this.offlineHandler); + // Add connection id, and last_sequence_number to the query param. // We cannot use a cookie because it will bleed across tabs. // We cannot also send it as part of the auth_challenge, because the session cookie is already sent with the request. @@ -144,57 +204,8 @@ export default class WebSocketClient { } else { this.conn = new WebSocket(websocketUrl); } - this.connectionUrl = connectionUrl; - this.conn.onopen = () => { - if (token) { - this.sendMessage('authentication_challenge', {token}); - } - - if (this.connectFailCount > 0) { - console.log('websocket re-established connection'); //eslint-disable-line no-console - - this.reconnectCallback?.(); - this.reconnectListeners.forEach((listener) => listener()); - } else if (this.firstConnectCallback || this.firstConnectListeners.size > 0) { - this.firstConnectCallback?.(); - this.firstConnectListeners.forEach((listener) => listener()); - } - - this.stopPingInterval(); - - // Send a ping immediately to test the socket - var waitingForPong = true; - this.ping(() => { - waitingForPong = false; - }); - - // And every 30 seconds after, checking to ensure - // we're getting responses from the server - this.pingInterval = setInterval( - () => { - if (!waitingForPong) { - waitingForPong = true; - this.ping(() => { - waitingForPong = false; - }); - return; - } - - console.log('ping received no response within time limit: re-establishing websocket'); //eslint-disable-line no-console - - // We are not calling this.close() because we need to auto-restart. - this.connectFailCount = 0; - this.responseSequence = 1; - this.stopPingInterval(); - this.conn?.close(); // Will auto-reconnect after configured retry time - }, - this.config.clientPingInterval); - - this.connectFailCount = 0; - }; - - this.conn.onclose = () => { + const onclose = () => { this.conn = null; this.responseSequence = 1; @@ -231,11 +242,73 @@ export default class WebSocketClient { this.reconnectTimeout = setTimeout( () => { this.reconnectTimeout = null; - this.initialize(this.connectionUrl, token, this.postedAck); + this.initialize(connectionUrl, token, this.postedAck); }, retryTime, ); }; + this.conn.onclose = onclose; + + this.conn.onopen = () => { + if (token) { + this.sendMessage('authentication_challenge', {token}); + } + + if (this.connectFailCount > 0) { + console.log('websocket re-established connection'); //eslint-disable-line no-console + + this.reconnectCallback?.(); + this.reconnectListeners.forEach((listener) => listener()); + } else if (this.firstConnectCallback || this.firstConnectListeners.size > 0) { + this.firstConnectCallback?.(); + this.firstConnectListeners.forEach((listener) => listener()); + } + + this.stopPingInterval(); + + // Send a ping immediately to test the socket + this.waitingForPong = true; + this.ping(() => { + this.waitingForPong = false; + }); + + // And every 30 seconds after, checking to ensure + // we're getting responses from the server + this.pingInterval = setInterval( + () => { + if (!this.waitingForPong) { + this.waitingForPong = true; + this.ping(() => { + this.waitingForPong = false; + }); + return; + } + + this.stopPingInterval(); + + // If we aren't connected, we should already be trying to + // re-connect the websocket. So there's nothing more to do here. + if (!this.conn || this.conn.readyState !== WebSocket.OPEN) { + return; + } + + console.log('ping received no response within time limit: re-establishing websocket'); //eslint-disable-line no-console + + // Calling conn.close() will trigger the onclose callback, + // but sometimes with a significant delay. So instead, we + // call the onclose callback ourselves immediately. We also + // unset the callback on the old connection to ensure it + // is only called once. + this.connectFailCount = 0; + this.responseSequence = 1; + this.conn.onclose = () => {}; + this.conn.close(); + onclose(); + }, + this.config.clientPingInterval); + + this.connectFailCount = 0; + }; this.conn.onerror = (evt) => { if (this.connectFailCount <= 1) { @@ -433,17 +506,31 @@ export default class WebSocketClient { close() { this.connectFailCount = 0; this.responseSequence = 1; - if (this.reconnectTimeout) { - clearTimeout(this.reconnectTimeout); - this.reconnectTimeout = null; - } + this.clearReconnectTimeout(); this.stopPingInterval(); + if (this.conn && this.conn.readyState === WebSocket.OPEN) { this.conn.onclose = () => {}; this.conn.close(); this.conn = null; console.log('websocket closed'); //eslint-disable-line no-console } + + if (this.onlineHandler) { + window.removeEventListener('online', this.onlineHandler); + this.onlineHandler = null; + } + if (this.offlineHandler) { + window.removeEventListener('offline', this.offlineHandler); + this.offlineHandler = null; + } + } + + clearReconnectTimeout() { + if (this.reconnectTimeout) { + clearTimeout(this.reconnectTimeout); + this.reconnectTimeout = null; + } } stopPingInterval() {