Skip to content

Commit

Permalink
Preventing false positive alarms for connection state mode
Browse files Browse the repository at this point in the history 
Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
  • Loading branch information
@vzhukovs
vzhukovs committed Mar 2, 2021
1 parent ab74a91 commit d7dbea3
Show file tree
Hide file tree
Showing 4 changed files with 182 additions and 35 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down
127 changes: 126 additions & 1 deletion packages/core/src/browser/connection-status-service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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));
}
69 changes: 36 additions & 33 deletions packages/core/src/browser/connection-status-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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()
Expand All @@ -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);
Expand All @@ -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);
}
}
}

Expand Down
20 changes: 19 additions & 1 deletion packages/core/src/browser/messaging/ws-connection-provider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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);
}
Expand All @@ -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);
Expand Down Expand Up @@ -98,4 +108,12 @@ export class WebSocketConnectionProvider extends AbstractConnectionProvider<WebS
});
}

protected fireSocketDidOpen(): void {
this.onSocketDidOpenEmitter.fire(undefined);
}

protected fireSocketDidClose(): void {
this.onSocketDidCloseEmitter.fire(undefined);
}

}

0 comments on commit d7dbea3

Please sign in to comment.