Preventing false positive alarms of offline mode

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## v1.12.0
 
 - [filesystem] add text input and navigate up icon to file dialog [#8748](https://github.com/eclipse-theia/theia/pull/8748)
+- [core] updated connection status service to prevent false positive alerts about offline mode [#9068](https://github.com/eclipse-theia/theia/pull/9068)
 
 <a name="breaking_changes_1.12.0">[Breaking Changes:](#breaking_changes_1.12.0)</a>
 

packages/core/src/browser/connection-status-service.spec.ts

@@ -25,9 +25,20 @@ FrontendApplicationConfigProvider.set({
 });
 
 import { expect } from 'chai';
-import { ConnectionStatus } from './connection-status-service';
+import {
+    ConnectionStatus,
+    ConnectionStatusOptions,
+    FrontendConnectionStatusService,
+    PingService
+} from './connection-status-service';
 import { MockConnectionStatusService } from './test/mock-connection-status-service';
 
+import * as sinon from 'sinon';
+
+import { Container } from 'inversify';
+import { WebSocketConnectionProvider } from './messaging/ws-connection-provider';
+import { ILogger, Emitter, Loggable } from '../common';
+
 disableJSDOM();
 
 describe('connection-status', function (): void {
@@ -73,6 +84,120 @@ describe('connection-status', function (): void {
 
 });
 
+describe('frontend-connection-status', function (): void {
+    const OFFLINE_TIMEOUT = 10;
+
+    let testContainer: Container;
+
+    const mockSocketOpenedEmitter: Emitter<void> = new Emitter();
+    const mockSocketClosedEmitter: Emitter<void> = new Emitter();
+    const mockIncomingMessageActivityEmitter: Emitter<void> = new Emitter();
+
+    before(() => {
+        disableJSDOM = enableJSDOM();
+    });
+
+    after(() => {
+        disableJSDOM();
+    });
+
+    let timer: sinon.SinonFakeTimers;
+    let pingSpy: sinon.SinonSpy;
+    beforeEach(() => {
+        const mockWebSocketConnectionProvider = sinon.createStubInstance(WebSocketConnectionProvider);
+        const mockPingService: PingService = <PingService>{
+            ping(): Promise<void> {
+                return Promise.resolve(undefined);
+            }
+        };
+        const mockILogger: ILogger = <ILogger>{
+            error(loggable: Loggable): Promise<void> {
+                return Promise.resolve(undefined);
+            }
+        };
+
+        testContainer = new Container();
+        testContainer.bind(FrontendConnectionStatusService).toSelf().inSingletonScope();
+        testContainer.bind(PingService).toConstantValue(mockPingService);
+        testContainer.bind(ILogger).toConstantValue(mockILogger);
+        testContainer.bind(ConnectionStatusOptions).toConstantValue({ offlineTimeout: OFFLINE_TIMEOUT });
+        testContainer.bind(WebSocketConnectionProvider).toConstantValue(mockWebSocketConnectionProvider);
+
+        sinon.stub(mockWebSocketConnectionProvider, 'onSocketDidOpen').value(mockSocketOpenedEmitter.event);
+        sinon.stub(mockWebSocketConnectionProvider, 'onSocketDidClose').value(mockSocketClosedEmitter.event);
+        sinon.stub(mockWebSocketConnectionProvider, 'onIncomingMessageActivity').value(mockIncomingMessageActivityEmitter.event);
+
+        timer = sinon.useFakeTimers();
+
+        pingSpy = sinon.spy(mockPingService, 'ping');
+    });
+
+    afterEach(() => {
+        pingSpy.restore();
+        timer.restore();
+        testContainer.unbindAll();
+    });
+
+    it('should switch status to offline on websocket close', () => {
+        const frontendConnectionStatusService = testContainer.get<FrontendConnectionStatusService>(FrontendConnectionStatusService);
+        frontendConnectionStatusService['init']();
+        expect(frontendConnectionStatusService.currentStatus).to.be.equal(ConnectionStatus.ONLINE);
+        mockSocketClosedEmitter.fire(undefined);
+        expect(frontendConnectionStatusService.currentStatus).to.be.equal(ConnectionStatus.OFFLINE);
+    });
+
+    it('should switch status to online on websocket established', () => {
+        const frontendConnectionStatusService = testContainer.get<FrontendConnectionStatusService>(FrontendConnectionStatusService);
+        frontendConnectionStatusService['init']();
+        mockSocketClosedEmitter.fire(undefined);
+        expect(frontendConnectionStatusService.currentStatus).to.be.equal(ConnectionStatus.OFFLINE);
+        mockSocketOpenedEmitter.fire(undefined);
+        expect(frontendConnectionStatusService.currentStatus).to.be.equal(ConnectionStatus.ONLINE);
+    });
+
+    it('should switch status to online on any websocket activity', () => {
+        const frontendConnectionStatusService = testContainer.get<FrontendConnectionStatusService>(FrontendConnectionStatusService);
+        frontendConnectionStatusService['init']();
+        mockSocketClosedEmitter.fire(undefined);
+        expect(frontendConnectionStatusService.currentStatus).to.be.equal(ConnectionStatus.OFFLINE);
+        mockIncomingMessageActivityEmitter.fire(undefined);
+        expect(frontendConnectionStatusService.currentStatus).to.be.equal(ConnectionStatus.ONLINE);
+    });
+
+    it('should perform ping request after socket activity', () => {
+        const frontendConnectionStatusService = testContainer.get<FrontendConnectionStatusService>(FrontendConnectionStatusService);
+        frontendConnectionStatusService['init']();
+        mockIncomingMessageActivityEmitter.fire(undefined);
+        expect(frontendConnectionStatusService.currentStatus).to.be.equal(ConnectionStatus.ONLINE);
+        sinon.assert.notCalled(pingSpy);
+        timer.tick(OFFLINE_TIMEOUT);
+        sinon.assert.calledOnce(pingSpy);
+    });
+
+    it('should not perform ping request before desired timeout',  () => {
+        const frontendConnectionStatusService = testContainer.get<FrontendConnectionStatusService>(FrontendConnectionStatusService);
+        frontendConnectionStatusService['init']();
+        mockIncomingMessageActivityEmitter.fire(undefined);
+        expect(frontendConnectionStatusService.currentStatus).to.be.equal(ConnectionStatus.ONLINE);
+        sinon.assert.notCalled(pingSpy);
+        timer.tick(OFFLINE_TIMEOUT - 1);
+        sinon.assert.notCalled(pingSpy);
+    });
+
+    it('should switch to offline mode if ping request was rejected', () => {
+        const pingService = testContainer.get<PingService>(PingService);
+        pingSpy.restore();
+        const stub = sinon.stub(pingService, 'ping').onFirstCall().throws('failed to make a ping request');
+        const frontendConnectionStatusService = testContainer.get<FrontendConnectionStatusService>(FrontendConnectionStatusService);
+        frontendConnectionStatusService['init']();
+        mockIncomingMessageActivityEmitter.fire(undefined);
+        expect(frontendConnectionStatusService.currentStatus).to.be.equal(ConnectionStatus.ONLINE);
+        timer.tick(OFFLINE_TIMEOUT);
+        sinon.assert.calledOnce(stub);
+        expect(frontendConnectionStatusService.currentStatus).to.be.equal(ConnectionStatus.OFFLINE);
+    });
+});
+
 function pause(time: number = 1): Promise<unknown> {
     return new Promise(resolve => setTimeout(resolve, time));
 }

packages/core/src/browser/connection-status-service.ts

@@ -81,7 +81,6 @@ export abstract class AbstractConnectionStatusService implements ConnectionStatu
     protected readonly statusChangeEmitter = new Emitter<ConnectionStatus>();
 
     protected connectionStatus: ConnectionStatus = ConnectionStatus.ONLINE;
-    protected timer: number | undefined;
 
     @inject(ILogger)
     protected readonly logger: ILogger;
@@ -98,42 +97,21 @@ export abstract class AbstractConnectionStatusService implements ConnectionStatu
 
     dispose(): void {
         this.statusChangeEmitter.dispose();
-        if (this.timer) {
-            this.clearTimeout(this.timer);
-        }
     }
 
     protected updateStatus(success: boolean): void {
-        // clear existing timer
-        if (this.timer) {
-            this.clearTimeout(this.timer);
-        }
         const previousStatus = this.connectionStatus;
         const newStatus = success ? ConnectionStatus.ONLINE : ConnectionStatus.OFFLINE;
         if (previousStatus !== newStatus) {
             this.connectionStatus = newStatus;
             this.fireStatusChange(newStatus);
         }
-        // schedule offline
-        this.timer = this.setTimeout(() => {
-            this.logger.trace(`No activity for ${this.options.offlineTimeout} ms. We are offline.`);
-            this.updateStatus(false);
-        }, this.options.offlineTimeout);
     }
 
     protected fireStatusChange(status: ConnectionStatus): void {
         this.statusChangeEmitter.fire(status);
     }
 
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    protected setTimeout(handler: (...args: any[]) => void, timeout: number): number {
-        return window.setTimeout(handler, timeout);
-    }
-
-    protected clearTimeout(handle: number): void {
-        window.clearTimeout(handle);
-    }
-
 }
 
 @injectable()
@@ -144,9 +122,20 @@ export class FrontendConnectionStatusService extends AbstractConnectionStatusSer
     @inject(WebSocketConnectionProvider) protected readonly wsConnectionProvider: WebSocketConnectionProvider;
     @inject(PingService) protected readonly pingService: PingService;
 
+    constructor(@inject(ConnectionStatusOptions) @optional() protected readonly options: ConnectionStatusOptions = ConnectionStatusOptions.DEFAULT) {
+        super(options);
+    }
+
     @postConstruct()
     protected init(): void {
-        this.schedulePing();
+        this.wsConnectionProvider.onSocketDidOpen(() => {
+            this.updateStatus(true);
+            this.schedulePing();
+        });
+        this.wsConnectionProvider.onSocketDidClose(() => {
+            this.clearTimeout(this.scheduledPing);
+            this.updateStatus(false);
+        });
         this.wsConnectionProvider.onIncomingMessageActivity(() => {
             // natural activity
             this.updateStatus(true);
@@ -155,18 +144,32 @@ export class FrontendConnectionStatusService extends AbstractConnectionStatusSer
     }
 
     protected schedulePing(): void {
-        if (this.scheduledPing) {
-            this.clearTimeout(this.scheduledPing);
-        }
+        this.clearTimeout(this.scheduledPing);
         this.scheduledPing = this.setTimeout(async () => {
-            try {
-                await this.pingService.ping();
-                this.updateStatus(true);
-            } catch (e) {
-                this.logger.trace(e);
-            }
+            await this.performPingRequest();
             this.schedulePing();
-        }, this.options.offlineTimeout * 0.8);
+        }, this.options.offlineTimeout);
+    }
+
+    protected async performPingRequest(): Promise<void> {
+        try {
+            await this.pingService.ping();
+            this.updateStatus(true);
+        } catch (e) {
+            this.updateStatus(false);
+            await this.logger.error(e);
+        }
+    }
+
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    protected setTimeout(handler: (...args: any[]) => void, timeout: number): number {
+        return window.setTimeout(handler, timeout);
+    }
+
+    protected clearTimeout(handle?: number): void {
+        if (handle !== undefined) {
+            window.clearTimeout(handle);
+        }
     }
 }
 

packages/core/src/browser/messaging/ws-connection-provider.ts

@@ -15,7 +15,7 @@
  ********************************************************************************/
 
 import { injectable, interfaces, decorate, unmanaged } from 'inversify';
-import { JsonRpcProxyFactory, JsonRpcProxy } from '../../common';
+import { JsonRpcProxyFactory, JsonRpcProxy, Emitter, Event } from '../../common';
 import { WebSocketChannel } from '../../common/messaging/web-socket-channel';
 import { Endpoint } from '../endpoint';
 import ReconnectingWebSocket from 'reconnecting-websocket';
@@ -34,6 +34,12 @@ export interface WebSocketOptions {
 @injectable()
 export class WebSocketConnectionProvider extends AbstractConnectionProvider<WebSocketOptions> {
 
+    protected readonly onSocketDidOpenEmitter: Emitter<void> = new Emitter();
+    readonly onSocketDidOpen: Event<void> = this.onSocketDidOpenEmitter.event;
+
+    protected readonly onSocketDidCloseEmitter: Emitter<void> = new Emitter();
+    readonly onSocketDidClose: Event<void> = this.onSocketDidCloseEmitter.event;
+
     static createProxy<T extends object>(container: interfaces.Container, path: string, arg?: object): JsonRpcProxy<T> {
         return container.get(WebSocketConnectionProvider).createProxy<T>(path, arg);
     }
@@ -45,10 +51,14 @@ export class WebSocketConnectionProvider extends AbstractConnectionProvider<WebS
         const url = this.createWebSocketUrl(WebSocketChannel.wsPath);
         const socket = this.createWebSocket(url);
         socket.onerror = console.error;
+        socket.onopen = () => {
+            this.fireSocketDidOpen();
+        };
         socket.onclose = ({ code, reason }) => {
             for (const channel of [...this.channels.values()]) {
                 channel.close(code, reason);
             }
+            this.fireSocketDidClose();
         };
         socket.onmessage = ({ data }) => {
             this.handleIncomingRawMessage(data);
@@ -98,4 +108,12 @@ export class WebSocketConnectionProvider extends AbstractConnectionProvider<WebS
         });
     }
 
+    protected fireSocketDidOpen(): void {
+        this.onSocketDidOpenEmitter.fire(undefined);
+    }
+
+    protected fireSocketDidClose(): void {
+        this.onSocketDidCloseEmitter.fire(undefined);
+    }
+
 }