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

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,
       }
     }
   />

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

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<typeof setInterval> | null;
+    private waitingForPong: boolean;
 
+    // reconnectTimeout is used for automatic reconnect after socket close
     private reconnectTimeout: ReturnType<typeof setTimeout> | null;
 
+    // Network event handlers
+    private onlineHandler: (() => void) | null = null;
+    private offlineHandler: (() => void) | null = null;
+
     constructor(config?: Partial<WebSocketClientConfig>) {
         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 = () => {
+        const onclose = () => {
-            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 = () => {
             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) {
+        this.clearReconnectTimeout();
-            clearTimeout(this.reconnectTimeout);
-            this.reconnectTimeout = null;
-        }
         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() {