From ec912b32898540dd2bcef0aabeba201c74eda581 Mon Sep 17 00:00:00 2001 From: Winston Liu Date: Thu, 28 Apr 2022 22:50:05 -0700 Subject: [PATCH 01/11] Convert autoStart into an async function --- src/omnisharp/server.ts | 106 ++++++++++++++++++++-------------------- 1 file changed, 53 insertions(+), 53 deletions(-) diff --git a/src/omnisharp/server.ts b/src/omnisharp/server.ts index fd82bba9b..2ba7d6537 100644 --- a/src/omnisharp/server.ts +++ b/src/omnisharp/server.ts @@ -546,67 +546,67 @@ export class OmniSharpServer { } } - public autoStart(preferredPath: string): Thenable { + public async autoStart(preferredPath: string): Promise { const options = this.optionProvider.GetLatestOptions(); - return findLaunchTargets(options).then(async launchTargets => { - // If there aren't any potential launch targets, we create file watcher and try to - // start the server again once a *.sln, *.slnf, *.csproj, project.json, CSX or Cake file is created. - if (launchTargets.length === 0) { - return new Promise((resolve, reject) => { - // 1st watch for files - let watcher = this.vscode.workspace.createFileSystemWatcher('{**/*.sln,**/*.slnf,**/*.csproj,**/project.json,**/*.csx,**/*.cake}', - /*ignoreCreateEvents*/ false, - /*ignoreChangeEvents*/ true, - /*ignoreDeleteEvents*/ true); - - watcher.onDidCreate(uri => { - watcher.dispose(); - resolve(); - }); - }).then(() => { - // 2nd try again - return this.autoStart(preferredPath); + const launchTargets = await findLaunchTargets(options); + + // If there aren't any potential launch targets, we create file watcher and try to + // start the server again once a *.sln, *.slnf, *.csproj, project.json, CSX or Cake file is created. + if (launchTargets.length === 0) { + await new Promise((resolve, reject) => { + // 1st watch for files + const watcher = this.vscode.workspace.createFileSystemWatcher('{**/*.sln,**/*.slnf,**/*.csproj,**/project.json,**/*.csx,**/*.cake}', + /*ignoreCreateEvents*/ false, + /*ignoreChangeEvents*/ true, + /*ignoreDeleteEvents*/ true); + + watcher.onDidCreate(uri => { + watcher.dispose(); + resolve(); }); - } - else if (launchTargets.length === 1) { - // If there's only one target, just start - return this.restart(launchTargets[0]); - } + }); - // First, try to launch against something that matches the user's preferred target - const defaultLaunchSolutionConfigValue = this.optionProvider.GetLatestOptions().defaultLaunchSolution; - const defaultLaunchSolutionTarget = launchTargets.find((a) => (path.basename(a.target) === defaultLaunchSolutionConfigValue)); - if (defaultLaunchSolutionTarget) { - return this.restart(defaultLaunchSolutionTarget); - } + // 2nd try again + return this.autoStart(preferredPath); + } + else if (launchTargets.length === 1) { + // If there's only one target, just start + return this.restart(launchTargets[0]); + } - // If there's more than one launch target, we start the server if one of the targets - // matches the preferred path. - if (preferredPath.length > 0) { - const preferredLaunchTarget = launchTargets.find((a) => a.target === preferredPath); - if (preferredLaunchTarget) { - return this.restart(preferredLaunchTarget); - } - } + // First, try to launch against something that matches the user's preferred target + const defaultLaunchSolutionConfigValue = this.optionProvider.GetLatestOptions().defaultLaunchSolution; + const defaultLaunchSolutionTarget = launchTargets.find((a) => (path.basename(a.target) === defaultLaunchSolutionConfigValue)); + if (defaultLaunchSolutionTarget) { + return this.restart(defaultLaunchSolutionTarget); + } - // To maintain previous behavior when there are mulitple targets available, - // launch with first Solution or Folder target. - const firstFolderOrSolutionTarget = launchTargets - .find(target => target.workspaceKind == LaunchTargetKind.Folder || target.workspaceKind == LaunchTargetKind.Solution); - if (firstFolderOrSolutionTarget) { - return this.restart(firstFolderOrSolutionTarget); + // If there's more than one launch target, we start the server if one of the targets + // matches the preferred path. + if (preferredPath.length > 0) { + const preferredLaunchTarget = launchTargets.find((a_1) => a_1.target === preferredPath); + if (preferredLaunchTarget) { + return this.restart(preferredLaunchTarget); } + } - // When running integration tests, open the first launch target. - if (process.env.RUNNING_INTEGRATION_TESTS === "true") { - return this.restart(launchTargets[0]); - } + // To maintain previous behavior when there are mulitple targets available, + // launch with first Solution or Folder target. + const firstFolderOrSolutionTarget = launchTargets + .find(target => target.workspaceKind == LaunchTargetKind.Folder || target.workspaceKind == LaunchTargetKind.Solution); + if (firstFolderOrSolutionTarget) { + return this.restart(firstFolderOrSolutionTarget); + } - // Otherwise, we fire the "MultipleLaunchTargets" event, - // which is handled in status.ts to display the launch target selector. - this._fireEvent(Events.MultipleLaunchTargets, launchTargets); - return showProjectSelector(this, launchTargets); - }); + // When running integration tests, open the first launch target. + if (process.env.RUNNING_INTEGRATION_TESTS === "true") { + return this.restart(launchTargets[0]); + } + + // Otherwise, we fire the "MultipleLaunchTargets" event, + // which is handled in status.ts to display the launch target selector. + this._fireEvent(Events.MultipleLaunchTargets, launchTargets); + return await showProjectSelector(this, launchTargets); } // --- requests et al From a4d5845d6040aa298338ad2e1d3366ce678954d3 Mon Sep 17 00:00:00 2001 From: Winston Liu Date: Thu, 28 Apr 2022 22:51:23 -0700 Subject: [PATCH 02/11] Handle request.id being undefined --- src/observers/OmnisharpDebugModeLoggerObserver.ts | 4 ++-- src/omnisharp/loggingEvents.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/observers/OmnisharpDebugModeLoggerObserver.ts b/src/observers/OmnisharpDebugModeLoggerObserver.ts index 67dbc350e..6bea8fca2 100644 --- a/src/observers/OmnisharpDebugModeLoggerObserver.ts +++ b/src/observers/OmnisharpDebugModeLoggerObserver.ts @@ -57,7 +57,7 @@ export class OmnisharpDebugModeLoggerObserver extends BaseLoggerObserver { } private handleOmnisharpServerRequestCancelled(event: OmnisharpServerRequestCancelled) { - this.logger.appendLine(`Cancelled request for ${event.command} (${event.id}).`); + this.logger.appendLine(`Cancelled request for ${event.command}${event.id ? ` (${event.id})` : ''}.`); this.logger.appendLine(); } @@ -101,4 +101,4 @@ export class OmnisharpDebugModeLoggerObserver extends BaseLoggerObserver { default: throw new Error(`Unknown log level value: ${logLevel}`); } } -} \ No newline at end of file +} diff --git a/src/omnisharp/loggingEvents.ts b/src/omnisharp/loggingEvents.ts index dc463ec63..89d843831 100644 --- a/src/omnisharp/loggingEvents.ts +++ b/src/omnisharp/loggingEvents.ts @@ -125,7 +125,7 @@ export class OmnisharpServerDequeueRequest implements BaseEvent { export class OmnisharpServerRequestCancelled implements BaseEvent { type = EventType.OmnisharpServerRequestCancelled; - constructor(public command: string, public id: number) { } + constructor(public command: string, public id: number | undefined) { } } export class OmnisharpServerProcessRequestStart implements BaseEvent { From 718152a3e6c99797d36d51c1b9153036ea87b22f Mon Sep 17 00:00:00 2001 From: Winston Liu Date: Thu, 28 Apr 2022 22:54:34 -0700 Subject: [PATCH 03/11] Localize readLine --- src/omnisharp/server.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/omnisharp/server.ts b/src/omnisharp/server.ts index 2ba7d6537..a4197a2ae 100644 --- a/src/omnisharp/server.ts +++ b/src/omnisharp/server.ts @@ -12,7 +12,7 @@ import * as serverUtils from '../omnisharp/utils'; import { vscode, CancellationToken } from '../vscodeAdapter'; import { ChildProcess, exec } from 'child_process'; import { LaunchTarget, findLaunchTargets, LaunchTargetKind } from './launcher'; -import { ReadLine, createInterface } from 'readline'; +import { createInterface } from 'readline'; import { Request, RequestQueueCollection } from './requestQueue'; import { DelayTracker } from './delayTracker'; import { EventEmitter } from 'events'; @@ -83,7 +83,6 @@ const latestVersionFileServerPath = 'releases/versioninfo.txt'; export class OmniSharpServer { private static _nextId = 1; - private _readLine: ReadLine; private _disposables: CompositeDisposable; private _delayTrackers!: { [requestName: string]: DelayTracker }; // Initialized via _start @@ -660,7 +659,7 @@ export class OmniSharpServer { } }); - this._readLine = createInterface({ + const readLine = createInterface({ input: this._serverProcess.stdout, output: this._serverProcess.stdin, terminal: false @@ -694,10 +693,10 @@ export class OmniSharpServer { const lineReceived = this._onLineReceived.bind(this); - this._readLine.addListener('line', lineReceived); + readLine.addListener('line', lineReceived); this._disposables.add(new Disposable(() => { - this._readLine.removeListener('line', lineReceived); + readLine.removeListener('line', lineReceived); })); return promise; From 15eab84f07b0495c5b120f157089b5bc049c15a7 Mon Sep 17 00:00:00 2001 From: Winston Liu Date: Thu, 28 Apr 2022 23:07:54 -0700 Subject: [PATCH 04/11] Introduce State...state machine Encapsulates variables that only have meaning during certain states. --- src/omnisharp/server.ts | 110 ++++++++++++++++++++++++---------------- 1 file changed, 65 insertions(+), 45 deletions(-) diff --git a/src/omnisharp/server.ts b/src/omnisharp/server.ts index a4197a2ae..f3be08846 100644 --- a/src/omnisharp/server.ts +++ b/src/omnisharp/server.ts @@ -40,6 +40,15 @@ enum ServerState { Stopped } +type State = { + status: ServerState.Stopped, +} | { + status: ServerState.Starting, +} | { + status: ServerState.Started, + serverProcess: ChildProcess, +}; + module Events { export const StateChanged = 'stateChanged'; @@ -89,10 +98,9 @@ export class OmniSharpServer { private _telemetryIntervalId: NodeJS.Timer | undefined; private _eventBus = new EventEmitter(); - private _state: ServerState = ServerState.Stopped; + private _state: State = { status: ServerState.Stopped }; private _launchTarget: LaunchTarget | undefined; private _requestQueue: RequestQueueCollection; - private _serverProcess: ChildProcess; private _sessionProperties: { [key: string]: any } = {}; private _omnisharpManager: OmnisharpManager; @@ -122,7 +130,7 @@ export class OmniSharpServer { } public isRunning(): boolean { - return this._state === ServerState.Started; + return this._state.status === ServerState.Started; } public async waitForEmptyEventQueue(): Promise { @@ -132,10 +140,10 @@ export class OmniSharpServer { } } - private _setState(value: ServerState): void { - if (typeof value !== 'undefined' && value !== this._state) { - this._state = value; - this._fireEvent(Events.StateChanged, this._state); + private _setState(state: State): void { + if (state.status !== this._state.status) { + this._state = state; + this._fireEvent(Events.StateChanged, this._state.status); } } @@ -258,7 +266,7 @@ export class OmniSharpServer { // --- start, stop, and connect private async _start(launchTarget: LaunchTarget, options: Options): Promise { - if (this._state != ServerState.Stopped) { + if (this._state.status != ServerState.Stopped) { this.eventStream.post(new ObservableEvents.OmnisharpServerOnServerError("Attempt to start OmniSharp server failed because another server instance is running.")); return; } @@ -324,7 +332,7 @@ export class OmniSharpServer { this._disposables = disposables; - this._setState(ServerState.Starting); + this._setState({ status: ServerState.Starting }); this._launchTarget = launchTarget; const solutionPath = launchTarget.target; @@ -434,11 +442,13 @@ export class OmniSharpServer { } } - this._serverProcess = launchResult.process; this._delayTrackers = {}; - await this._doConnect(options); - this._setState(ServerState.Started); + await this._doConnect(launchResult.process, options); + this._setState({ + status: ServerState.Started, + serverProcess: launchResult.process, + }); this._fireEvent(Events.ServerStart, solutionPath); this._telemetryIntervalId = setInterval(() => this._reportTelemetry(), TelemetryReportingDelay); @@ -487,43 +497,49 @@ export class OmniSharpServer { this._reportTelemetry(); } - if (!this._serverProcess) { + if (this._state.status !== ServerState.Started) { // nothing to kill cleanupPromise = Promise.resolve(); } - else if (process.platform === 'win32') { - // when killing a process in windows its child - // processes are *not* killed but become root - // processes. Therefore we use TASKKILL.EXE - cleanupPromise = new Promise((resolve, reject) => { - const killer = exec(`taskkill /F /T /PID ${this._serverProcess.pid}`, (err, stdout, stderr) => { - if (err) { - return reject(err); - } - }); - - killer.on('exit', resolve); - killer.on('error', reject); - }); - } else { - // Kill Unix process and children - cleanupPromise = utils.getUnixChildProcessIds(this._serverProcess.pid) - .then(children => { - for (let child of children) { - process.kill(child, 'SIGTERM'); - } - - this._serverProcess.kill('SIGTERM'); + // Cache serverProcess to pass to the promises, or else TypeScript will complain. + // While we know that there's no way the state can change before cleanupPromise + // is executed (as we await it below), TypeScript is unable to infer that. + const { serverProcess } = this._state; + + if (process.platform === 'win32') { + // when killing a process in windows its child + // processes are *not* killed but become root + // processes. Therefore we use TASKKILL.EXE + cleanupPromise = new Promise((resolve, reject) => { + const killer = exec(`taskkill /F /T /PID ${serverProcess.pid}`, (err, stdout, stderr) => { + if (err) { + return reject(err); + } + }); + + killer.on('exit', resolve); + killer.on('error', reject); }); + } + else { + // Kill Unix process and children + cleanupPromise = utils.getUnixChildProcessIds(serverProcess.pid) + .then(children => { + for (let child of children) { + process.kill(child, 'SIGTERM'); + } + + serverProcess.kill('SIGTERM'); + }); + } } let disposables = this._disposables; this._disposables = null; return cleanupPromise.then(() => { - this._serverProcess = null; - this._setState(ServerState.Stopped); + this._setState({ status: ServerState.Stopped }); this._fireEvent(Events.ServerStop, this); if (disposables) { disposables.dispose(); @@ -532,7 +548,7 @@ export class OmniSharpServer { } public async restart(launchTarget: LaunchTarget | undefined = this._launchTarget): Promise { - if (this._state == ServerState.Starting) { + if (this._state.status == ServerState.Starting) { this.eventStream.post(new ObservableEvents.OmnisharpServerOnServerError("Attempt to restart OmniSharp server failed because another server instance is starting.")); return; } @@ -650,9 +666,9 @@ export class OmniSharpServer { }); } - private async _doConnect(options: Options): Promise { + private async _doConnect(serverProcess: ChildProcess, options: Options): Promise { - this._serverProcess.stderr.on('data', (data: Buffer) => { + serverProcess.stderr.on('data', (data: Buffer) => { let trimData = removeBOMFromBuffer(data); if (trimData.length > 0) { this._fireEvent(Events.StdErr, trimData.toString()); @@ -660,8 +676,8 @@ export class OmniSharpServer { }); const readLine = createInterface({ - input: this._serverProcess.stdout, - output: this._serverProcess.stdin, + input: serverProcess.stdout, + output: serverProcess.stdin, terminal: false }); @@ -768,7 +784,11 @@ export class OmniSharpServer { } } - private _makeRequest(request: Request) { + private _makeRequest(request: Request): number { + if (this._state.status !== ServerState.Started) { + throw new Error("Tried to make a request when the OmniSharp server wasn't running"); + } + const id = OmniSharpServer._nextId++; request.id = id; @@ -780,7 +800,7 @@ export class OmniSharpServer { }; this.eventStream.post(new ObservableEvents.OmnisharpRequestMessage(request, id)); - this._serverProcess.stdin.write(JSON.stringify(requestPacket) + '\n'); + this._state.serverProcess.stdin.write(JSON.stringify(requestPacket) + '\n'); return id; } } From afaae2f2d5b76c201d938932d3d87a7bbf409bd4 Mon Sep 17 00:00:00 2001 From: Winston Liu Date: Thu, 28 Apr 2022 23:13:42 -0700 Subject: [PATCH 05/11] Move disposables to State --- src/omnisharp/server.ts | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/omnisharp/server.ts b/src/omnisharp/server.ts index f3be08846..c23529c2f 100644 --- a/src/omnisharp/server.ts +++ b/src/omnisharp/server.ts @@ -44,8 +44,10 @@ type State = { status: ServerState.Stopped, } | { status: ServerState.Starting, + disposables: CompositeDisposable, } | { status: ServerState.Started, + disposables: CompositeDisposable, serverProcess: ChildProcess, }; @@ -92,7 +94,6 @@ const latestVersionFileServerPath = 'releases/versioninfo.txt'; export class OmniSharpServer { private static _nextId = 1; - private _disposables: CompositeDisposable; private _delayTrackers!: { [requestName: string]: DelayTracker }; // Initialized via _start private _telemetryIntervalId: NodeJS.Timer | undefined; @@ -276,7 +277,7 @@ export class OmniSharpServer { return; } - let disposables = new CompositeDisposable(); + const disposables = new CompositeDisposable(); disposables.add(this.onServerError(err => this.eventStream.post(new ObservableEvents.OmnisharpServerOnServerError(err)) @@ -330,9 +331,10 @@ export class OmniSharpServer { disposables.add(this.onProjectChange(this.debounceUpdateProjectWithLeadingTrue)); disposables.add(this.onProjectRemoved(this.debounceUpdateProjectWithLeadingTrue)); - this._disposables = disposables; - - this._setState({ status: ServerState.Starting }); + this._setState({ + status: ServerState.Starting, + disposables, + }); this._launchTarget = launchTarget; const solutionPath = launchTarget.target; @@ -444,9 +446,10 @@ export class OmniSharpServer { this._delayTrackers = {}; - await this._doConnect(launchResult.process, options); + await this._doConnect(disposables, launchResult.process, options); this._setState({ status: ServerState.Started, + disposables, serverProcess: launchResult.process, }); this._fireEvent(Events.ServerStart, solutionPath); @@ -505,7 +508,8 @@ export class OmniSharpServer { // Cache serverProcess to pass to the promises, or else TypeScript will complain. // While we know that there's no way the state can change before cleanupPromise // is executed (as we await it below), TypeScript is unable to infer that. - const { serverProcess } = this._state; + const { disposables, serverProcess } = this._state; + disposables.dispose(); if (process.platform === 'win32') { // when killing a process in windows its child @@ -535,16 +539,10 @@ export class OmniSharpServer { } } - let disposables = this._disposables; - this._disposables = null; + await cleanupPromise; - return cleanupPromise.then(() => { - this._setState({ status: ServerState.Stopped }); - this._fireEvent(Events.ServerStop, this); - if (disposables) { - disposables.dispose(); - } - }); + this._setState({ status: ServerState.Stopped }); + this._fireEvent(Events.ServerStop, this); } public async restart(launchTarget: LaunchTarget | undefined = this._launchTarget): Promise { @@ -666,7 +664,10 @@ export class OmniSharpServer { }); } - private async _doConnect(serverProcess: ChildProcess, options: Options): Promise { + private async _doConnect( + disposables: CompositeDisposable, + serverProcess: ChildProcess, + options: Options): Promise { serverProcess.stderr.on('data', (data: Buffer) => { let trimData = removeBOMFromBuffer(data); @@ -711,7 +712,7 @@ export class OmniSharpServer { readLine.addListener('line', lineReceived); - this._disposables.add(new Disposable(() => { + disposables.add(new Disposable(() => { readLine.removeListener('line', lineReceived); })); From a96381b3e6dafebde69d6b184af565b67829716a Mon Sep 17 00:00:00 2001 From: Winston Liu Date: Thu, 28 Apr 2022 23:16:05 -0700 Subject: [PATCH 06/11] Move telemetryIntervalId to state --- src/omnisharp/server.ts | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/omnisharp/server.ts b/src/omnisharp/server.ts index c23529c2f..d47049e7e 100644 --- a/src/omnisharp/server.ts +++ b/src/omnisharp/server.ts @@ -49,6 +49,7 @@ type State = { status: ServerState.Started, disposables: CompositeDisposable, serverProcess: ChildProcess, + telemetryIntervalId: NodeJS.Timeout, }; module Events { @@ -96,7 +97,6 @@ export class OmniSharpServer { private static _nextId = 1; private _delayTrackers!: { [requestName: string]: DelayTracker }; // Initialized via _start - private _telemetryIntervalId: NodeJS.Timer | undefined; private _eventBus = new EventEmitter(); private _state: State = { status: ServerState.Stopped }; @@ -451,10 +451,10 @@ export class OmniSharpServer { status: ServerState.Started, disposables, serverProcess: launchResult.process, + telemetryIntervalId: setInterval(() => this._reportTelemetry(), TelemetryReportingDelay), }); this._fireEvent(Events.ServerStart, solutionPath); - this._telemetryIntervalId = setInterval(() => this._reportTelemetry(), TelemetryReportingDelay); this._requestQueue.drain(); } catch (err) { @@ -493,13 +493,6 @@ export class OmniSharpServer { // Clear the session properties when the session ends. this._sessionProperties = {}; - if (this._telemetryIntervalId !== undefined) { - // Stop reporting telemetry - clearInterval(this._telemetryIntervalId); - this._telemetryIntervalId = undefined; - this._reportTelemetry(); - } - if (this._state.status !== ServerState.Started) { // nothing to kill cleanupPromise = Promise.resolve(); @@ -508,9 +501,12 @@ export class OmniSharpServer { // Cache serverProcess to pass to the promises, or else TypeScript will complain. // While we know that there's no way the state can change before cleanupPromise // is executed (as we await it below), TypeScript is unable to infer that. - const { disposables, serverProcess } = this._state; + const { disposables, serverProcess, telemetryIntervalId } = this._state; disposables.dispose(); + clearInterval(telemetryIntervalId); + this._reportTelemetry(); + if (process.platform === 'win32') { // when killing a process in windows its child // processes are *not* killed but become root From 099690fa1d8e7358871f91e8d11003de1ff647b1 Mon Sep 17 00:00:00 2001 From: Winston Liu Date: Thu, 28 Apr 2022 23:19:17 -0700 Subject: [PATCH 07/11] Cleanup --- src/omnisharp/server.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/omnisharp/server.ts b/src/omnisharp/server.ts index d47049e7e..f4ad26bc2 100644 --- a/src/omnisharp/server.ts +++ b/src/omnisharp/server.ts @@ -542,7 +542,7 @@ export class OmniSharpServer { } public async restart(launchTarget: LaunchTarget | undefined = this._launchTarget): Promise { - if (this._state.status == ServerState.Starting) { + if (this._state.status === ServerState.Starting) { this.eventStream.post(new ObservableEvents.OmnisharpServerOnServerError("Attempt to restart OmniSharp server failed because another server instance is starting.")); return; } @@ -615,13 +615,12 @@ export class OmniSharpServer { // Otherwise, we fire the "MultipleLaunchTargets" event, // which is handled in status.ts to display the launch target selector. this._fireEvent(Events.MultipleLaunchTargets, launchTargets); - return await showProjectSelector(this, launchTargets); + return showProjectSelector(this, launchTargets); } // --- requests et al public async makeRequest(command: string, data?: any, token?: CancellationToken): Promise { - if (!this.isRunning()) { return Promise.reject('OmniSharp server is not running.'); } @@ -629,7 +628,7 @@ export class OmniSharpServer { let startTime: number; let request: Request; - let promise = new Promise((resolve, reject) => { + const promise = new Promise((resolve, reject) => { startTime = Date.now(); request = { @@ -642,7 +641,7 @@ export class OmniSharpServer { this._requestQueue.enqueue(request); }); - if (token) { + if (token !== undefined) { token.onCancellationRequested(() => { this.eventStream.post(new ObservableEvents.OmnisharpServerRequestCancelled(request.command, request.id)); this._requestQueue.cancelRequest(request); @@ -664,7 +663,6 @@ export class OmniSharpServer { disposables: CompositeDisposable, serverProcess: ChildProcess, options: Options): Promise { - serverProcess.stderr.on('data', (data: Buffer) => { let trimData = removeBOMFromBuffer(data); if (trimData.length > 0) { From 7a194bdd02e418c91e85b709bf5d1f16da33f395 Mon Sep 17 00:00:00 2001 From: Winston Liu Date: Thu, 28 Apr 2022 23:19:44 -0700 Subject: [PATCH 08/11] Initialize _delayTracker --- src/omnisharp/server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/omnisharp/server.ts b/src/omnisharp/server.ts index f4ad26bc2..ab11e6fae 100644 --- a/src/omnisharp/server.ts +++ b/src/omnisharp/server.ts @@ -96,7 +96,7 @@ export class OmniSharpServer { private static _nextId = 1; - private _delayTrackers!: { [requestName: string]: DelayTracker }; // Initialized via _start + private _delayTrackers: { [requestName: string]: DelayTracker } = {}; private _eventBus = new EventEmitter(); private _state: State = { status: ServerState.Stopped }; From b4480e5951882f2cfb2238201894413d4466a2b2 Mon Sep 17 00:00:00 2001 From: Winston Liu Date: Thu, 28 Apr 2022 23:48:27 -0700 Subject: [PATCH 09/11] Call _start directly from autoStart --- src/features/commands.ts | 10 +++++----- src/omnisharp/server.ts | 21 +++++++++++---------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/features/commands.ts b/src/features/commands.ts index 95dc512b0..166e1c466 100644 --- a/src/features/commands.ts +++ b/src/features/commands.ts @@ -97,14 +97,14 @@ async function pickProjectAndStart(server: OmniSharpServer, optionProvider: Opti } export async function showProjectSelector(server: OmniSharpServer, targets: LaunchTarget[]): Promise { - return vscode.window.showQuickPick(targets, { + const launchTarget = await vscode.window.showQuickPick(targets, { matchOnDescription: true, placeHolder: `Select 1 of ${targets.length} projects` - }).then(async launchTarget => { - if (launchTarget) { - return server.restart(launchTarget); - } }); + + if (launchTarget !== undefined) { + return server.restart(launchTarget); + } } interface Command { diff --git a/src/omnisharp/server.ts b/src/omnisharp/server.ts index ab11e6fae..6f3b57417 100644 --- a/src/omnisharp/server.ts +++ b/src/omnisharp/server.ts @@ -266,8 +266,8 @@ export class OmniSharpServer { // --- start, stop, and connect - private async _start(launchTarget: LaunchTarget, options: Options): Promise { - if (this._state.status != ServerState.Stopped) { + private async _start(launchTarget: LaunchTarget): Promise { + if (this._state.status !== ServerState.Stopped) { this.eventStream.post(new ObservableEvents.OmnisharpServerOnServerError("Attempt to start OmniSharp server failed because another server instance is running.")); return; } @@ -340,7 +340,9 @@ export class OmniSharpServer { const solutionPath = launchTarget.target; const cwd = path.dirname(solutionPath); - let args = [ + const options = this.optionProvider.GetLatestOptions(); + + const args = [ '-z', '-s', solutionPath, '--hostPID', process.pid.toString(), @@ -550,8 +552,7 @@ export class OmniSharpServer { if (launchTarget !== undefined) { await this.stop(); this.eventStream.post(new ObservableEvents.OmnisharpRestart()); - const options = this.optionProvider.GetLatestOptions(); - await this._start(launchTarget, options); + await this._start(launchTarget); } } @@ -580,14 +581,14 @@ export class OmniSharpServer { } else if (launchTargets.length === 1) { // If there's only one target, just start - return this.restart(launchTargets[0]); + return this._start(launchTargets[0]); } // First, try to launch against something that matches the user's preferred target const defaultLaunchSolutionConfigValue = this.optionProvider.GetLatestOptions().defaultLaunchSolution; const defaultLaunchSolutionTarget = launchTargets.find((a) => (path.basename(a.target) === defaultLaunchSolutionConfigValue)); if (defaultLaunchSolutionTarget) { - return this.restart(defaultLaunchSolutionTarget); + return this._start(defaultLaunchSolutionTarget); } // If there's more than one launch target, we start the server if one of the targets @@ -595,7 +596,7 @@ export class OmniSharpServer { if (preferredPath.length > 0) { const preferredLaunchTarget = launchTargets.find((a_1) => a_1.target === preferredPath); if (preferredLaunchTarget) { - return this.restart(preferredLaunchTarget); + return this._start(preferredLaunchTarget); } } @@ -604,12 +605,12 @@ export class OmniSharpServer { const firstFolderOrSolutionTarget = launchTargets .find(target => target.workspaceKind == LaunchTargetKind.Folder || target.workspaceKind == LaunchTargetKind.Solution); if (firstFolderOrSolutionTarget) { - return this.restart(firstFolderOrSolutionTarget); + return this._start(firstFolderOrSolutionTarget); } // When running integration tests, open the first launch target. if (process.env.RUNNING_INTEGRATION_TESTS === "true") { - return this.restart(launchTargets[0]); + return this._start(launchTargets[0]); } // Otherwise, we fire the "MultipleLaunchTargets" event, From d985b772e13078671f857eb1b70ba9ff0e4eb305 Mon Sep 17 00:00:00 2001 From: Winston Liu Date: Sun, 1 May 2022 23:17:22 -0700 Subject: [PATCH 10/11] Dispose after firing ServerStop event --- src/omnisharp/server.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/omnisharp/server.ts b/src/omnisharp/server.ts index 6f3b57417..b5b7aa38b 100644 --- a/src/omnisharp/server.ts +++ b/src/omnisharp/server.ts @@ -489,6 +489,10 @@ export class OmniSharpServer { } public async stop(): Promise { + // We're already stopped, nothing to do :). + if (this._state.status === ServerState.Stopped) { + return; + } let cleanupPromise: Promise; @@ -500,11 +504,7 @@ export class OmniSharpServer { cleanupPromise = Promise.resolve(); } else { - // Cache serverProcess to pass to the promises, or else TypeScript will complain. - // While we know that there's no way the state can change before cleanupPromise - // is executed (as we await it below), TypeScript is unable to infer that. - const { disposables, serverProcess, telemetryIntervalId } = this._state; - disposables.dispose(); + const { serverProcess, telemetryIntervalId } = this._state; clearInterval(telemetryIntervalId); this._reportTelemetry(); @@ -539,8 +539,13 @@ export class OmniSharpServer { await cleanupPromise; + const { disposables } = this._state; + this._setState({ status: ServerState.Stopped }); this._fireEvent(Events.ServerStop, this); + + // Dispose of the disposables only _after_ we've fired the last server event. + disposables.dispose(); } public async restart(launchTarget: LaunchTarget | undefined = this._launchTarget): Promise { From 662daee116961210a09d24ac85e2c9a6fe3e8eec Mon Sep 17 00:00:00 2001 From: Winston Liu Date: Sun, 1 May 2022 23:26:11 -0700 Subject: [PATCH 11/11] Simplify stop logic now that states are clearer --- src/omnisharp/server.ts | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/src/omnisharp/server.ts b/src/omnisharp/server.ts index b5b7aa38b..db32db004 100644 --- a/src/omnisharp/server.ts +++ b/src/omnisharp/server.ts @@ -494,16 +494,10 @@ export class OmniSharpServer { return; } - let cleanupPromise: Promise; - // Clear the session properties when the session ends. this._sessionProperties = {}; - if (this._state.status !== ServerState.Started) { - // nothing to kill - cleanupPromise = Promise.resolve(); - } - else { + if (this._state.status === ServerState.Started) { const { serverProcess, telemetryIntervalId } = this._state; clearInterval(telemetryIntervalId); @@ -513,7 +507,7 @@ export class OmniSharpServer { // when killing a process in windows its child // processes are *not* killed but become root // processes. Therefore we use TASKKILL.EXE - cleanupPromise = new Promise((resolve, reject) => { + await new Promise((resolve, reject) => { const killer = exec(`taskkill /F /T /PID ${serverProcess.pid}`, (err, stdout, stderr) => { if (err) { return reject(err); @@ -526,19 +520,15 @@ export class OmniSharpServer { } else { // Kill Unix process and children - cleanupPromise = utils.getUnixChildProcessIds(serverProcess.pid) - .then(children => { - for (let child of children) { - process.kill(child, 'SIGTERM'); - } + const children = await utils.getUnixChildProcessIds(serverProcess.pid); + for (const child of children) { + process.kill(child, 'SIGTERM'); + } - serverProcess.kill('SIGTERM'); - }); + serverProcess.kill('SIGTERM'); } } - await cleanupPromise; - const { disposables } = this._state; this._setState({ status: ServerState.Stopped });