forked from eclipse-cdt-cloud/theia-trace-extension
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
refactor+fix trace server url initialization (eclipse-cdt-cloud#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 <paul.marechal@ericsson.com>
- Loading branch information
1 parent
17380d0
commit 40e0c28
Showing
8 changed files
with
175 additions
and
124 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<infer R>) | ||
| ? (...args: A) => Promise<R> | ||
| : never // Discard property. | ||
| }; | ||
|
|
||
| export type LazyTspClientFactory = typeof LazyTspClientFactory; | ||
| export function LazyTspClientFactory(url: Promise<string>): 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; | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
16 changes: 8 additions & 8 deletions
16
theia-extensions/viewer-prototype/src/browser/trace-server-preference.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
144 changes: 102 additions & 42 deletions
144
theia-extensions/viewer-prototype/src/browser/trace-server-url-provider-frontend-impl.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string>; | ||
|
|
||
| }); | ||
| protected _onDidChangeTraceServerUrlEmitter = new Emitter<string>(); | ||
|
|
||
| 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<string> { | ||
| 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<void> { | ||
| this._traceServerUrl = await this.traceViewerEnvironment.getTraceServerUrl(); | ||
| this.updateListeners(); | ||
| async initialize(): Promise<void> { | ||
| // Don't start the application until the Trace Server URL is initialized. | ||
| await this._traceServerUrlPromise; | ||
| } | ||
|
|
||
| async updateListeners(): Promise<void> { | ||
| this._listeners.forEach(listener => listener(this._traceServerUrl)); | ||
| async getTraceServerUrlPromise(): Promise<string> { | ||
| 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); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
21 changes: 13 additions & 8 deletions
21
theia-extensions/viewer-prototype/src/common/trace-server-url-provider.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string>; | ||
| getTraceServerUrlPromise(): Promise<string>; | ||
|
|
||
| /** | ||
| * 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; | ||
| } |
56 changes: 0 additions & 56 deletions
56
theia-extensions/viewer-prototype/src/common/trace-viewer-environment.ts
This file was deleted.
Oops, something went wrong.