Potential fixes for cancel and session timeout on shutdown (microsoft#4845)

rchiodo · web-flow · commit 91737c6d2bf3 · 2019-03-20T10:46:21.000-07:00
For #4827  - [x] Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR) - [x] Title summarizes what is changing - [x] Has a [news entry](https://github.com/Microsoft/vscode-python/tree/master/news) file (remember to thank yourself!) - [ ] Has sufficient logging. - [ ] Has telemetry for enhancements. - [x] Unit tests & system/integration tests are added/updated - [ ] [Test plan](https://github.com/Microsoft/vscode-python/blob/master/.github/test_plan.md) is updated as appropriate - [ ] [`package-lock.json`](https://github.com/Microsoft/vscode-python/blob/master/package-lock.json) has been regenerated by running `npm install` (if dependencies have changed) - [ ] The wiki is updated with any design decisions/details.
diff --git a/news/3 Code Health/4827.md b/news/3 Code Health/4827.md
@@ -0,0 +1 @@
+Handle done on all jupyter requests to make sure an unhandle exception isn't passed on shutdown.
diff --git a/src/client/common/utils/async.ts b/src/client/common/utils/async.ts
@@ -84,3 +84,16 @@ export function createDeferredFromPromise<T>(promise: Promise<T>): Deferred<T> {
         .catch(deferred.reject.bind(deferred));
     return deferred;
 }
+export function callWithTimeout<T>(func: () => T, timeoutMS: number) : Promise<T> {
+    return new Promise<T>((resolve, reject) => {
+        setTimeout(() => {
+            reject(new Error('Timed out'));
+        }, timeoutMS);
+        try {
+            const result = func();
+            resolve(result);
+        } catch (e) {
+            reject(e);
+        }
+    });
+}
diff --git a/src/client/datascience/jupyter/jupyterSession.ts b/src/client/datascience/jupyter/jupyterSession.ts
@@ -16,7 +16,7 @@ import { Event, EventEmitter } from 'vscode';
 import { CancellationToken } from 'vscode-jsonrpc';
 
 import { Cancellation } from '../../common/cancellation';
-import { sleep } from '../../common/utils/async';
+import { callWithTimeout, sleep } from '../../common/utils/async';
 import * as localize from '../../common/utils/localize';
 import { noop } from '../../common/utils/misc';
 import { IConnection, IJupyterKernelSpec, IJupyterSession } from '../types';
@@ -180,22 +180,23 @@ export class JupyterSession implements IJupyterSession {
                     this.statusHandler = undefined;
                 }
                 if (this.session) {
-                    // Shutdown may fail if the process has been killed
-                    await Promise.race([this.session.shutdown(), sleep(100)]);
+                    try {
+                        // Shutdown may fail if the process has been killed
+                        await Promise.race([this.session.shutdown(), sleep(100)]);
+                    } catch {
+                        noop();
+                    }
+                    // Dispose may not return. Wrap in a promise instead. Kernel futures can die if
+                    // process is already dead.
                     if (this.session) {
-                        this.session.dispose();
+                        await callWithTimeout(this.session.dispose.bind(this.session), 100);
                     }
                 }
                 if (this.sessionManager) {
-                    this.sessionManager.dispose();
+                    await callWithTimeout(this.sessionManager.dispose.bind(this.sessionManager), 100);
                 }
             } catch {
-                if (this.session) {
-                    this.session.dispose();
-                }
-                if (this.sessionManager) {
-                    this.sessionManager.dispose();
-                }
+                noop();
             }
             this.session = undefined;
             this.sessionManager = undefined;
diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts
@@ -427,17 +427,18 @@ suite('Jupyter notebook tests', () => {
 
     async function testCancelableCall<T>(method: (t: CancellationToken) => Promise<T>, messageFormat: string, timeout: number): Promise<boolean> {
         const tokenSource = new TaggedCancellationTokenSource(messageFormat.format(timeout.toString()));
+        let canceled = false;
         const disp = setTimeout((s) => {
+            canceled = true;
             tokenSource.cancel();
         }, timeout, tokenSource.tag);
 
         try {
             // tslint:disable-next-line:no-string-literal
             (tokenSource.token as any)['tag'] = messageFormat.format(timeout.toString());
             await method(tokenSource.token);
-            // We might get here before the cancel finishes. Wait for a timeout and then check cancel fired.
-            await sleep(timeout);
-            assert.ok(!tokenSource.token.isCancellationRequested, messageFormat.format(timeout.toString()));
+            // We might get here before the cancel finishes
+            assert.ok(!canceled, messageFormat.format(timeout.toString()));
         } catch (exc) {
             // This should happen. This means it was canceled.
             assert.ok(exc instanceof CancellationError, `Non cancellation error found : ${exc.stack}`);

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Handle done on all jupyter requests to make sure an unhandle exception isn't passed on shutdown.`