Windows extension fixes/improvements (#4740)

* Windows extension fixes/improvements

Remove error message when activating extension on non-win platforms (instead just don't do anything). Add hooks to kill child processes on exit if they haven't ended already. Formatting fixes.

* Remove shelljs

* Remove error messages from telemetry until I can follow up on whether we're allowed to send these up. Fix some issues with the exec callback - the child_process exec function returns an ExecException object instead of the code directly like the shelljs did so check for that correctly.

extensions/admin-tool-ext-win/package.json

@@ -52,7 +52,6 @@
   },
   "dependencies": {
     "service-downloader": "github:anthonydresser/service-downloader#0.1.5",
-    "shelljs": "^0.7.5",
     "vscode-extension-telemetry": "^0.0.15",
     "vscode-nls": "^3.2.1"
   },

extensions/admin-tool-ext-win/src/main.ts

@@ -7,70 +7,75 @@
 
 import * as nls from 'vscode-nls';
 import * as path from 'path';
-import * as shelljs from 'shelljs';
 import * as azdata from 'azdata';
 import * as vscode from 'vscode';
 import { IConfig, ServerProvider } from 'service-downloader';
 import { Telemetry } from './telemetry';
 import * as utils from './utils';
+import { ChildProcess, exec, ExecException } from 'child_process';
+import { stringify } from 'querystring';
 
 const baseConfig = require('./config.json');
 const localize = nls.loadMessageBundle();
-let exePath:string;
+let exePath: string;
+let runningProcesses: Map<number, ChildProcess> = new Map<number, ChildProcess>();
 
 // Params to pass to SsmsMin.exe, only an action and server are required - the rest are optional based on the
 // action used. Exported for use in testing.
 export interface LaunchSsmsDialogParams {
-    action: string;
+	action: string;
-    server: string;
+	server: string;
-    database?: string;
+	database?: string;
-    user?: string;
+	user?: string;
-    password?: string;
+	password?: string;
-    useAad?: boolean;
+	useAad?: boolean;
-    urn?: string;
+	urn?: string;
 }
 
 export async function activate(context: vscode.ExtensionContext): Promise<void> {
-    Telemetry.sendTelemetryEvent('startup/ExtensionActivated');
+	// This is for Windows-specific support so do nothing on other platforms
+	if (process.platform === 'win32') {
+		Telemetry.sendTelemetryEvent('startup/ExtensionActivated');
 
-    // Only supported on Win32 currently, display error message if not that until extensions are able to block install
+		let config: IConfig = JSON.parse(JSON.stringify(baseConfig));
-    // based on conditions
+		config.installDirectory = path.join(context.extensionPath, config.installDirectory);
-    if(process.platform === 'win32') {
+		config.proxy = utils.getConfiguration('http').get('proxy');
-        let config: IConfig = JSON.parse(JSON.stringify(baseConfig));
+		config.strictSSL = utils.getConfiguration('http').get('proxyStrictSSL') || true;
-        config.installDirectory = path.join(context.extensionPath, config.installDirectory);
-        config.proxy = utils.getConfiguration('http').get('proxy');
-        config.strictSSL = utils.getConfiguration('http').get('proxyStrictSSL') || true;
 
-        const serverdownloader = new ServerProvider(config);
+		const serverdownloader = new ServerProvider(config);
-        const installationStart = Date.now();
+		const installationStart = Date.now();
 
-        try {
+		try {
-            let downloadedExePath = await serverdownloader.getOrDownloadServer();
+			let downloadedExePath = await serverdownloader.getOrDownloadServer();
-            const installationComplete = Date.now();
+			const installationComplete = Date.now();
 
-            // Don't register the command if we couldn't find the EXE since it won't be able to do anything
+			// Don't register the command if we couldn't find the EXE since it won't be able to do anything
-            if(downloadedExePath) {
+			if (downloadedExePath) {
-                exePath = downloadedExePath;
+				exePath = downloadedExePath;
-            } else {
+			} else {
-                throw new Error('Could not find SsmsMin.exe after downloading');
+				throw new Error('Could not find SsmsMin.exe after downloading');
-            }
+			}
-            // Add the command now that we have the exePath to run the tool with
+			// Add the command now that we have the exePath to run the tool with
-            context.subscriptions.push(
+			context.subscriptions.push(
-                vscode.commands.registerCommand('adminToolExtWin.launchSsmsServerPropertiesDialog', handleLaunchSsmsServerPropertiesDialogCommand));
+				vscode.commands.registerCommand('adminToolExtWin.launchSsmsServerPropertiesDialog', handleLaunchSsmsServerPropertiesDialogCommand));
 
-            Telemetry.sendTelemetryEvent('startup/ExtensionStarted', {
+			Telemetry.sendTelemetryEvent('startup/ExtensionStarted', {
-                installationTime: String(installationComplete - installationStart),
+				installationTime: String(installationComplete - installationStart),
-                beginningTimestamp: String(installationStart)
+				beginningTimestamp: String(installationStart)
-            });
+			});
-        }
+		}
-        catch(err) {
+		catch (err) {
-            Telemetry.sendTelemetryEvent('startup/ExtensionInitializationFailed', {
+			Telemetry.sendTelemetryEvent('startup/ExtensionInitializationFailed');
-                error: err
+			console.error(`Error Initializing Admin Tool Extension for Windows - ${err}`);
-            });
+		}
-        }
+	}
-    } else {
+}
-        vscode.window.showErrorMessage(localize('adminToolExtWin.onlySupportedOnWindows', 'The Admin Tool Extension is only supported on Windows platforms.'));
+
-    }
+export async function deactivate(): Promise<void> {
+	// If the extension is being deactivated we want to kill all processes that are still currently
+	// running otherwise they will continue to run as orphan processes. We use taskkill here in case
+	// they started off child processes of their own
+	runningProcesses.forEach(p => exec('taskkill /pid ' + p.pid + ' /T /F'));
 }
 
 /**
@@ -78,11 +83,11 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
  * @param connectionId The connection context from the command
  */
 function handleLaunchSsmsServerPropertiesDialogCommand(connectionContext?: azdata.ObjectExplorerContext) {
-    if(connectionContext && connectionContext.connectionProfile) {
+	if (connectionContext && connectionContext.connectionProfile) {
-        launchSsmsDialog(
+		launchSsmsDialog(
-            /*action*/'sqla:Properties@Microsoft.SqlServer.Management.Smo.Server',
+			/*action*/'sqla:Properties@Microsoft.SqlServer.Management.Smo.Server',
-            /*connectionProfile*/connectionContext.connectionProfile);
+			/*connectionProfile*/connectionContext.connectionProfile);
-    }
+	}
 }
 
 /**
@@ -91,41 +96,49 @@ function handleLaunchSsmsServerPropertiesDialogCommand(connectionContext?: azdat
  * @param params The params used to construct the command
  * @param urn The URN to pass to SsmsMin
  */
-function launchSsmsDialog(action:string, connectionProfile: azdata.IConnectionProfile, urn?:string) {
+function launchSsmsDialog(action: string, connectionProfile: azdata.IConnectionProfile, urn?: string) {
-    if(!exePath) {
+	if (!exePath) {
-        vscode.window.showErrorMessage(localize('adminToolExtWin.noExeError', 'Unable to find SsmsMin.exe.'));
+		vscode.window.showErrorMessage(localize('adminToolExtWin.noExeError', 'Unable to find SsmsMin.exe.'));
-        return;
+		return;
-    }
+	}
 
-    Telemetry.sendTelemetryEvent('LaunchSsmsDialog', { 'action': action});
+	Telemetry.sendTelemetryEvent('LaunchSsmsDialog', { 'action': action });
 
-    let params:LaunchSsmsDialogParams = {
+	let params: LaunchSsmsDialogParams = {
-        action:action,
+		action: action,
-        server:connectionProfile.serverName,
+		server: connectionProfile.serverName,
-        database:connectionProfile.databaseName,
+		database: connectionProfile.databaseName,
-        password:connectionProfile.password,
+		password: connectionProfile.password,
-        user:connectionProfile.userName,
+		user: connectionProfile.userName,
-        useAad:connectionProfile.authenticationType === 'AzureMFA',
+		useAad: connectionProfile.authenticationType === 'AzureMFA',
-        urn: urn};
+		urn: urn
-    let args = buildSsmsMinCommandArgs(params);
+	};
+	let args = buildSsmsMinCommandArgs(params);
 
-    // This will be an async call since we pass in the callback
+	// This will be an async call since we pass in the callback
-    var proc = shelljs.exec(
+	var proc: ChildProcess = exec(
-       /*command*/`"${exePath}" ${args}`,
+	   /*command*/`"${exePath}" ${args}`,
-       /*options*/'',
+	   /*options*/undefined,
-       (code, stdout, stderr) => {
+		(execException, stdout, stderr) => {
-           Telemetry.sendTelemetryEvent('LaunchSsmsDialogResult', {
+			// Process has exited so remove from map of running processes
-               'action': params.action,
+			runningProcesses.delete(proc.pid);
-               'returnCode': code,
+			Telemetry.sendTelemetryEvent('LaunchSsmsDialogResult', {
-               'error': stderr
+				'action': params.action,
-           });
+				'returnCode': execException && execException.code ? execException.code.toString() : '0'
+			});
+			let err = stderr.toString();
+			if (err !== '') {
+				console.warn(`Error calling SsmsMin with args '${args}' - ${err}`);
+			}
+		});
 
-            // If we're not using AAD the tool prompts for a password on stdin
+	// If we're not using AAD the tool prompts for a password on stdin
-            if(params.useAad !== true) {
+	if (params.useAad !== true) {
-                proc.stdin.end(params.password ? params.password : '');
+		proc.stdin.end(params.password ? params.password : '');
-            }
+	}
-       });
 
+	// Save the process into our map so we can make sure to stop them if we exit before shutting down
+	runningProcesses.set(proc.pid, proc);
 }
 
 /**
@@ -134,11 +147,11 @@ function launchSsmsDialog(action:string, connectionProfile: azdata.IConnectionPr
  * escaping will occur.
  * @param params The params used to build up the command parameter string
  */
-export function buildSsmsMinCommandArgs(params:LaunchSsmsDialogParams): string {
+export function buildSsmsMinCommandArgs(params: LaunchSsmsDialogParams): string {
-    return `${params.action ? '-a "' + params.action.replace(/"/g, '\\"') + '"' : ''}\
+	return `${params.action ? '-a "' + params.action.replace(/"/g, '\\"') + '"' : ''}\
 ${params.server ? ' -S "' + params.server.replace(/"/g, '\\"') + '"' : ''}\
 ${params.database ? ' -D "' + params.database.replace(/"/g, '\\"') + '"' : ''}\
 ${params.useAad !== true && params.user ? ' -U "' + params.user.replace(/"/g, '\\"') + '"' : ''}\
-${params.useAad === true ? ' -G': ''}\
+${params.useAad === true ? ' -G' : ''}\
 ${params.urn ? ' -u "' + params.urn.replace(/"/g, '\\"') + '"' : ''}`;
 }

extensions/admin-tool-ext-win/yarn.lock

@@ -832,7 +832,7 @@ glob@^5.0.15, glob@^5.0.3:
     once "^1.3.0"
     path-is-absolute "^1.0.0"
 
-glob@^7.0.0, glob@^7.1.3:
+glob@^7.1.3:
   version "7.1.3"
   resolved "https://registry.yarnpkg.com/glob/-/glob-7.1.3.tgz#3960832d3f1574108342dafd3a67b332c0969df1"
   integrity sha512-vcfuiIxogLV4DlGBHIUOwI0IbrJ8HWPc4MU7HzviGeNho/UJDfi6B5p3sHeWIQ0KGIU0Jpxi5ZHxemQfLkkAwQ==
@@ -1096,11 +1096,6 @@ inherits@2, inherits@^2.0.1, inherits@~2.0.0, inherits@~2.0.1, inherits@~2.0.3:
   resolved "https://registry.yarnpkg.com/inherits/-/inherits-2.0.3.tgz#633c2c83e3da42a502f52466022480f4208261de"
   integrity sha1-Yzwsg+PaQqUC9SRmAiSA9CCCYd4=
 
-interpret@^1.0.0:
-  version "1.2.0"
-  resolved "https://registry.yarnpkg.com/interpret/-/interpret-1.2.0.tgz#d5061a6224be58e8083985f5014d844359576296"
-  integrity sha512-mT34yGKMNceBQUoVn7iCDKDntA7SC6gycMAWzGx1z/CMCTV7b2AAtXlo3nRyHZ1FelRkQbQjprHSYGwzLtkVbw==
-
 is-arrayish@^0.2.1:
   version "0.2.1"
   resolved "https://registry.yarnpkg.com/is-arrayish/-/is-arrayish-0.2.1.tgz#77c99840527aa8ecb1a8ba697b80645a7a926a9d"
@@ -1978,13 +1973,6 @@ readable-stream@~1.1.9:
     isarray "0.0.1"
     string_decoder "~0.10.x"
 
-rechoir@^0.6.2:
-  version "0.6.2"
-  resolved "https://registry.yarnpkg.com/rechoir/-/rechoir-0.6.2.tgz#85204b54dba82d5742e28c96756ef43af50e3384"
-  integrity sha1-hSBLVNuoLVdC4oyWdW70OvUOM4Q=
-  dependencies:
-    resolve "^1.1.6"
-
 redent@^1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/redent/-/redent-1.0.0.tgz#cf916ab1fd5f1f16dfb20822dd6ec7f730c2afde"
@@ -2058,7 +2046,7 @@ request@^2.67.0, request@^2.88.0:
     tunnel-agent "^0.6.0"
     uuid "^3.3.2"
 
-resolve@^1.1.6, resolve@^1.10.0:
+resolve@^1.10.0:
   version "1.10.0"
   resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.10.0.tgz#3bdaaeaf45cc07f375656dfd2e54ed0810b101ba"
   integrity sha512-3sUr9aq5OfSg2S9pNtPA9hL1FVEAjvfOC4leW0SNf/mpnaakz2a9femSd6LqAww2RaFctwyf1lCqnTHuF1rxDg==
@@ -2105,15 +2093,6 @@ seek-bzip@^1.0.5:
     mkdirp "^0.5.1"
     tmp "^0.0.33"
 
-shelljs@^0.7.5:
-  version "0.7.8"
-  resolved "https://registry.yarnpkg.com/shelljs/-/shelljs-0.7.8.tgz#decbcf874b0d1e5fb72e14b164a9683048e9acb3"
-  integrity sha1-3svPh0sNHl+3LhSxZKloMEjprLM=
-  dependencies:
-    glob "^7.0.0"
-    interpret "^1.0.0"
-    rechoir "^0.6.2"
-
 sigmund@~1.0.0:
   version "1.0.1"
   resolved "https://registry.yarnpkg.com/sigmund/-/sigmund-1.0.1.tgz#3ff21f198cad2175f9f3b781853fd94d0d19b590"