diff --git a/news/2 Fixes/5025.md b/news/2 Fixes/5025.md new file mode 100644 index 000000000000..1bbfc4f917f1 --- /dev/null +++ b/news/2 Fixes/5025.md @@ -0,0 +1 @@ +Restarting the kernel will eventually force Jupyter server to shutdown if it doesn't come back. diff --git a/src/client/common/utils/async.ts b/src/client/common/utils/async.ts index 6552ded1f723..885706c60fd7 100644 --- a/src/client/common/utils/async.ts +++ b/src/client/common/utils/async.ts @@ -3,9 +3,9 @@ 'use strict'; -export async function sleep(timeout: number) { - return new Promise((resolve) => { - setTimeout(resolve, timeout); +export async function sleep(timeout: number) : Promise { + return new Promise((resolve) => { + setTimeout(() => resolve(timeout), timeout); }); } diff --git a/src/client/datascience/history/history.ts b/src/client/datascience/history/history.ts index f753ae73edc6..debac6dee450 100644 --- a/src/client/datascience/history/history.ts +++ b/src/client/datascience/history/history.ts @@ -32,6 +32,7 @@ import { IInterpreterService, PythonInterpreter } from '../../interpreter/contra import { captureTelemetry, sendTelemetryEvent } from '../../telemetry'; import { EditorContexts, Identifiers, Telemetry } from '../constants'; import { JupyterInstallError } from '../jupyter/jupyterInstallError'; +import { JupyterKernelPromiseFailedError } from '../jupyter/jupyterKernelPromiseFailedError'; import { CellState, ICell, @@ -182,7 +183,7 @@ export class History implements IHistory { break; case HistoryMessages.RestartKernel: - this.restartKernel(); + this.restartKernel().ignoreErrors(); break; case HistoryMessages.ReturnAllCells: @@ -190,7 +191,7 @@ export class History implements IHistory { break; case HistoryMessages.Interrupt: - this.interruptKernel(); + this.interruptKernel().ignoreErrors(); break; case HistoryMessages.Export: @@ -313,58 +314,52 @@ export class History implements IHistory { } @captureTelemetry(Telemetry.RestartKernel) - public restartKernel() { + public async restartKernel() : Promise { if (this.jupyterServer && !this.restartingKernel) { // 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) { - this.restartKernelInternal().catch(e => { - this.applicationShell.showErrorMessage(e); - this.logger.logError(e); - }); - } - }); + const v = await this.applicationShell.showInformationMessage(message, yes, no); + if (v === yes) { + await this.restartKernelInternal(); + } } + + return Promise.resolve(); } @captureTelemetry(Telemetry.Interrupt) - public interruptKernel() { + public async interruptKernel() : Promise { if (this.jupyterServer && !this.restartingKernel) { 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.addSysInfo(SysInfoReason.Interrupt).ignoreErrors(); + try { + const result = await this.jupyterServer.interruptKernel(interruptTimeout); + status.dispose(); + + // We timed out, ask the user if they want to restart instead. + if (result === InterruptResult.TimedOut) { + const message = localize.DataScience.restartKernelAfterInterruptMessage(); + const yes = localize.DataScience.restartKernelMessageYes(); + const no = localize.DataScience.restartKernelMessageNo(); + const v = await this.applicationShell.showInformationMessage(message, yes, no); + if (v === yes) { + await this.restartKernelInternal(); } - }) - .catch(err => { - status.dispose(); - this.logger.logError(err); - this.applicationShell.showErrorMessage(err); - }); + } else if (result === InterruptResult.Restarted) { + // Uh-oh, keyboard interrupt crashed the kernel. + this.addSysInfo(SysInfoReason.Interrupt).ignoreErrors(); + } + } catch (err) { + status.dispose(); + this.logger.logError(err); + this.applicationShell.showErrorMessage(err); + } } } @@ -448,10 +443,7 @@ export class History implements IHistory { } } - private async restartKernelInternal(): Promise { - this.restartingKernel = true; - - // First we need to finish all outstanding cells. + private finishOutstandingCells() { this.unfinishedCells.forEach(c => { c.state = CellState.error; this.postMessage(HistoryMessages.FinishCell, c).ignoreErrors(); @@ -459,15 +451,33 @@ export class History implements IHistory { this.unfinishedCells = []; this.potentiallyUnfinishedStatus.forEach(s => s.dispose()); this.potentiallyUnfinishedStatus = []; + } + + private async restartKernelInternal(): Promise { + this.restartingKernel = true; + + // First we need to finish all outstanding cells. + this.finishOutstandingCells(); // Set our status const status = this.statusProvider.set(localize.DataScience.restartingKernelStatus()); try { if (this.jupyterServer) { - await this.jupyterServer.restartKernel(); + await this.jupyterServer.restartKernel(this.generateDataScienceExtraSettings().jupyterInterruptTimeout); await this.addSysInfo(SysInfoReason.Restart); } + } catch (exc) { + // If we get a kernel promise failure, then restarting timed out. Just shutdown and restart the entire server + if (exc instanceof JupyterKernelPromiseFailedError && this.jupyterServer) { + await this.jupyterServer.dispose(); + await this.loadJupyterServer(true); + await this.addSysInfo(SysInfoReason.Restart); + } else { + // Show the error message + this.applicationShell.showErrorMessage(exc); + this.logger.logError(exc); + } } finally { status.dispose(); this.restartingKernel = false; diff --git a/src/client/datascience/jupyter/jupyterExecution.ts b/src/client/datascience/jupyter/jupyterExecution.ts index ceeaa54dc43f..72763dc7e8be 100644 --- a/src/client/datascience/jupyter/jupyterExecution.ts +++ b/src/client/datascience/jupyter/jupyterExecution.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. 'use strict'; import { Kernel } from '@jupyterlab/services'; +import { execSync } from 'child_process'; import * as fs from 'fs-extra'; import * as os from 'os'; import * as path from 'path'; @@ -36,7 +37,6 @@ import { import { JupyterConnection, JupyterServerInfo } from './jupyterConnection'; import { JupyterKernelSpec } from './jupyterKernelSpec'; import { JupyterWaitForIdleError } from './jupyterWaitForIdleError'; -import { execSync } from 'child_process'; enum ModuleExistsResult { NotFound, @@ -360,7 +360,7 @@ export class JupyterExecutionBase implements IJupyterExecution { // We definitely need an ip address. extraArgs.push('--ip'); extraArgs.push('127.0.0.1'); - + // Now see if we need --allow-root. const idResults = execSync('id', {encoding: 'utf-8'}); if (idResults.includes('(root)')) { diff --git a/src/client/datascience/jupyter/jupyterInterruptError.ts b/src/client/datascience/jupyter/jupyterInterruptError.ts new file mode 100644 index 000000000000..8189045a6359 --- /dev/null +++ b/src/client/datascience/jupyter/jupyterInterruptError.ts @@ -0,0 +1,9 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +'use strict'; + +export class JupyterInterruptError extends Error { + constructor(message: string) { + super(message); + } +} diff --git a/src/client/datascience/jupyter/jupyterKernelPromiseFailedError.ts b/src/client/datascience/jupyter/jupyterKernelPromiseFailedError.ts new file mode 100644 index 000000000000..a1727c77877c --- /dev/null +++ b/src/client/datascience/jupyter/jupyterKernelPromiseFailedError.ts @@ -0,0 +1,9 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +'use strict'; + +export class JupyterKernelPromiseFailedError extends Error { + constructor(message: string) { + super(message); + } +} diff --git a/src/client/datascience/jupyter/jupyterServer.ts b/src/client/datascience/jupyter/jupyterServer.ts index bfd914a872db..636775fac3c3 100644 --- a/src/client/datascience/jupyter/jupyterServer.ts +++ b/src/client/datascience/jupyter/jupyterServer.ts @@ -80,12 +80,23 @@ class CellSubscriber { this.attemptToFinish(); } - public reject() { + // tslint:disable-next-line:no-any + public reject(e: any) { + if (!this.deferred.completed) { + this.cellRef.state = CellState.error; + this.subscriber.next(this.cellRef); + this.subscriber.complete(); + this.deferred.reject(e); + this.promiseComplete(this); + } + } + + public cancel() { if (!this.deferred.completed) { this.cellRef.state = CellState.error; this.subscriber.next(this.cellRef); this.subscriber.complete(); - this.deferred.reject(); + this.deferred.resolve(); this.promiseComplete(this); } } @@ -162,7 +173,7 @@ export class JupyterServerBase implements INotebookServer { // Wait for it to be ready traceInfo(`Waiting for idle ${this.id}`); - await this.session.waitForIdle(); + await this.session.waitForIdle(Number.MAX_SAFE_INTEGER); traceInfo(`Performing initial setup ${this.id}`); // Run our initial setup and plot magics @@ -186,8 +197,8 @@ export class JupyterServerBase implements INotebookServer { return this.shutdown(); } - public waitForIdle(): Promise { - return this.session ? this.session.waitForIdle() : Promise.resolve(); + public waitForIdle(timeoutMs: number): Promise { + return this.session ? this.session.waitForIdle(timeoutMs) : Promise.resolve(); } public execute(code: string, file: string, line: number, id: string, cancelToken?: CancellationToken, silent?: boolean): Promise { @@ -261,17 +272,16 @@ export class JupyterServerBase implements INotebookServer { }; } - public async restartKernel(): Promise { + public async restartKernel(timeoutMs: number): Promise { if (this.session) { // Update our start time so we don't keep sending responses this.sessionStartTime = Date.now(); // Complete all pending as an error. We're restarting - const copyPending = [...this.pendingCellSubscriptions]; - copyPending.forEach(c => c.reject()); + this.finishUncompletedCells(); // Restart our kernel - await this.session.restart(); + await this.session.restart(timeoutMs); // Rerun our initial setup for the notebook this.ranInitialSetup = false; @@ -289,14 +299,12 @@ export class JupyterServerBase implements INotebookServer { // restarted the kernel. const interruptBeginTime = Date.now(); - // Copy the list of pending cells. If these don't finish before the timeout - // then our interrupt didn't work. - const copyPending = [...this.pendingCellSubscriptions]; + // Get just the first pending cell (it should be the oldest). If it doesn't finish + // by our timeout, then our interrupt didn't work. + const firstPending = this.pendingCellSubscriptions.length > 0 ? this.pendingCellSubscriptions[0] : undefined; - // Create a promise that resolves when all of our currently - // pending cells finish. - const finished = copyPending.length > 0 ? - Promise.all(copyPending.map(d => d.promise)) : Promise.resolve([CellState.finished]); + // Create a promise that resolves when the first pending cell finishes + const finished = firstPending ? firstPending.promise : Promise.resolve(CellState.finished); // Create a deferred promise that resolves if we have a failure const restarted = createDeferred(); @@ -315,15 +323,12 @@ export class JupyterServerBase implements INotebookServer { restarted.resolve([]); // Fail all of the active (might be new ones) pending cell executes. We restarted. - const newCopyPending = [...this.pendingCellSubscriptions]; - newCopyPending.forEach(c => { - c.reject(); - }); + this.finishUncompletedCells(); }; const restartHandlerToken = this.session.onRestarted(restartHandler); // Start our interrupt. If it fails, indicate a restart - this.session.interrupt().catch(exc => { + this.session.interrupt(timeoutMs).catch(exc => { this.logger.logWarning(`Error during interrupt: ${exc}`); restarted.resolve([]); }); @@ -331,21 +336,25 @@ export class JupyterServerBase implements INotebookServer { try { // Wait for all of the pending cells to finish or the timeout to fire const result = await Promise.race([finished, restarted.promise, sleep(timeoutMs)]); - const states = result as CellState[]; // See if we restarted or not if (restarted.completed) { return InterruptResult.Restarted; } - if (states) { - // We got back the pending cells - return InterruptResult.Success; + // See if we timed out or not. + if (result === timeoutMs) { + // We timed out. You might think we should stop our pending list, but that's not + // up to us. The cells are still executing. The user has to request a restart or try again + return InterruptResult.TimedOut; } - // We timed out. You might think we should stop our pending list, but that's not - // up to us. The cells are still executing. The user has to request a restart or try again - return InterruptResult.TimedOut; + // Cancel all other pending cells as we interrupted. + this.finishUncompletedCells(); + + // Indicate the interrupt worked. + return InterruptResult.Success; + } catch (exc) { // Something failed. See if we restarted or not. if (this.sessionStartTime && (interruptBeginTime < this.sessionStartTime)) { @@ -379,6 +388,12 @@ export class JupyterServerBase implements INotebookServer { }; } + private finishUncompletedCells() { + const copyPending = [...this.pendingCellSubscriptions]; + copyPending.forEach(c => c.cancel()); + this.pendingCellSubscriptions = []; + } + private getDisposedError(): Error { // We may have been disposed because of a crash. See if our connection info is indicating shutdown if (this.serverExitCode) { diff --git a/src/client/datascience/jupyter/jupyterServerFactory.ts b/src/client/datascience/jupyter/jupyterServerFactory.ts index ccbdd3183d9b..96c4dd1fc895 100644 --- a/src/client/datascience/jupyter/jupyterServerFactory.ts +++ b/src/client/datascience/jupyter/jupyterServerFactory.ts @@ -83,9 +83,9 @@ export class JupyterServerFactory implements INotebookServer { return server.dispose(); } - public async waitForIdle(): Promise { + public async waitForIdle(timeoutMs: number): Promise { const server = await this.serverFactory.get(); - return server.waitForIdle(); + return server.waitForIdle(timeoutMs); } public async execute(code: string, file: string, line: number, id: string, cancelToken?: CancellationToken, silent?: boolean): Promise { @@ -118,9 +118,9 @@ export class JupyterServerFactory implements INotebookServer { }); } - public async restartKernel(): Promise { + public async restartKernel(timeoutMs: number): Promise { const server = await this.serverFactory.get(); - return server.restartKernel(); + return server.restartKernel(timeoutMs); } public async interruptKernel(timeoutMs: number): Promise { diff --git a/src/client/datascience/jupyter/jupyterSession.ts b/src/client/datascience/jupyter/jupyterSession.ts index 6d0d99654339..1aa2851da598 100644 --- a/src/client/datascience/jupyter/jupyterSession.ts +++ b/src/client/datascience/jupyter/jupyterSession.ts @@ -22,6 +22,7 @@ import { sleep } from '../../common/utils/async'; import * as localize from '../../common/utils/localize'; import { noop } from '../../common/utils/misc'; import { IConnection, IJupyterKernelSpec, IJupyterSession } from '../types'; +import { JupyterKernelPromiseFailedError } from './jupyterKernelPromiseFailedError'; import { JupyterWaitForIdleError } from './jupyterWaitForIdleError'; export class JupyterSession implements IJupyterSession { @@ -72,7 +73,7 @@ export class JupyterSession implements IJupyterSession { return this.onRestartedEvent.event; } - public async waitForIdle() : Promise { + public async waitForIdle(timeout: number) : Promise { if (this.session && this.session.kernel) { // This function seems to cause CI builds to timeout randomly on // different tests. Waiting for status to go idle doesn't seem to work and @@ -81,7 +82,7 @@ export class JupyterSession implements IJupyterSession { while (this.session && this.session.kernel && this.session.kernel.status !== 'idle' && - (Date.now() - startTime < 10000)) { + (Date.now() - startTime < timeout)) { traceInfo(`Waiting for idle: ${this.session.kernel.status}`); await sleep(10); } @@ -93,15 +94,15 @@ export class JupyterSession implements IJupyterSession { } } - public restart() : Promise { + public restart(timeout: number) : Promise { return this.session && this.session.kernel ? - this.waitForKernelPromise(this.session.kernel.restart(), localize.DataScience.restartingKernelFailed()) : + this.waitForKernelPromise(this.session.kernel.restart(), timeout, localize.DataScience.restartingKernelFailed()) : Promise.resolve(); } - public interrupt() : Promise { + public interrupt(timeout: number) : Promise { return this.session && this.session.kernel ? - this.waitForKernelPromise(this.session.kernel.interrupt(), localize.DataScience.interruptingKernelFailed()) : + this.waitForKernelPromise(this.session.kernel.interrupt(), timeout, localize.DataScience.interruptingKernelFailed()) : Promise.resolve(); } @@ -152,22 +153,13 @@ export class JupyterSession implements IJupyterSession { return this.connected; } - private async waitForKernelPromise(kernelPromise: Promise, errorMessage: string, secondTime?: boolean) : Promise { - // Wait for five seconds for this kernel promise to happen - await Promise.race([kernelPromise, sleep(5000)]); - - // If that didn't work, check status. Might have just not responded. - if (this.session && this.session.kernel && this.session.kernel.status === 'idle') { - return; - } - - // Otherwise wait another 5 seconds and check again - if (!secondTime) { - return this.waitForKernelPromise(kernelPromise, errorMessage, true); + private async waitForKernelPromise(kernelPromise: Promise, timeout: number, errorMessage: string) : Promise { + // Wait for this kernel promise to happen + const result = await Promise.race([kernelPromise, sleep(timeout)]); + if (result === timeout) { + // We timed out. Throw a specific exception + throw new JupyterKernelPromiseFailedError(errorMessage); } - - // If this is our second try, then show an error - throw new Error(errorMessage); } private onStatusChanged(_s: Session.ISession, a: Kernel.Status) { diff --git a/src/client/datascience/jupyter/liveshare/hostJupyterServer.ts b/src/client/datascience/jupyter/liveshare/hostJupyterServer.ts index 199bdcf724e3..86d9dc03ab01 100644 --- a/src/client/datascience/jupyter/liveshare/hostJupyterServer.ts +++ b/src/client/datascience/jupyter/liveshare/hostJupyterServer.ts @@ -81,7 +81,7 @@ export class HostJupyterServer // Requests return arrays service.onRequest(LiveShareCommands.syncRequest, (_args: any[], _cancellation: CancellationToken) => this.onSync()); service.onRequest(LiveShareCommands.getSysInfo, (_args: any[], cancellation: CancellationToken) => this.onGetSysInfoRequest(cancellation)); - service.onRequest(LiveShareCommands.restart, (_args: any[], cancellation: CancellationToken) => this.onRestartRequest(cancellation)); + service.onRequest(LiveShareCommands.restart, (args: any[], cancellation: CancellationToken) => this.onRestartRequest(args.length > 0 ? args[0] as number : LiveShare.InterruptDefaultTimeout, cancellation)); service.onRequest(LiveShareCommands.interrupt, (args: any[], cancellation: CancellationToken) => this.onInterruptRequest(args.length > 0 ? args[0] as number : LiveShare.InterruptDefaultTimeout, cancellation)); service.onRequest(LiveShareCommands.disposeServer, (_args: any[], _cancellation: CancellationToken) => this.dispose()); @@ -139,9 +139,9 @@ export class HostJupyterServer } } - public async restartKernel(): Promise { + public async restartKernel(timeoutMs: number): Promise { try { - await super.restartKernel(); + await super.restartKernel(timeoutMs); } catch (exc) { this.postException(exc, []); throw exc; @@ -228,9 +228,9 @@ export class HostJupyterServer return super.getSysInfo(); } - private onRestartRequest(_cancellation: CancellationToken) : Promise { + private onRestartRequest(timeout: number, _cancellation: CancellationToken) : Promise { // Just call the base - return super.restartKernel(); + return super.restartKernel(timeout); } private onInterruptRequest(timeout: number, _cancellation: CancellationToken) : Promise { // Just call the base diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index b9441bb89939..e2851666b5f5 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -57,8 +57,8 @@ export interface INotebookServer extends IAsyncDisposable { connect(launchInfo: INotebookServerLaunchInfo, cancelToken?: CancellationToken) : Promise; executeObservable(code: string, file: string, line: number, id: string, silent: boolean) : Observable; execute(code: string, file: string, line: number, id: string, cancelToken?: CancellationToken, silent?: boolean) : Promise; - restartKernel() : Promise; - waitForIdle() : Promise; + restartKernel(timeoutInMs: number) : Promise; + waitForIdle(timeoutInMs: number) : Promise; shutdown() : Promise; interruptKernel(timeoutInMs: number) : Promise; setInitialDirectory(directory: string): Promise; @@ -93,9 +93,9 @@ export interface IJupyterExecution extends IAsyncDisposable { export const IJupyterSession = Symbol('IJupyterSession'); export interface IJupyterSession extends IAsyncDisposable { onRestarted: Event; - restart() : Promise; - interrupt() : Promise; - waitForIdle() : Promise; + restart(timeout: number) : Promise; + interrupt(timeout: number) : Promise; + waitForIdle(timeout: number) : Promise; requestExecute(content: KernelMessage.IExecuteRequest, disposeOnDone?: boolean, metadata?: JSONObject) : Kernel.IFuture | undefined; } export const IJupyterSessionManager = Symbol('IJupyterSessionManager'); @@ -140,8 +140,8 @@ export interface IHistory extends Disposable { undoCells(): void; redoCells(): void; removeAllCells(): void; - interruptKernel(): void; - restartKernel(): void; + interruptKernel(): Promise; + restartKernel(): Promise; expandAllCells(): void; collapseAllCells(): void; exportCells(): void; diff --git a/src/test/datascience/dataScienceIocContainer.ts b/src/test/datascience/dataScienceIocContainer.ts index 24e66a694d60..1f05094b6fe8 100644 --- a/src/test/datascience/dataScienceIocContainer.ts +++ b/src/test/datascience/dataScienceIocContainer.ts @@ -453,6 +453,7 @@ export class DataScienceIocContainer extends UnitTestIocContainer { appShell.setup(a => a.showErrorMessage(TypeMoq.It.isAnyString())).returns((e) => { throw e; }); appShell.setup(a => a.showInformationMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('')); + appShell.setup(a => a.showInformationMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((_a1: string, a2: string, _a3: string) => Promise.resolve(a2)); appShell.setup(a => a.showSaveDialog(TypeMoq.It.isAny())).returns(() => Promise.resolve(Uri.file('test.ipynb'))); appShell.setup(a => a.setStatusBarMessage(TypeMoq.It.isAny())).returns(() => dummyDisposable); diff --git a/src/test/datascience/history.functional.test.tsx b/src/test/datascience/history.functional.test.tsx index 4ba6ed017378..71a2778b698d 100644 --- a/src/test/datascience/history.functional.test.tsx +++ b/src/test/datascience/history.functional.test.tsx @@ -78,6 +78,10 @@ suite('History output tests', () => { return ioc.wrapper!; } + // suiteTeardown(() => { + // asyncDump(); + // }); + teardown(async () => { for (const disposable of disposables) { if (!disposable) { @@ -90,7 +94,6 @@ suite('History output tests', () => { } } await ioc.dispose(); - delete (global as any).ascquireVsCodeApi; }); async function getOrCreateHistory(): Promise { @@ -537,4 +540,38 @@ for _ in range(50): await enterInput(wrapper, 'print("hello")'); verifyHtmlOnCell(wrapper, '>hello { + // Prime the pump + await addCode(getOrCreateHistory, wrapper, 'a=1\na'); + verifyHtmlOnCell(wrapper, '1', CellPosition.Last); + + // Then something that could possibly timeout + addContinuousMockData(ioc, 'import time\r\ntime.sleep(1000)', (_c) => { + return Promise.resolve({ result: '', haveMore: true}); + }); + + // Then get our mock session and force it to not restart ever. + if (ioc.mockJupyter) { + const currentSession = ioc.mockJupyter.getCurrentSession(); + if (currentSession) { + currentSession.prolongRestarts(); + } + } + + // Then try executing our long running cell and restarting in the middle + const history = await getOrCreateHistory(); + const executed = createDeferred(); + // We have to wait until the execute goes through before we reset. + history.onExecutedCode(() => executed.resolve()); + const added = history.addCode('import time\r\ntime.sleep(1000)', 'foo', 0); + await executed.promise; + await history.restartKernel(); + await added; + + // Now see if our wrapper still works. History should have force a restart + await history.addCode('a=1\na', 'foo', 0); + verifyHtmlOnCell(wrapper, '1', CellPosition.Last); + + }); }); diff --git a/src/test/datascience/mockJupyterManager.ts b/src/test/datascience/mockJupyterManager.ts index c01a790c1ab0..f9613db20716 100644 --- a/src/test/datascience/mockJupyterManager.ts +++ b/src/test/datascience/mockJupyterManager.ts @@ -61,6 +61,7 @@ export class MockJupyterManager implements IJupyterSessionManager { private sessionTimeout: number | undefined; private cellDictionary: Record = {}; private kernelSpecs : {name: string; dir: string}[] = []; + private currentSession: MockJupyterSession | undefined; constructor(serviceManager: IServiceManager) { // Save async registry. Need to stick servers created into it @@ -109,6 +110,10 @@ export class MockJupyterManager implements IJupyterSessionManager { this.activeInterpreter = interpreter; } + public getCurrentSession() : MockJupyterSession | undefined { + return this.currentSession; + } + public setProcessDelay(timeout: number | undefined) { this.processService.setDelay(timeout); this.pythonServices.forEach(p => p.setDelay(timeout)); @@ -217,10 +222,10 @@ export class MockJupyterManager implements IJupyterSessionManager { const localTimeout = this.sessionTimeout; return Cancellation.race(async () => { await sleep(localTimeout); - return new MockJupyterSession(this.cellDictionary, MockJupyterTimeDelay); + return this.createNewSession(); }, cancelToken); } else { - return Promise.resolve(new MockJupyterSession(this.cellDictionary, MockJupyterTimeDelay)); + return Promise.resolve(this.createNewSession()); } } @@ -232,6 +237,11 @@ export class MockJupyterManager implements IJupyterSessionManager { this.changedInterpreterEvent.fire(); } + private createNewSession() : MockJupyterSession { + this.currentSession = new MockJupyterSession(this.cellDictionary, MockJupyterTimeDelay); + return this.currentSession; + } + private createStreamResult(str: string) : nbformat.IStream { return { output_type: 'stream', diff --git a/src/test/datascience/mockJupyterRequest.ts b/src/test/datascience/mockJupyterRequest.ts index ef2fd64f17ee..83e94a3c7a95 100644 --- a/src/test/datascience/mockJupyterRequest.ts +++ b/src/test/datascience/mockJupyterRequest.ts @@ -191,10 +191,12 @@ export class MockJupyterRequest implements Kernel.IFuture { } // Move onto the next producer if allowed - if (r.haveMore) { - this.sendMessages(producers, delay); - } else { - this.sendMessages(producers.slice(1), delay); + if (!this.cancelToken.isCancellationRequested) { + if (r.haveMore) { + this.sendMessages(producers, delay); + } else { + this.sendMessages(producers.slice(1), delay); + } } }).ignoreErrors(); }, delay); diff --git a/src/test/datascience/mockJupyterSession.ts b/src/test/datascience/mockJupyterSession.ts index f71a0e0710e9..2da97a1512cd 100644 --- a/src/test/datascience/mockJupyterSession.ts +++ b/src/test/datascience/mockJupyterSession.ts @@ -5,6 +5,7 @@ import { Kernel, KernelMessage } from '@jupyterlab/services'; import { JSONObject } from '@phosphor/coreutils/lib/json'; import { CancellationTokenSource, Event, EventEmitter } from 'vscode'; +import { JupyterKernelPromiseFailedError } from '../../client/datascience/jupyter/jupyterKernelPromiseFailedError'; import { ICell, IJupyterSession } from '../../client/datascience/types'; import { sleep } from '../core'; import { MockJupyterRequest } from './mockJupyterRequest'; @@ -19,6 +20,7 @@ export class MockJupyterSession implements IJupyterSession { private executionCount: number = 0; private outstandingRequestTokenSources: CancellationTokenSource[] = []; private executes: string[] = []; + private forceRestartTimeout : boolean = false; constructor(cellDictionary: Record, timedelay: number) { this.dict = cellDictionary; @@ -29,20 +31,29 @@ export class MockJupyterSession implements IJupyterSession { return this.restartedEvent.event; } - public async restart(): Promise { + public async restart(_timeout: number): Promise { // For every outstanding request, switch them to fail mode const requests = [...this.outstandingRequestTokenSources]; requests.forEach(r => r.cancel()); + + if (this.forceRestartTimeout) { + throw new JupyterKernelPromiseFailedError('Forcing restart timeout'); + } + return sleep(this.timedelay); } - public interrupt(): Promise { + public interrupt(_timeout: number): Promise { const requests = [...this.outstandingRequestTokenSources]; requests.forEach(r => r.cancel()); return sleep(this.timedelay); } - public waitForIdle(): Promise { + public waitForIdle(_timeout: number): Promise { return sleep(this.timedelay); } + + public prolongRestarts() { + this.forceRestartTimeout = true; + } public requestExecute(content: KernelMessage.IExecuteRequest, _disposeOnDone?: boolean, _metadata?: JSONObject): Kernel.IFuture { // Content should have the code const cell = this.findCell(content.code); diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts index ed60691484d8..876494f6e274 100644 --- a/src/test/datascience/notebook.functional.test.ts +++ b/src/test/datascience/notebook.functional.test.ts @@ -20,6 +20,7 @@ import { createDeferred } from '../../client/common/utils/async'; import { noop } from '../../client/common/utils/misc'; import { concatMultilineString } from '../../client/datascience/common'; import { JupyterExecutionFactory } from '../../client/datascience/jupyter/jupyterExecutionFactory'; +import { JupyterKernelPromiseFailedError } from '../../client/datascience/jupyter/jupyterKernelPromiseFailedError'; import { IRoleBasedObject, RoleBasedFactory } from '../../client/datascience/jupyter/liveshare/roleBasedFactory'; import { CellState, @@ -388,17 +389,22 @@ suite('Jupyter notebook tests', () => { // In unit tests we have to wait for status idle before restarting. Unit tests // seem to be timing out if the restart throws any exceptions (even if they're caught) - await server!.waitForIdle(); + await server!.waitForIdle(10000); console.log('Restarting kernel'); + try { + await server!.restartKernel(10000); - await server!.restartKernel(); + console.log('Waiting for idle'); + await server!.waitForIdle(10000); - console.log('Waiting for idle'); - await server!.waitForIdle(); + console.log('Verifying restart'); + await verifyError(server, 'a', `name 'a' is not defined`); + + } catch (exc) { + assert.ok(exc instanceof JupyterKernelPromiseFailedError, 'Restarting did not timeout correctly'); + } - console.log('Verifying restart'); - await verifyError(server, 'a', `name 'a' is not defined`); }); class TaggedCancellationTokenSource extends CancellationTokenSource {