Fix interrupt to support a timeout and handle crashes

news/2 Fixes/3511.md

@@ -0,0 +1 @@
+Handle interrupt crashing the kernel and provide a timeout for interrupting in case interrupts are not handled.

package.json

@@ -1042,6 +1042,12 @@
  "description": "When running Jupyter locally, create a default empty Jupyter config for the Python Interactive window",
  "scope": "resource"
  },
+ "python.dataScience.jupyterInterruptTimeout": {
+ "type": "number",
+ "default": 10000,
+ "description": "Amount of time (in ms) to wait for an interrupt before asking to restart the Jupyter kernel.",
+ "scope": "resource"
+ },
  "python.disableInstallationCheck": {
  "type": "boolean",
  "default": false,

src/client/common/cancellation.ts

@@ -2,6 +2,8 @@
 // Licensed under the MIT License.
 'use strict';
 import { CancellationToken } from 'vscode-jsonrpc';
+
+import { createDeferred } from './utils/async';
 import * as localize from './utils/localize';
 
 /**
@@ -23,11 +25,37 @@ export namespace Cancellation {
  */
  export function race<T>(work : (token?: CancellationToken) => Promise<T>, token?: CancellationToken) : Promise<T> {
  if (token) {
- // Race rejection. This allows the callback to run because the second promise
- // will be in the promise queue.
- return Promise.race([work(token), new Promise<T>((resolve, reject) => {
- token.onCancellationRequested(() => reject(new CancellationError()));
- })]);
+ // Use a deferred promise. Resolves when the work finishes
+ const deferred = createDeferred<T>();
+
+ // Cancel the deferred promise when the cancellation happens
+ token.onCancellationRequested(() => {
+ if (!deferred.completed) {
+ deferred.reject(new CancellationError());
+ }
+ });
+
+ // Might already be canceled
+ if (token.isCancellationRequested) {
+ // Just start out as rejected
+ deferred.reject(new CancellationError());
+ } else {
+ // Not canceled yet. When the work finishes
+ // either resolve our promise or cancel.
+ work(token)
+ .then((v) => {
+ if (!deferred.completed) {
+ deferred.resolve(v);
+ }
+ })
+ .catch((e) => {
+ if (!deferred.completed) {
+ deferred.reject(e);
+ }
+ });
+ }
+
+ return deferred.promise;
  } else {
  // No actual token, just do the original work.
  return work();

src/client/common/types.ts

@@ -266,6 +266,7 @@ export interface IAnalysisSettings {
 export interface IDataScienceSettings {
  allowImportFromNotebook: boolean;
  enabled: boolean;
+ jupyterInterruptTimeout: number;
  jupyterLaunchTimeout: number;
  jupyterServerURI: string;
  notebookFileRoot: string;

src/client/common/utils/localize.ts

@@ -99,7 +99,10 @@ export namespace DataScience {
  export const jupyterKernelNotSupportedOnActive = localize('DataScience.jupyterKernelNotSupportedOnActive', `iPython kernel cannot be started from '{0}'. Using closest match {1} instead.`);
  export const jupyterKernelSpecNotFound = localize('DataScience.jupyterKernelSpecNotFound', 'Cannot create a iPython kernel spec and none are available for use');
  export const interruptKernel = localize('DataScience.interruptKernel', 'Interrupt iPython Kernel');
+ export const interruptKernelStatus = localize('DataScience.interruptKernelStatus', 'Interrupting iPython Kernel');
  export const exportCancel = localize('DataScience.exportCancel', 'Cancel');
+ export const restartKernelAfterInterruptMessage = localize('DataScience.restartKernelAfterInterruptMessage', 'Interrupting the kernel timed out. Do you want to restart the kernel instead? All variables will be lost.');
+ export const pythonInterruptFailedHeader = localize('DataScience.pythonInterruptFailedHeader', 'Keyboard interrupt crashed the kernel. Kernel restarted.');
 }
 
 // Skip using vscode-nls and instead just compute our strings based on key values. Key values

src/client/datascience/history.ts

@@ -39,6 +39,7 @@ import {
  IJupyterExecution,
  INotebookExporter,
  INotebookServer,
+ InterruptResult,
  IStatusProvider
 } from './types';
 
@@ -148,7 +149,7 @@ export class History implements IWebPanelMessageListener, IHistory {
  status.dispose();
 
  // We failed, dispose of ourselves too so that nobody uses us again
- this.dispose();
+ this.dispose().ignoreErrors();
 
  throw err;
  }
@@ -217,13 +218,13 @@ export class History implements IWebPanelMessageListener, IHistory {
  }
  }
 
- public dispose() {
+ public async dispose()  {
  if (!this.disposed) {
  this.disposed = true;
  this.settingsChangedDisposable.dispose();
  this.closedEvent.fire(this);
  if (this.jupyterServer) {
- this.jupyterServer.shutdown();
+ await this.jupyterServer.shutdown();
  }
  this.updateContexts();
  }
@@ -265,43 +266,17 @@ export class History implements IWebPanelMessageListener, IHistory {
  @captureTelemetry(Telemetry.RestartKernel)
  public restartKernel() {
  if (this.jupyterServer && !this.restartingKernel) {
- this.restartingKernel = true;
-
  // Ask the user if they want us to restart or not.
  const message = localize.DataScience.restartKernelMessage();
  const yes = localize.DataScience.restartKernelMessageYes();
  const no = localize.DataScience.restartKernelMessageNo();
 
  this.applicationShell.showInformationMessage(message, yes, no).then(v => {
  if (v === yes) {
- // First we need to finish all outstanding cells.
- this.unfinishedCells.forEach(c => {
- c.state = CellState.error;
- if (this.webPanel) {
- this.webPanel.postMessage({ type: HistoryMessages.FinishCell, payload: c });
- }
+ this.restartKernelInternal().catch(e => {
+ this.applicationShell.showErrorMessage(e);
+ this.logger.logError(e);
  });
- this.unfinishedCells = [];
- this.potentiallyUnfinishedStatus.forEach(s => s.dispose());
- this.potentiallyUnfinishedStatus = [];
-
- // Set our status
- const status = this.statusProvider.set(localize.DataScience.restartingKernelStatus(), this);
-
- // Then restart the kernel. When that finishes, add our sys info again
- if (this.jupyterServer) {
- this.jupyterServer.restartKernel()
- .then(() => {
- this.addRestartSysInfo().then(status.dispose()).ignoreErrors();
- })
- .catch(err => {
- this.logger.logError(err);
- status.dispose();
- });
- }
- this.restartingKernel = false;
- } else {
- this.restartingKernel = false;
  }
  });
  }
@@ -310,14 +285,68 @@ export class History implements IWebPanelMessageListener, IHistory {
  @captureTelemetry(Telemetry.Interrupt)
  public interruptKernel() {
  if (this.jupyterServer && !this.restartingKernel) {
- this.jupyterServer.interruptKernel()
- .then()
+ const status = this.statusProvider.set(localize.DataScience.interruptKernelStatus());
+
+ const settings = this.configuration.getSettings();
+ const interruptTimeout = settings.datascience.jupyterInterruptTimeout;
+
+ this.jupyterServer.interruptKernel(interruptTimeout)
+ .then(result => {
+ status.dispose();
+ if (result === InterruptResult.TimedOut) {
+ const message = localize.DataScience.restartKernelAfterInterruptMessage();
+ const yes = localize.DataScience.restartKernelMessageYes();
+ const no = localize.DataScience.restartKernelMessageNo();
+
+ this.applicationShell.showInformationMessage(message, yes, no).then(v => {
+ if (v === yes) {
+ this.restartKernelInternal().catch(e => {
+ this.applicationShell.showErrorMessage(e);
+ this.logger.logError(e);
+ });
+ }
+ });
+ } else if (result === InterruptResult.Restarted) {
+ // Uh-oh, keyboard interrupt crashed the kernel.
+ this.addInterruptFailedInfo().ignoreErrors();
+ }
+ })
  .catch(err => {
+ status.dispose();
  this.logger.logError(err);
+ this.applicationShell.showErrorMessage(err);
  });
  }
  }
 
+ private async restartKernelInternal() : Promise<void> {
+ this.restartingKernel = true;
+
+ // First we need to finish all outstanding cells.
+ this.unfinishedCells.forEach(c => {
+ c.state = CellState.error;
+ if (this.webPanel) {
+ this.webPanel.postMessage({ type: HistoryMessages.FinishCell, payload: c });
+ }
+ });
+ this.unfinishedCells = [];
+ this.potentiallyUnfinishedStatus.forEach(s => s.dispose());
+ this.potentiallyUnfinishedStatus = [];
+
+ // Set our status
+ const status = this.statusProvider.set(localize.DataScience.restartingKernelStatus());
+
+ try {
+ if (this.jupyterServer) {
+ await this.jupyterServer.restartKernel();
+ await this.addRestartSysInfo();
+ }
+ } finally {
+ status.dispose();
+ this.restartingKernel = false;
+ }
+ }
+
  // tslint:disable-next-line:no-any
  private handleReturnAllCells = (payload: any) => {
  // See what we're waiting for.
@@ -351,7 +380,7 @@ export class History implements IWebPanelMessageListener, IHistory {
  }
 
  private setStatus = (message: string) : Disposable => {
- const result = this.statusProvider.set(message, this);
+ const result = this.statusProvider.set(message);
  this.potentiallyUnfinishedStatus.push(result);
  return result;
  }
@@ -642,6 +671,11 @@ export class History implements IWebPanelMessageListener, IHistory {
  return this.addSysInfo(localize.DataScience.pythonRestartHeader());
  }
 
+ private addInterruptFailedInfo = () : Promise<void> => {
+ this.addedSysInfo = false;
+ return this.addSysInfo(localize.DataScience.pythonInterruptFailedHeader());
+ }
+
  private addSysInfo = async (message: string) : Promise<void> => {
  // Add our sys info if necessary
  if (!this.addedSysInfo) {

src/client/datascience/historycommandlistener.ts

@@ -232,35 +232,59 @@ export class HistoryCommandListener implements IDataScienceCommandListener {
  }
 
  private undoCells() {
- this.historyProvider.getActive()!.undoCells();
+ const history = this.historyProvider.getActive();
+ if (history) {
+ history.undoCells();
+ }
  }
 
  private redoCells() {
- this.historyProvider.getActive()!.redoCells();
+ const history = this.historyProvider.getActive();
+ if (history) {
+ history.redoCells();
+ }
  }
 
  private removeAllCells() {
- this.historyProvider.getActive()!.removeAllCells();
+ const history = this.historyProvider.getActive();
+ if (history) {
+ history.removeAllCells();
+ }
  }
 
  private interruptKernel() {
- this.historyProvider.getActive()!.interruptKernel();
+ const history = this.historyProvider.getActive();
+ if (history) {
+ history.interruptKernel();
+ }
  }
 
  private restartKernel() {
- this.historyProvider.getActive()!.restartKernel();
+ const history = this.historyProvider.getActive();
+ if (history) {
+ history.restartKernel();
+ }
  }
 
  private expandAllCells() {
- this.historyProvider.getActive()!.expandAllCells();
+ const history = this.historyProvider.getActive();
+ if (history) {
+ history.expandAllCells();
+ }
  }
 
  private collapseAllCells() {
- this.historyProvider.getActive()!.collapseAllCells();
+ const history = this.historyProvider.getActive();
+ if (history) {
+ history.collapseAllCells();
+ }
  }
 
  private exportCells() {
- this.historyProvider.getActive()!.exportCells();
+ const history = this.historyProvider.getActive();
+ if (history) {
+ history.exportCells();
+ }
  }
 
  private canImportFromOpenedFile = () => {
@@ -306,7 +330,7 @@ export class HistoryCommandListener implements IDataScienceCommandListener {
 
  private waitForStatus<T>(promise: () => Promise<T>, format: string, file?: string, canceled?: () => void) : Promise<T> {
  const message = file ? format.format(file) : format;
- return this.statusProvider.waitWithStatus(promise, message, undefined, undefined, canceled);
+ return this.statusProvider.waitWithStatus(promise, message, undefined, canceled);
  }
 
  @captureTelemetry(Telemetry.ImportNotebook, { scope: 'command' }, false)

src/client/datascience/jupyterConnection.ts

@@ -15,7 +15,6 @@ import { JupyterConnectError } from './jupyterConnectError';
 import { IConnection } from './types';
 
 const UrlPatternRegEx = /(https?:\/\/[^\s]+)/ ;
-const ForbiddenPatternRegEx = /Forbidden/;
 const HttpPattern = /https?:\/\//;
 
 export type JupyterServerInfo = {
@@ -152,11 +151,8 @@ class JupyterConnectionWaiter {
  }).ignoreErrors();
  }
 
- // Look for 'Forbidden' in the result
- const forbiddenMatch = ForbiddenPatternRegEx.exec(data);
- if (forbiddenMatch && this.startPromise && !this.startPromise.resolved) {
- this.rejectStartPromise(data.toString('utf8'));
- }
+ // Sometimes jupyter will return a 403 error. Not sure why. We used
+ // to fail on this, but it looks like jupyter works with this error in place.
  }
 
  private launchTimedOut = () => {