From e72d0d03ed8aefb6dcf8e511090f79a5bcf4ac99 Mon Sep 17 00:00:00 2001
From: Chris LaFreniere <40371649+chlafreniere@users.noreply.github.com>
Date: Tue, 30 Apr 2019 14:13:04 -0700
Subject: [PATCH] Notebooks: More Jupyter Server Hardening (#5264)

* Do not rely on same starting port every time, misc

* put back to 5 seconds for process kill timeout

* extHostNotebook shutdown manager handle
---
 extensions/notebook/src/common/ports.ts           | 4 ++--
 extensions/notebook/src/jupyter/serverInstance.ts | 9 +++++----
 extensions/notebook/src/test/common/port.test.ts  | 4 ++--
 src/sql/workbench/api/node/extHostNotebook.ts     | 4 ++++
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/extensions/notebook/src/common/ports.ts b/extensions/notebook/src/common/ports.ts
index 61ad299f5e..8a519269e2 100644
--- a/extensions/notebook/src/common/ports.ts
+++ b/extensions/notebook/src/common/ports.ts
@@ -7,7 +7,7 @@
 import * as net from 'net';
 
 export class StrictPortFindOptions {
-	constructor(public startPort: number, public minPort: number, public maxport: number) {
+	constructor(public minPort: number, public maxport: number) {
 	}
 	public maxRetriesPerStartPort: number = 5;
 	public totalRetryLoops: number = 10;
@@ -21,7 +21,7 @@ export class StrictPortFindOptions {
  */
 export async function strictFindFreePort(options: StrictPortFindOptions): Promise<number> {
 	let totalRetries = options.totalRetryLoops;
-	let startPort = options.startPort;
+	let startPort = getRandomInt(options.minPort, options.maxport);
 	let port = await findFreePort(startPort, options.maxRetriesPerStartPort, options.timeout);
 	while (port === 0 && totalRetries > 0) {
 		startPort = getRandomInt(options.minPort, options.maxport);
diff --git a/extensions/notebook/src/jupyter/serverInstance.ts b/extensions/notebook/src/jupyter/serverInstance.ts
index a999519486..f40dc46690 100644
--- a/extensions/notebook/src/jupyter/serverInstance.ts
+++ b/extensions/notebook/src/jupyter/serverInstance.ts
@@ -86,8 +86,9 @@ export class ServerInstanceUtils {
 			try {
 				process.kill(childProcess.pid, 'SIGKILL');
 			} catch (error) {
-				console.log(error);
-				// All is fine.
+				if (!error || !error.code || (typeof error.code === 'string' && error.code !== 'ESRCH')) {
+					console.log(error);
+				}
 			}
 		}, 5000);
 	}
@@ -230,8 +231,8 @@ export class PerNotebookServerInstance implements IServerInstance {
 			return;
 		}
 		let notebookDirectory = this.getNotebookDirectory();
-		// Find a port in a given range. If run into trouble, got up 100 in range and search inside a larger range
-		let port = await ports.strictFindFreePort(new ports.StrictPortFindOptions(defaultPort, defaultPort + 100, defaultPort + 1000));
+		// Find a port in a given range. If run into trouble, try another port inside the given range
+		let port = await ports.strictFindFreePort(new ports.StrictPortFindOptions(defaultPort, defaultPort + 1000));
 		let token = await notebookUtils.getRandomToken();
 		this._uri = vscode.Uri.parse(`http://localhost:${port}/?token=${token}`);
 		this._port = port.toString();
diff --git a/extensions/notebook/src/test/common/port.test.ts b/extensions/notebook/src/test/common/port.test.ts
index 21927aa04c..14ab8bcdc1 100644
--- a/extensions/notebook/src/test/common/port.test.ts
+++ b/extensions/notebook/src/test/common/port.test.ts
@@ -39,7 +39,7 @@ describe('Ports', () => {
 		this.timeout(1000 * 10); // higher timeout for this test
 
 		// get an initial freeport >= 7000
-		let options = new ports.StrictPortFindOptions(7000, 7100, 7200);
+		let options = new ports.StrictPortFindOptions(7000, 7200);
 		options.timeout = 300000;
 		ports.strictFindFreePort(options).then(initialPort => {
 			assert.ok(initialPort >= 7000);
@@ -49,7 +49,7 @@ describe('Ports', () => {
 			server.listen(initialPort, undefined, undefined, () => {
 
 				// once listening, find another free port and assert that the port is different from the opened one
-				options.startPort = initialPort;
+				options.minPort = initialPort;
 				options.maxRetriesPerStartPort = 1;
 				options.totalRetryLoops = 50;
 				ports.strictFindFreePort(options).then(freePort => {
diff --git a/src/sql/workbench/api/node/extHostNotebook.ts b/src/sql/workbench/api/node/extHostNotebook.ts
index 46060038aa..ecbeb3c88c 100644
--- a/src/sql/workbench/api/node/extHostNotebook.ts
+++ b/src/sql/workbench/api/node/extHostNotebook.ts
@@ -117,6 +117,10 @@ export class ExtHostNotebook implements ExtHostNotebookShape {
 	}
 
 	$shutdownSession(managerHandle: number, sessionId: string): Thenable<void> {
+		// If manager handle has already been removed, don't try to access it again when shutting down
+		if (this._adapters.get(managerHandle) === undefined) {
+			return undefined;
+		}
 		return this._withSessionManager(managerHandle, async (sessionManager) => {
 			return sessionManager.shutdown(sessionId);
 		});