Skip to content

Allow double (or more) interrupts #12278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/2 Fixes/10587.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow interrupt to happen more than once in a notebook or the interactive window.
6 changes: 1 addition & 5 deletions src/client/datascience/interactive-common/interactiveBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -430,8 +430,6 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
@captureTelemetry(Telemetry.Interrupt)
public async interruptKernel(): Promise<void> {
if (this._notebook && !this.restartingKernel) {
this.restartingKernel = true;

const status = this.statusProvider.set(
localize.DataScience.interruptKernelStatus(),
true,
Expand All @@ -448,7 +446,7 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
status.dispose();

// We timed out, ask the user if they want to restart instead.
if (result === InterruptResult.TimedOut) {
if (result === InterruptResult.TimedOut && !this.restartingKernel) {
const message = localize.DataScience.restartKernelAfterInterruptMessage();
const yes = localize.DataScience.restartKernelMessageYes();
const no = localize.DataScience.restartKernelMessageNo();
Expand All @@ -464,8 +462,6 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
status.dispose();
traceError(err);
this.applicationShell.showErrorMessage(err);
} finally {
this.restartingKernel = false;
}
}
}
Expand Down
8 changes: 8 additions & 0 deletions src/client/datascience/jupyter/jupyterNotebook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,12 @@ export class JupyterNotebookBase implements INotebook {
public get onKernelRestarted(): Event<void> {
return this.kernelRestarted.event;
}

public get onKernelInterrupted(): Event<void> {
return this.kernelInterrupted.event;
}
private readonly kernelRestarted = new EventEmitter<void>();
private readonly kernelInterrupted = new EventEmitter<void>();
private disposed = new EventEmitter<void>();
private sessionStatusChanged: Disposable | undefined;
private initializedMatplotlib = false;
Expand Down Expand Up @@ -522,6 +527,9 @@ export class JupyterNotebookBase implements INotebook {
// Cancel all other pending cells as we interrupted.
this.finishUncompletedCells();

// Fire event that we interrupted.
this.kernelInterrupted.fire();

// Indicate the interrupt worked.
return InterruptResult.Success;
} catch (exc) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export class GuestJupyterNotebook
IJupyterKernelSpec | LiveKernelModel
>().event;
public onKernelRestarted = new EventEmitter<void>().event;
public onKernelInterrupted = new EventEmitter<void>().event;
public onDisposed = new EventEmitter<void>().event;
private _jupyterLab?: typeof import('@jupyterlab/services');
private responseQueue: ResponseQueue = new ResponseQueue();
Expand Down
1 change: 1 addition & 0 deletions src/client/datascience/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ export interface INotebook extends IAsyncDisposable {
onDisposed: Event<void>;
onKernelChanged: Event<IJupyterKernelSpec | LiveKernelModel>;
onKernelRestarted: Event<void>;
onKernelInterrupted: Event<void>;
clear(id: string): void;
executeObservable(code: string, file: string, line: number, id: string, silent: boolean): Observable<ICell[]>;
execute(
Expand Down
2 changes: 0 additions & 2 deletions src/datascience-ui/history-react/interactivePanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ ${buildSettingsCss(this.props.settings)}`}</style>
<ImageButton
baseTheme={this.props.baseTheme}
onClick={this.props.restartKernel}
disabled={this.props.busy}
tooltip={getLocString('DataScience.restartServer', 'Restart IPython kernel')}
>
<Image
Expand All @@ -148,7 +147,6 @@ ${buildSettingsCss(this.props.settings)}`}</style>
<ImageButton
baseTheme={this.props.baseTheme}
onClick={this.props.interruptKernel}
disabled={this.props.busy}
tooltip={getLocString('DataScience.interruptKernel', 'Interrupt IPython kernel')}
>
<Image
Expand Down
4 changes: 2 additions & 2 deletions src/datascience-ui/native-editor/toolbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ export class Toolbar extends React.PureComponent<INativeEditorToolbarProps> {
<ImageButton
baseTheme={this.props.baseTheme}
onClick={this.props.restartKernel}
disabled={this.props.busy || !canRestartAndInterruptKernel}
disabled={!canRestartAndInterruptKernel}
className="native-button"
tooltip={getLocString('DataScience.restartServer', 'Restart IPython kernel')}
>
Expand All @@ -173,7 +173,7 @@ export class Toolbar extends React.PureComponent<INativeEditorToolbarProps> {
<ImageButton
baseTheme={this.props.baseTheme}
onClick={this.props.interruptKernel}
disabled={this.props.busy || !canRestartAndInterruptKernel}
disabled={!canRestartAndInterruptKernel}
className="native-button"
tooltip={getLocString('DataScience.interruptKernel', 'Interrupt IPython kernel')}
>
Expand Down
48 changes: 47 additions & 1 deletion src/test/datascience/interactiveWindow.functional.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { Disposable, Selection, TextDocument, TextEditor, Uri } from 'vscode';
import { ReactWrapper } from 'enzyme';
import { IApplicationShell, IDocumentManager } from '../../client/common/application/types';
import { IDataScienceSettings } from '../../client/common/types';
import { createDeferred, waitForPromise } from '../../client/common/utils/async';
import { createDeferred, sleep, waitForPromise } from '../../client/common/utils/async';
import { noop } from '../../client/common/utils/misc';
import { EXTENSION_ROOT_DIR } from '../../client/constants';
import { generateCellsFromDocument } from '../../client/datascience/cellFactory';
Expand Down Expand Up @@ -555,6 +555,52 @@ Type: builtin_function_or_method`,
}
);

const interruptCode = `
import time
for i in range(0, 100):
try:
time.sleep(0.5)
except KeyboardInterrupt:
time.sleep(0.5)`;

runMountedTest(
'Interrupt double',
async (wrapper) => {
let interruptedKernel = false;
const interactiveWindow = await getOrCreateInteractiveWindow(ioc);
await interactiveWindow.show();
interactiveWindow.notebook?.onKernelInterrupted(() => (interruptedKernel = true));

let timerCount = 0;
addContinuousMockData(ioc, interruptCode, async (_c) => {
timerCount += 1;
await sleep(0.5);
return Promise.resolve({ result: '', haveMore: timerCount < 100 });
});

addMockData(ioc, interruptCode, undefined, 'text/plain');

// Run the interrupt code and then interrupt it twice to make sure we can interrupt twice
const waitForAdd = addCode(ioc, wrapper, interruptCode);

// 'Click' the button in the react control. We need to verify we can
// click it more than once.
const interrupt = findButton(wrapper, InteractivePanel, 4);
interrupt?.simulate('click');
await sleep(0.1);
interrupt?.simulate('click');

// We should get out of the wait for add
await waitForAdd;

// We should have also fired an interrupt
assert.ok(interruptedKernel, 'Kernel was not interrupted');
},
() => {
return ioc;
}
);

runMountedTest(
'Export',
async (wrapper) => {
Expand Down
4 changes: 4 additions & 0 deletions src/test/datascience/mockJupyterNotebook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ export class MockJupyterNotebook implements INotebook {
>().event;
public onDisposed = new EventEmitter<void>().event;
public onKernelRestarted = new EventEmitter<void>().event;
public get onKernelInterrupted(): Event<void> {
return this.kernelInterrupted.event;
}
private kernelInterrupted = new EventEmitter<void>();
private onStatusChangedEvent: EventEmitter<ServerStatus> | undefined;

constructor(private providerConnection: INotebookProviderConnection | undefined) {
Expand Down
13 changes: 3 additions & 10 deletions src/test/datascience/nativeEditor.toolbar.functional.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -176,13 +176,6 @@ suite('DataScience Native Toolbar', () => {
suite('Restart & Interrupt Kernel', () => {
getNamesAndValues<ServerStatus>(ServerStatus).forEach((status) => {
// Should always be disabled if busy.
test(`If Kernel status is ${status.name} and busy, both are disabled`, () => {
props.kernel.jupyterServerStatus = status.name as any;
props.busy = true;
mountToolbar();
assertDisabled(Button.RestartKernel);
assertDisabled(Button.InterruptKernel);
});
if (status.name === ServerStatus.NotStarted) {
// Should be disabled if not busy and status === 'Not Started'.
test(`If Kernel status is ${ServerStatus.NotStarted} and not busy, both are disabled`, () => {
Expand All @@ -193,10 +186,10 @@ suite('DataScience Native Toolbar', () => {
assertDisabled(Button.InterruptKernel);
});
} else {
// Should be enabled if not busy and status != 'Not Started'.
test(`If Kernel status is ${status.name} and not busy, both are enabled`, () => {
// Should be enabled if busy and status != 'Not Started'.
test(`If Kernel status is ${status.name}, both are enabled`, () => {
props.kernel.jupyterServerStatus = status.name as any;
props.busy = false;
props.busy = true;
mountToolbar();
assertEnabled(Button.RestartKernel);
assertEnabled(Button.InterruptKernel);
Expand Down