Merge pull request #807 from DustinCampbell/fix-omnisharp-launchNix

Don't immediately return promise within newly created promise

Author: DustinCampbell

src/omnisharp/launcher.ts

@@ -161,21 +161,15 @@ export interface LaunchResult {
 
 export function launchOmniSharp(details: LaunchDetails): Promise<LaunchResult> {
     return new Promise<LaunchResult>((resolve, reject) => {
-        try {
-            return 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<LaunchResult> {
 }
 
 function launchWindows(details: LaunchDetails): Promise<LaunchResult> {
-    return new Promise<LaunchResult>(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, <any>{
-            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, <any>{
+        windowsVerbatimArguments: true,
+        detached: false,
+        cwd: details.cwd
+    });
 
-        return resolve({
-            process,
-            command: details.serverPath,
-            usingMono: false
-        });
+    return Promise.resolve({
+        process,
+        command: details.serverPath,
+        usingMono: false
     });
 }
 
@@ -233,48 +224,44 @@ function launchNix(details: LaunchDetails): Promise<LaunchResult> {
 }
 
 function launchNixCoreCLR(details: LaunchDetails): Promise<LaunchResult> {
-    return new Promise<LaunchResult>(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
+    });
 
-        return resolve({
-            process,
-            command: details.serverPath,
-            usingMono: false
-        });
+    return Promise.resolve({
+        process,
+        command: details.serverPath,
+        usingMono: false
     });
 }
 
 function launchNixMono(details: LaunchDetails): Promise<LaunchResult> {
-    return new Promise<LaunchResult>((resolve, reject) => {
-        return 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
+        };
     });
 }
 
 function canLaunchMono(): Promise<void> {
     return new Promise<void>((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.'));
             }
         });
     });

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<void> {
 
-        let ret: Promise<void>;
+        let cleanupPromise: Promise<void>;
 
         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<void>((resolve, reject) => {
+            cleanupPromise = new Promise<void>((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;
         });
     }