From 875e3a6b89dd96dbb440896bf9c7829ff6586320 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Mon, 10 Oct 2016 14:59:38 -0700 Subject: [PATCH 1/2] Don't immediately return promise within newly created promise --- src/omnisharp/launcher.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/omnisharp/launcher.ts b/src/omnisharp/launcher.ts index d2cf0629e2..43c15bd96c 100644 --- a/src/omnisharp/launcher.ts +++ b/src/omnisharp/launcher.ts @@ -162,7 +162,7 @@ export interface LaunchResult { export function launchOmniSharp(details: LaunchDetails): Promise { return new Promise((resolve, reject) => { try { - return launch(details).then(result => { + launch(details).then(result => { // async error - when target not not ENEOT result.process.on('error', reject); @@ -212,7 +212,7 @@ function launchWindows(details: LaunchDetails): Promise { cwd: details.cwd }); - return resolve({ + resolve({ process, command: details.serverPath, usingMono: false @@ -239,7 +239,7 @@ function launchNixCoreCLR(details: LaunchDetails): Promise { cwd: details.cwd }); - return resolve({ + resolve({ process, command: details.serverPath, usingMono: false @@ -249,7 +249,7 @@ function launchNixCoreCLR(details: LaunchDetails): Promise { function launchNixMono(details: LaunchDetails): Promise { return new Promise((resolve, reject) => { - return canLaunchMono().then(() => { + canLaunchMono().then(() => { let args = details.args.slice(0); // create copy of details.args args.unshift(details.serverPath); @@ -271,10 +271,10 @@ function canLaunchMono(): Promise { return new Promise((resolve, reject) => { hasMono('>=4.0.1').then(success => { if (success) { - return resolve(); + resolve(); } else { - return reject(new Error('Cannot start Omnisharp because Mono version >=4.0.1 is required.')); + reject(new Error('Cannot start Omnisharp because Mono version >=4.0.1 is required.')); } }); }); From a880c20ee4749a67e807f73881ec3cc12c54e62c Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Tue, 11 Oct 2016 09:48:50 -0700 Subject: [PATCH 2/2] Further clean up of OmniSharp launcher promises --- src/omnisharp/launcher.ts | 117 +++++++++++++++++--------------------- src/omnisharp/server.ts | 16 +++--- 2 files changed, 59 insertions(+), 74 deletions(-) diff --git a/src/omnisharp/launcher.ts b/src/omnisharp/launcher.ts index 43c15bd96c..672ec6bcd5 100644 --- a/src/omnisharp/launcher.ts +++ b/src/omnisharp/launcher.ts @@ -161,21 +161,15 @@ export interface LaunchResult { export function launchOmniSharp(details: LaunchDetails): Promise { return new Promise((resolve, reject) => { - try { - launch(details).then(result => { - // async error - when target not not ENEOT - result.process.on('error', reject); - - // success after a short freeing event loop - setTimeout(function () { - resolve(result); - }, 0); - }, err => { - reject(err); - }); - } catch (err) { - reject(err); - } + launch(details).then(result => { + // async error - when target not not ENEOT + result.process.on('error', reject); + + // success after a short freeing event loop + setTimeout(function () { + resolve(result); + }, 0); + }); }); } @@ -189,34 +183,31 @@ function launch(details: LaunchDetails): Promise { } function launchWindows(details: LaunchDetails): Promise { - return new Promise(resolve => { - - function escapeIfNeeded(arg: string) { - const hasSpaceWithoutQuotes = /^[^"].* .*[^"]/; - return hasSpaceWithoutQuotes.test(arg) - ? `"${arg}"` - : arg; - } + function escapeIfNeeded(arg: string) { + const hasSpaceWithoutQuotes = /^[^"].* .*[^"]/; + return hasSpaceWithoutQuotes.test(arg) + ? `"${arg}"` + : arg; + } - let args = details.args.slice(0); // create copy of details.args - args.unshift(details.serverPath); - args = [[ - '/s', - '/c', - '"' + args.map(escapeIfNeeded).join(' ') + '"' - ].join(' ')]; - - let process = spawn('cmd', args, { - windowsVerbatimArguments: true, - detached: false, - cwd: details.cwd - }); + let args = details.args.slice(0); // create copy of details.args + args.unshift(details.serverPath); + args = [[ + '/s', + '/c', + '"' + args.map(escapeIfNeeded).join(' ') + '"' + ].join(' ')]; + + let process = spawn('cmd', args, { + windowsVerbatimArguments: true, + detached: false, + cwd: details.cwd + }); - resolve({ - process, - command: details.serverPath, - usingMono: false - }); + return Promise.resolve({ + process, + command: details.serverPath, + usingMono: false }); } @@ -233,37 +224,33 @@ function launchNix(details: LaunchDetails): Promise { } function launchNixCoreCLR(details: LaunchDetails): Promise { - return new Promise(resolve => { - let process = spawn(details.serverPath, details.args, { - detached: false, - cwd: details.cwd - }); + let process = spawn(details.serverPath, details.args, { + detached: false, + cwd: details.cwd + }); - resolve({ - process, - command: details.serverPath, - usingMono: false - }); + return Promise.resolve({ + process, + command: details.serverPath, + usingMono: false }); } function launchNixMono(details: LaunchDetails): Promise { - return new Promise((resolve, reject) => { - canLaunchMono().then(() => { - let args = details.args.slice(0); // create copy of details.args - args.unshift(details.serverPath); - - let process = spawn('mono', args, { - detached: false, - cwd: details.cwd - }); + return canLaunchMono().then(() => { + let args = details.args.slice(0); // create copy of details.args + args.unshift(details.serverPath); - return resolve({ - process, - command: details.serverPath, - usingMono: true - }); + let process = spawn('mono', args, { + detached: false, + cwd: details.cwd }); + + return { + process, + command: details.serverPath, + usingMono: true + }; }); } diff --git a/src/omnisharp/server.ts b/src/omnisharp/server.ts index 8973a8c851..04e2c516af 100644 --- a/src/omnisharp/server.ts +++ b/src/omnisharp/server.ts @@ -299,10 +299,9 @@ export abstract class OmnisharpServer { this._telemetryIntervalId = setInterval(() => this._reportTelemetry(), TelemetryReportingDelay); }).then(() => { this._processQueue(); - }, err => { + }).catch(err => { this._fireEvent(Events.ServerError, err); - this._setState(ServerState.Stopped); - throw err; + return this.stop(); }); }); } @@ -311,7 +310,7 @@ export abstract class OmnisharpServer { public stop(): Promise { - let ret: Promise; + let cleanupPromise: Promise; if (this._telemetryIntervalId !== undefined) { // Stop reporting telemetry @@ -322,13 +321,13 @@ export abstract class OmnisharpServer { if (!this._serverProcess) { // nothing to kill - ret = Promise.resolve(); + 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 - ret = new Promise((resolve, reject) => { + cleanupPromise = new Promise((resolve, reject) => { const killer = exec(`taskkill /F /T /PID ${this._serverProcess.pid}`, (err, stdout, stderr) => { if (err) { return reject(err); @@ -342,14 +341,13 @@ export abstract class OmnisharpServer { else { // Kill Unix process this._serverProcess.kill('SIGTERM'); - ret = Promise.resolve(); + cleanupPromise = Promise.resolve(); } - return ret.then(() => { + return cleanupPromise.then(() => { this._serverProcess = null; this._setState(ServerState.Stopped); this._fireEvent(Events.ServerStop, this); - return; }); }