From 40e0c28c18cd3018cfa29ad691cc268b0a228d57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20Mar=C3=A9chal?= Date: Mon, 27 Sep 2021 14:20:47 -0400 Subject: [PATCH] refactor+fix trace server url initialization (#502) Both `TraceServerUrlProviderImpl` and `TraceViewerEnvironment` are riddled with concurrency issues. A lot of events happen asynchronously but the logic doesn't take this into account, leaving the application vulnerable to race conditions when trying to fetch the trace server url. Refactor by removing `TraceViewerEnvironment` and moving its functionality directly into `TraceServerUrlProviderImpl`, add logic to prevent race conditions. Signed-off-by: Paul Marechal --- packages/base/src/lazy-tsp-client.ts | 35 +++++ .../src/browser/trace-server-bindings.ts | 1 - .../src/browser/trace-server-preference.ts | 16 +- ...trace-server-url-provider-frontend-impl.ts | 144 +++++++++++++----- .../trace-viewer-frontend-module.ts | 4 +- .../src/browser/tsp-client-provider-impl.ts | 22 ++- .../src/common/trace-server-url-provider.ts | 21 ++- .../src/common/trace-viewer-environment.ts | 56 ------- 8 files changed, 175 insertions(+), 124 deletions(-) create mode 100644 packages/base/src/lazy-tsp-client.ts delete mode 100644 theia-extensions/viewer-prototype/src/common/trace-viewer-environment.ts diff --git a/packages/base/src/lazy-tsp-client.ts b/packages/base/src/lazy-tsp-client.ts new file mode 100644 index 000000000..c85b35457 --- /dev/null +++ b/packages/base/src/lazy-tsp-client.ts @@ -0,0 +1,35 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ + +import { TspClient } from 'tsp-typescript-client'; + +/** + * Hack! + * The `LazyTspClient` replaces _every_ method with an asynchronous one. + * Only keep methods, discard properties. + */ +export type LazyTspClient = { + [K in keyof TspClient]: TspClient[K] extends (...args: infer A) => (infer R | Promise) + ? (...args: A) => Promise + : never // Discard property. +}; + +export type LazyTspClientFactory = typeof LazyTspClientFactory; +export function LazyTspClientFactory(url: Promise): TspClient { + // All methods from the `TspClient` are asynchronous. The `LazyTspClient` + // will just delay each call to its methods by first awaiting for the + // asynchronous `baseUrl` resolution to then get a valid `TspClient`. + const tspClientPromise = url.then(baseUrl => new TspClient(baseUrl)); + // eslint-disable-next-line no-null/no-null + return new Proxy(Object.create(null), { + get(target, property, _receiver) { + let method = target[property]; + if (!method) { + target[property] = method = async (...args: any[]) => { + const tspClient = await tspClientPromise as any; + return tspClient[property](...args); + }; + } + return method; + } + }) as LazyTspClient as TspClient; +} diff --git a/theia-extensions/viewer-prototype/src/browser/trace-server-bindings.ts b/theia-extensions/viewer-prototype/src/browser/trace-server-bindings.ts index 7d6118faf..608bc5d33 100644 --- a/theia-extensions/viewer-prototype/src/browser/trace-server-bindings.ts +++ b/theia-extensions/viewer-prototype/src/browser/trace-server-bindings.ts @@ -7,7 +7,6 @@ export function bindTraceServerPreferences(bind: interfaces.Bind): void { const preferences = ctx.container.get(PreferenceService); return createPreferenceProxy(preferences, ServerSchema); }).inSingletonScope(); - bind(PreferenceContribution).toConstantValue({ schema: ServerSchema, }); diff --git a/theia-extensions/viewer-prototype/src/browser/trace-server-preference.ts b/theia-extensions/viewer-prototype/src/browser/trace-server-preference.ts index 0b58fb586..c3031215d 100644 --- a/theia-extensions/viewer-prototype/src/browser/trace-server-preference.ts +++ b/theia-extensions/viewer-prototype/src/browser/trace-server-preference.ts @@ -1,24 +1,24 @@ import { PreferenceSchema, PreferenceProxy, PreferenceScope } from '@theia/core/lib/browser'; +import { TRACE_SERVER_DEFAULT_PORT } from '../common/trace-server-url-provider'; export const TRACE_PATH = 'trace-viewer.path'; export const TRACE_PORT = 'trace-viewer.port'; export const ServerSchema: PreferenceSchema = { + scope: PreferenceScope.Folder, type: 'object', properties: { [TRACE_PATH]: { - 'type': 'string', - 'default': '', - 'description': 'The path to trace-server executable, e.g.: /usr/bin/tracecompass-server', + type: 'string', + default: '', + description: 'The path to trace-server executable, e.g.: /usr/bin/tracecompass-server', }, [TRACE_PORT]: { - 'type': 'number', - 'default': '', - 'description': 'Specify the port on which you want to execute the server. This change will take effect the next time you open a trace', + type: 'number', + default: TRACE_SERVER_DEFAULT_PORT, + description: 'Specify the port on which you want to execute the server. This change will take effect the next time you open a trace', } - }, - scope: PreferenceScope.Folder, }; interface TracePreferenceContribution { diff --git a/theia-extensions/viewer-prototype/src/browser/trace-server-url-provider-frontend-impl.ts b/theia-extensions/viewer-prototype/src/browser/trace-server-url-provider-frontend-impl.ts index 9c73937d4..1ba5f3232 100644 --- a/theia-extensions/viewer-prototype/src/browser/trace-server-url-provider-frontend-impl.ts +++ b/theia-extensions/viewer-prototype/src/browser/trace-server-url-provider-frontend-impl.ts @@ -1,66 +1,126 @@ +import { Emitter, Event, MessageService } from '@theia/core'; +import { FrontendApplicationContribution } from '@theia/core/lib/browser'; +import { EnvVariablesServer } from '@theia/core/lib/common/env-variables/env-variables-protocol'; import { inject, injectable } from 'inversify'; -import { TraceViewerEnvironment } from '../common/trace-viewer-environment'; -import { TraceServerUrlProvider, TRACE_SERVER_DEFAULT_URL, TRACE_SERVER_DEFAULT_PORT } from '../common/trace-server-url-provider'; -import { FrontendApplicationContribution, FrontendApplication, PreferenceService } from '@theia/core/lib/browser'; -import { TRACE_PORT } from './trace-server-preference'; import { TraceServerConfigService } from '../common/trace-server-config'; -import { MessageService } from '@theia/core'; +import { TraceServerUrlProvider, TRACE_SERVER_DEFAULT_URL } from '../common/trace-server-url-provider'; +import { TracePreferences, TRACE_PORT } from './trace-server-preference'; @injectable() export class TraceServerUrlProviderImpl implements TraceServerUrlProvider, FrontendApplicationContribution { - protected _traceServerUrl: string; - protected _listeners: ((url: string) => void)[]; - private port: string | undefined; + /** + * The Trace Server URL resolved from a URL template and a port number. + * Updated each time the port is changed from the preferences. + * `undefined` until both `_traceServerUrlTemplate` and `_traceServerPort` are initialized. + */ + protected _traceServerUrl?: string; - constructor( - @inject(TraceViewerEnvironment) protected readonly traceViewerEnvironment: TraceViewerEnvironment, - @inject(PreferenceService) protected readonly preferenceService: PreferenceService, - @inject(TraceServerConfigService) protected readonly traceServerConfigService: TraceServerConfigService, - @inject(MessageService) protected readonly messageService: MessageService + /** + * The Trace Server URL template. + * The `{}` characters will be replaced with the port defined in the preferences. + * `undefined` until fetched from the remote environment. + */ + protected _traceServerUrlTemplate?: string; - ) { - this.port = this.preferenceService.get(TRACE_PORT); - this.preferenceService.onPreferenceChanged(async event => { - if (event.preferenceName === TRACE_PORT) { - try { - await this.traceServerConfigService.stopTraceServer(); - this.messageService.info(`Trace server disconnected on port: ${this.port}.`); - } catch (e){ - // Do not show the error incase the user tries to modify the port before starting a server - } - this.port = event.newValue; - this._traceServerUrl = TRACE_SERVER_DEFAULT_URL.replace(/{}/g, this.port ? this.port : TRACE_SERVER_DEFAULT_PORT); - this.updateListeners(); + /** + * A configurable port number from the preferences. + * `undefined` until fetched from the preferences. + */ + protected _traceServerPort?: number; - } + /** + * Internal promise that is pending until `_traceServerUrl` is initialized. + * After then, each update of the Trace Server URL will create a new reference + * to a promise resolved with the new value. + */ + protected _traceServerUrlPromise: Promise; - }); + protected _onDidChangeTraceServerUrlEmitter = new Emitter(); - this._traceServerUrl = TRACE_SERVER_DEFAULT_URL.replace(/{}/g, this.port ? this.port : TRACE_SERVER_DEFAULT_PORT); - this._listeners = []; + /** + * Listen for updates to the Trace Server URL. + * Fired when Trace Server URL is first initiliazed and when the `TRACE_PORT` preference changes. + */ + get onDidChangeTraceServerUrl(): Event { + return this._onDidChangeTraceServerUrlEmitter.event; + } + + constructor( + @inject(EnvVariablesServer) protected environment: EnvVariablesServer, + @inject(TracePreferences) protected tracePreferences: TracePreferences, + @inject(TraceServerConfigService) protected traceServerConfigService: TraceServerConfigService, + @inject(MessageService) protected messageService: MessageService, + ) { + this._traceServerUrlPromise = new Promise(resolve => { + const self = this.onDidChangeTraceServerUrl(url => { + self.dispose(); + resolve(url); + }); + }); + // Get the URL template from the remote environment. + this.environment.getValue('TRACE_SERVER_URL').then(variable => { + const url = variable?.value; + this._traceServerUrlTemplate = url + ? this.normalizeUrl(url) + : TRACE_SERVER_DEFAULT_URL; + this.updateTraceServerUrl(); + }); + // Get the configurable port from Theia's preferences. + this.tracePreferences.ready.then(() => { + this._traceServerPort = this.tracePreferences[TRACE_PORT]; + this.updateTraceServerUrl(); + this.tracePreferences.onPreferenceChanged(async event => { + if (event.preferenceName === TRACE_PORT) { + this._traceServerPort = event.newValue; + this.updateTraceServerUrl(); + try { + await this.traceServerConfigService.stopTraceServer(); + this.messageService.info(`Trace server disconnected on port: ${event.oldValue}.`); + } catch (_) { + // Do not show the error incase the user tries to modify the port before starting a server + } + } + }); + }); } - async onStart(_app: FrontendApplication): Promise { - this._traceServerUrl = await this.traceViewerEnvironment.getTraceServerUrl(); - this.updateListeners(); + async initialize(): Promise { + // Don't start the application until the Trace Server URL is initialized. + await this._traceServerUrlPromise; } - async updateListeners(): Promise { - this._listeners.forEach(listener => listener(this._traceServerUrl)); + async getTraceServerUrlPromise(): Promise { + return this._traceServerUrlPromise; } getTraceServerUrl(): string { + if (this._traceServerUrl === undefined) { + throw new Error('The Trace Server URL is not yet defined. Try using getTraceServerUrlPromise.'); + } return this._traceServerUrl; } - /** - * Add a listener for trace server url changes - * @param listener The listener function to be called when the url is - * changed - */ - addTraceServerUrlChangedListener(listener: (url: string) => void): void { - this._listeners.push(listener); + protected normalizeUrl(url: string): string { + url = url.toLowerCase(); + // Add missing http protocol. + if (!url.startsWith('http://') && !url.startsWith('https://')) { + url = 'http://' + url; + } + // Remove trailing `/`. + if (url.endsWith('/')) { + url = url.substring(0, url.length - 1); + } + return url; } + protected updateTraceServerUrl(): void { + if (this._traceServerUrlTemplate === undefined || this._traceServerPort === undefined) { + return; // State is only partially initialized = try again later. + } + const traceServerUrl = this._traceServerUrlTemplate.replace(/{}/g, this._traceServerPort.toString()); + this._traceServerUrl = traceServerUrl; + this._traceServerUrlPromise = Promise.resolve(traceServerUrl); + this._onDidChangeTraceServerUrlEmitter.fire(traceServerUrl); + } } diff --git a/theia-extensions/viewer-prototype/src/browser/trace-viewer/trace-viewer-frontend-module.ts b/theia-extensions/viewer-prototype/src/browser/trace-viewer/trace-viewer-frontend-module.ts index c033f4e36..6f3b61e2f 100644 --- a/theia-extensions/viewer-prototype/src/browser/trace-viewer/trace-viewer-frontend-module.ts +++ b/theia-extensions/viewer-prototype/src/browser/trace-viewer/trace-viewer-frontend-module.ts @@ -2,7 +2,6 @@ import { ContainerModule, Container } from 'inversify'; import { WidgetFactory, OpenHandler, FrontendApplicationContribution, bindViewContribution, WebSocketConnectionProvider } from '@theia/core/lib/browser'; import { TraceViewerWidget, TraceViewerWidgetOptions } from './trace-viewer'; import { TraceViewerContribution } from './trace-viewer-contribution'; -import { TraceViewerEnvironment } from '../../common/trace-viewer-environment'; import { TraceServerUrlProvider } from '../../common/trace-server-url-provider'; import { CommandContribution } from '@theia/core/lib/common'; import 'ag-grid-community/dist/styles/ag-grid.css'; @@ -19,12 +18,13 @@ import { bindTraceServerPreferences } from '../trace-server-bindings'; import { TraceServerConfigService, traceServerPath } from '../../common/trace-server-config'; import { TabBarToolbarContribution } from '@theia/core/lib/browser/shell/tab-bar-toolbar'; import { TraceViewerToolbarContribution } from './trace-viewer-toolbar-contribution'; +import { LazyTspClientFactory } from 'traceviewer-base/lib/lazy-tsp-client'; export default new ContainerModule(bind => { - bind(TraceViewerEnvironment).toSelf().inRequestScope(); bind(TraceServerUrlProviderImpl).toSelf().inSingletonScope(); bind(FrontendApplicationContribution).toService(TraceServerUrlProviderImpl); bind(TraceServerUrlProvider).toService(TraceServerUrlProviderImpl); + bind(LazyTspClientFactory).toFunction(LazyTspClientFactory); bind(TspClientProvider).toSelf().inSingletonScope(); bind(TheiaMessageManager).toSelf().inSingletonScope(); diff --git a/theia-extensions/viewer-prototype/src/browser/tsp-client-provider-impl.ts b/theia-extensions/viewer-prototype/src/browser/tsp-client-provider-impl.ts index 603bc83a1..2e84cd159 100644 --- a/theia-extensions/viewer-prototype/src/browser/tsp-client-provider-impl.ts +++ b/theia-extensions/viewer-prototype/src/browser/tsp-client-provider-impl.ts @@ -4,6 +4,7 @@ import { TspClient } from 'tsp-typescript-client/lib/protocol/tsp-client'; import { ExperimentManager } from 'traceviewer-base/lib/experiment-manager'; import { TraceManager } from 'traceviewer-base/lib/trace-manager'; import { ITspClientProvider } from 'traceviewer-base/lib/tsp-client-provider'; +import { LazyTspClientFactory } from 'traceviewer-base/lib/lazy-tsp-client'; @injectable() export class TspClientProvider implements ITspClientProvider { @@ -14,17 +15,24 @@ export class TspClientProvider implements ITspClientProvider { private _listeners: ((tspClient: TspClient) => void)[]; constructor( - @inject(TraceServerUrlProvider) private tspUrlProvider: TraceServerUrlProvider + @inject(TraceServerUrlProvider) private tspUrlProvider: TraceServerUrlProvider, + @inject(LazyTspClientFactory) private lazyTspClientFactory: LazyTspClientFactory, ) { - this._tspClient = new TspClient(this.tspUrlProvider.getTraceServerUrl()); + const traceServerUrlPromise = this.tspUrlProvider.getTraceServerUrlPromise(); + this._tspClient = this.lazyTspClientFactory(traceServerUrlPromise); this._traceManager = new TraceManager(this._tspClient); this._experimentManager = new ExperimentManager(this._tspClient, this._traceManager); this._listeners = []; - tspUrlProvider.addTraceServerUrlChangedListener(url => { - this._tspClient = new TspClient(url); - this._traceManager = new TraceManager(this._tspClient); - this._experimentManager = new ExperimentManager(this._tspClient, this._traceManager); - this._listeners.forEach(listener => listener(this._tspClient)); + // Skip the first event fired when the Trace Server URL gets initialized. + traceServerUrlPromise.then(() => { + tspUrlProvider.onDidChangeTraceServerUrl(url => { + this._tspClient = new TspClient(url); + this._traceManager = new TraceManager(this._tspClient); + this._experimentManager = new ExperimentManager(this._tspClient, this._traceManager); + for (const listener of this._listeners) { + listener(this._tspClient); + } + }); }); } diff --git a/theia-extensions/viewer-prototype/src/common/trace-server-url-provider.ts b/theia-extensions/viewer-prototype/src/common/trace-server-url-provider.ts index 2b90f4dc2..77f7679f6 100644 --- a/theia-extensions/viewer-prototype/src/common/trace-server-url-provider.ts +++ b/theia-extensions/viewer-prototype/src/common/trace-server-url-provider.ts @@ -1,19 +1,24 @@ -export const TraceServerUrlProvider = Symbol('TraceServerUrlProvider'); export const TRACE_SERVER_DEFAULT_URL = 'http://localhost:{}/tsp/api'; export const TRACE_SERVER_DEFAULT_PORT = '8080'; +export const TraceServerUrlProvider = Symbol('TraceServerUrlProvider'); export interface TraceServerUrlProvider { - /** - * Get the default trace server URL from the server + * Get a promise that resolves once the Trace Server URL is initialized. + * @returns a new promise each time `.onDidChangeTraceServerUrl` fires. */ - getTraceServerUrl(): Readonly; + getTraceServerUrlPromise(): Promise; /** - * Add a listener for trace server url changes - * @param listener The listener function to be called when the url is - * changed + * Get the default Trace Server URL from the server. + * Will throw if called before initialization. See `getTraceServerUrlPromise` + * to get a promise to the value. */ - addTraceServerUrlChangedListener(listener: (url: string) => void): void; + getTraceServerUrl(): string; + /** + * Get notified when the Trace Server URL changes. + * @param listener function to be called when the url is changed. + */ + onDidChangeTraceServerUrl(listener: (url: string) => void): void; } diff --git a/theia-extensions/viewer-prototype/src/common/trace-viewer-environment.ts b/theia-extensions/viewer-prototype/src/common/trace-viewer-environment.ts deleted file mode 100644 index 978de64be..000000000 --- a/theia-extensions/viewer-prototype/src/common/trace-viewer-environment.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { injectable, inject } from 'inversify'; -import { EnvVariablesServer } from '@theia/core/lib/common/env-variables'; -import { TRACE_SERVER_DEFAULT_URL, TRACE_SERVER_DEFAULT_PORT } from './trace-server-url-provider'; -import { PreferenceService } from '@theia/core/lib/browser'; -import { TRACE_PORT } from '../browser/trace-server-preference'; -import { TraceServerConfigService } from './trace-server-config'; -import { MessageService } from '@theia/core'; - -@injectable() -export class TraceViewerEnvironment { - private port: string | undefined; - - constructor( - @inject(EnvVariablesServer) protected readonly environments: EnvVariablesServer, - @inject(PreferenceService) protected readonly preferenceService: PreferenceService, - @inject(TraceServerConfigService) protected readonly traceServerConfigService: TraceServerConfigService, - @inject(MessageService) protected readonly messageService: MessageService) { - - this.port = this.preferenceService.get(TRACE_PORT); - this.preferenceService.onPreferenceChanged(async event => { - if (event.preferenceName === TRACE_PORT) { - try { - await this.traceServerConfigService.stopTraceServer(); - this.messageService.info(`Trace server disconnected on port: ${this.port}.`); - } catch (e){ - // Do not show the error incase the user tries to modify the port before starting a server - } - this.port = event.newValue; - this._traceServerUrl = TRACE_SERVER_DEFAULT_URL.replace(/{}/g, this.port ? this.port : TRACE_SERVER_DEFAULT_PORT); - } - }); - } - - protected _traceServerUrl: string | undefined; - async getTraceServerUrl(): Promise { - if (!this._traceServerUrl) { - const traceServerUrl = await this.environments.getValue('TRACE_SERVER_URL'); - this._traceServerUrl = traceServerUrl ? this.parseUrl(traceServerUrl.value || TRACE_SERVER_DEFAULT_URL) : TRACE_SERVER_DEFAULT_URL; - } - return this._traceServerUrl.replace(/{}/g, this.port ? this.port : TRACE_SERVER_DEFAULT_PORT); - } - - private parseUrl(url: string): string { - let lcUrl = url.toLowerCase(); - // Add the http - if (!lcUrl.startsWith('http')) { - lcUrl = 'http://' + lcUrl; - } - // Make sure it does not end with a slash - if (lcUrl.endsWith('/')) { - lcUrl = lcUrl.substring(0, lcUrl.length - 1); - } - return lcUrl; - } - -}