diff --git a/CHANGELOG.md b/CHANGELOG.md index 112cd00b621ca..700d9f0a2b2a6 100644 --- a/CHANGELOG.md +++ b/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) [Breaking Changes:](#breaking_changes_1.12.0) diff --git a/packages/core/src/browser/connection-status-service.spec.ts b/packages/core/src/browser/connection-status-service.spec.ts index 9ecac7d26dbd4..04ad38ce59796 100644 --- a/packages/core/src/browser/connection-status-service.spec.ts +++ b/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 = new Emitter(); + const mockSocketClosedEmitter: Emitter = new Emitter(); + const mockIncomingMessageActivityEmitter: Emitter = new Emitter(); + + before(() => { + disableJSDOM = enableJSDOM(); + }); + + after(() => { + disableJSDOM(); + }); + + let timer: sinon.SinonFakeTimers; + let pingSpy: sinon.SinonSpy; + beforeEach(() => { + const mockWebSocketConnectionProvider = sinon.createStubInstance(WebSocketConnectionProvider); + const mockPingService: PingService = { + ping(): Promise { + return Promise.resolve(undefined); + } + }; + const mockILogger: ILogger = { + error(loggable: Loggable): Promise { + 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['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['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['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['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['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); + pingSpy.restore(); + const stub = sinon.stub(pingService, 'ping').onFirstCall().throws('failed to make a ping request'); + const frontendConnectionStatusService = testContainer.get(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 { return new Promise(resolve => setTimeout(resolve, time)); } diff --git a/packages/core/src/browser/connection-status-service.ts b/packages/core/src/browser/connection-status-service.ts index bfc9df6170a50..31986f8ea18f2 100644 --- a/packages/core/src/browser/connection-status-service.ts +++ b/packages/core/src/browser/connection-status-service.ts @@ -81,7 +81,6 @@ export abstract class AbstractConnectionStatusService implements ConnectionStatu protected readonly statusChangeEmitter = new Emitter(); 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 { + 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); + } } } diff --git a/packages/core/src/browser/messaging/ws-connection-provider.ts b/packages/core/src/browser/messaging/ws-connection-provider.ts index 83a367de72a44..56c7e616a58b5 100644 --- a/packages/core/src/browser/messaging/ws-connection-provider.ts +++ b/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 { + protected readonly onSocketDidOpenEmitter: Emitter = new Emitter(); + readonly onSocketDidOpen: Event = this.onSocketDidOpenEmitter.event; + + protected readonly onSocketDidCloseEmitter: Emitter = new Emitter(); + readonly onSocketDidClose: Event = this.onSocketDidCloseEmitter.event; + static createProxy(container: interfaces.Container, path: string, arg?: object): JsonRpcProxy { return container.get(WebSocketConnectionProvider).createProxy(path, arg); } @@ -45,10 +51,14 @@ export class WebSocketConnectionProvider extends AbstractConnectionProvider { + 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