Skip to content

Commit 493d27b

Browse files
roblourensroblourens
authored
Don't interrupt when deleting pending cells (microsoft#163609)
Fix microsoft#163133
1 parent 7ef8e6b commit 493d27b

File tree

7 files changed

+81
-39
lines changed

7 files changed

+81
-39
lines changed

‎src/vs/workbench/contrib/notebook/browser/contrib/execute/executionEditorProgress.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ export class ExecutionEditorProgressController extends Disposable implements INo
3838
return;
3939
}
4040

41-
const executing = this._notebookExecutionStateService.getCellExecutionStatesForNotebook(this._notebookEditor.textModel?.uri)
41+
const executing = this._notebookExecutionStateService.getCellExecutionsForNotebook(this._notebookEditor.textModel?.uri)
4242
.filter(exe => exe.state === NotebookCellExecutionState.Executing);
4343
const executionIsVisible = (exe: INotebookCellExecution) => {
4444
for (const range of this._notebookEditor.visibleRanges) {

‎src/vs/workbench/contrib/notebook/browser/controller/executeActions.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -618,7 +618,7 @@ registerAction2(class RevealRunningCellAction extends NotebookAction {
618618
async runWithContext(accessor: ServicesAccessor, context: INotebookActionContext): Promise<void> {
619619
const notebookExecutionStateService = accessor.get(INotebookExecutionStateService);
620620
const notebook = context.notebookEditor.textModel.uri;
621-
const executingCells = notebookExecutionStateService.getCellExecutionStatesForNotebook(notebook);
621+
const executingCells = notebookExecutionStateService.getCellExecutionsForNotebook(notebook);
622622
if (executingCells[0]) {
623623
const cell = context.notebookEditor.getCellByHandle(executingCells[0].cellHandle);
624624
if (cell) {

‎src/vs/workbench/contrib/notebook/browser/services/notebookExecutionStateServiceImpl.ts

Lines changed: 37 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,15 @@ import { Emitter } from 'vs/base/common/event';
77
import { combinedDisposable, Disposable, IDisposable } from 'vs/base/common/lifecycle';
88
import { ResourceMap } from 'vs/base/common/map';
99
import { isEqual } from 'vs/base/common/resources';
10+
import { withNullAsUndefined } from 'vs/base/common/types';
1011
import { URI } from 'vs/base/common/uri';
1112
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
1213
import { ILogService } from 'vs/platform/log/common/log';
1314
import { NotebookTextModel } from 'vs/workbench/contrib/notebook/common/model/notebookTextModel';
1415
import { CellEditType, CellUri, ICellEditOperation, NotebookCellExecutionState, NotebookCellInternalMetadata, NotebookTextModelWillAddRemoveEvent } from 'vs/workbench/contrib/notebook/common/notebookCommon';
1516
import { CellExecutionUpdateType, INotebookExecutionService } from 'vs/workbench/contrib/notebook/common/notebookExecutionService';
1617
import { ICellExecuteUpdate, ICellExecutionComplete, ICellExecutionStateChangedEvent, ICellExecutionStateUpdate, IFailedCellInfo, INotebookCellExecution, INotebookExecutionStateService, INotebookFailStateChangedEvent } from 'vs/workbench/contrib/notebook/common/notebookExecutionStateService';
18+
import { INotebookKernelService } from 'vs/workbench/contrib/notebook/common/notebookKernelService';
1719
import { INotebookService } from 'vs/workbench/contrib/notebook/common/notebookService';
1820

1921
export class NotebookExecutionStateService extends Disposable implements INotebookExecutionStateService {
@@ -68,11 +70,16 @@ export class NotebookExecutionStateService extends Disposable implements INotebo
6870
return undefined;
6971
}
7072

71-
getCellExecutionStatesForNotebook(notebook: URI): INotebookCellExecution[] {
73+
getCellExecutionsForNotebook(notebook: URI): INotebookCellExecution[] {
7274
const exeMap = this._executions.get(notebook);
7375
return exeMap ? Array.from(exeMap.values()) : [];
7476
}
7577

78+
getCellExecutionsByHandleForNotebook(notebook: URI): Map<number, INotebookCellExecution> | undefined {
79+
const exeMap = this._executions.get(notebook);
80+
return withNullAsUndefined(exeMap);
81+
}
82+
7683
private _onCellExecutionDidChange(notebookUri: URI, cellHandle: number, exe: CellExecution): void {
7784
this._onDidChangeCellExecution.fire(new NotebookExecutionEvent(notebookUri, cellHandle, exe));
7885
}
@@ -244,6 +251,7 @@ class NotebookExecutionListeners extends Disposable {
244251
constructor(
245252
notebook: URI,
246253
@INotebookService private readonly _notebookService: INotebookService,
254+
@INotebookKernelService private readonly _notebookKernelService: INotebookKernelService,
247255
@INotebookExecutionService private readonly _notebookExecutionService: INotebookExecutionService,
248256
@INotebookExecutionStateService private readonly _notebookExecutionStateService: INotebookExecutionStateService,
249257
@ILogService private readonly _logService: ILogService,
@@ -263,7 +271,7 @@ class NotebookExecutionListeners extends Disposable {
263271

264272
private cancelAll(): void {
265273
this._logService.debug(`NotebookExecutionListeners#cancelAll`);
266-
const exes = this._notebookExecutionStateService.getCellExecutionStatesForNotebook(this._notebookModel.uri);
274+
const exes = this._notebookExecutionStateService.getCellExecutionsForNotebook(this._notebookModel.uri);
267275
this._notebookExecutionService.cancelNotebookCellHandles(this._notebookModel, exes.map(exe => exe.cellHandle));
268276
}
269277

@@ -273,25 +281,34 @@ class NotebookExecutionListeners extends Disposable {
273281
}
274282

275283
private onWillAddRemoveCells(e: NotebookTextModelWillAddRemoveEvent): void {
276-
const notebookExes = this._notebookExecutionStateService.getCellExecutionStatesForNotebook(this._notebookModel.uri);
277-
const handles = new Set(notebookExes.map(exe => exe.cellHandle));
278-
const myDeletedHandles = new Set<number>();
279-
e.rawEvent.changes.forEach(([start, deleteCount]) => {
280-
if (deleteCount) {
281-
const deletedHandles = this._notebookModel.cells.slice(start, start + deleteCount).map(c => c.handle);
282-
deletedHandles.forEach(h => {
283-
if (handles.has(h)) {
284-
myDeletedHandles.add(h);
285-
}
286-
});
287-
}
288-
289-
return false;
290-
});
284+
const notebookExes = this._notebookExecutionStateService.getCellExecutionsByHandleForNotebook(this._notebookModel.uri);
285+
286+
const executingDeletedHandles = new Set<number>();
287+
const pendingDeletedHandles = new Set<number>();
288+
if (notebookExes) {
289+
e.rawEvent.changes.forEach(([start, deleteCount]) => {
290+
if (deleteCount) {
291+
const deletedHandles = this._notebookModel.cells.slice(start, start + deleteCount).map(c => c.handle);
292+
deletedHandles.forEach(h => {
293+
const exe = notebookExes.get(h);
294+
if (exe?.state === NotebookCellExecutionState.Executing) {
295+
executingDeletedHandles.add(h);
296+
} else if (exe) {
297+
pendingDeletedHandles.add(h);
298+
}
299+
});
300+
}
301+
});
302+
}
291303

292-
if (myDeletedHandles.size) {
293-
this._logService.debug(`NotebookExecution#onWillAddRemoveCells, ${JSON.stringify([...myDeletedHandles])}`);
294-
this._notebookExecutionService.cancelNotebookCellHandles(this._notebookModel, myDeletedHandles);
304+
if (executingDeletedHandles.size || pendingDeletedHandles.size) {
305+
const kernel = this._notebookKernelService.getSelectedOrSuggestedKernel(this._notebookModel);
306+
if (kernel) {
307+
const implementsInterrupt = kernel.implementsInterrupt;
308+
const handlesToCancel = implementsInterrupt ? [...executingDeletedHandles] : [...executingDeletedHandles, ...pendingDeletedHandles];
309+
this._logService.debug(`NotebookExecution#onWillAddRemoveCells, ${JSON.stringify([...handlesToCancel])}`);
310+
kernel.cancelNotebookCellExecution(this._notebookModel.uri, handlesToCancel);
311+
}
295312
}
296313
}
297314
}

‎src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorWidgetContextKeys.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ export class NotebookEditorContextKeys {
136136

137137
private _updateForCellExecution(): void {
138138
if (this._editor.textModel) {
139-
const notebookExe = this._notebookExecutionStateService.getCellExecutionStatesForNotebook(this._editor.textModel.uri);
139+
const notebookExe = this._notebookExecutionStateService.getCellExecutionsForNotebook(this._editor.textModel.uri);
140140
this._someCellRunning.set(notebookExe.length > 0);
141141
} else {
142142
this._someCellRunning.set(false);

‎src/vs/workbench/contrib/notebook/common/notebookExecutionStateService.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,8 @@ export interface INotebookExecutionStateService {
6060
onDidChangeLastRunFailState: Event<INotebookFailStateChangedEvent>;
6161

6262
forceCancelNotebookExecutions(notebookUri: URI): void;
63-
getCellExecutionStatesForNotebook(notebook: URI): INotebookCellExecution[];
63+
getCellExecutionsForNotebook(notebook: URI): INotebookCellExecution[];
64+
getCellExecutionsByHandleForNotebook(notebook: URI): Map<number, INotebookCellExecution> | undefined;
6465
getCellExecution(cellUri: URI): INotebookCellExecution | undefined;
6566
createCellExecution(notebook: URI, cellHandle: number): INotebookCellExecution;
6667
getLastFailedCellForNotebook(notebook: URI): number | undefined;

‎src/vs/workbench/contrib/notebook/test/browser/notebookExecutionStateService.test.ts

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -72,35 +72,56 @@ suite('NotebookExecutionStateService', () => {
7272
return _withTestNotebook(cells, (editor, viewModel) => callback(viewModel, viewModel.notebookDocument));
7373
}
7474

75-
test('cancel execution when cell is deleted', async function () { // TODO@roblou Should be a test for NotebookExecutionListeners, which can be a standalone contribution
75+
function testCancelOnDelete(expectedCancels: number, implementsInterrupt: boolean) {
7676
return withTestNotebook([], async viewModel => {
7777
testNotebookModel = viewModel.notebookDocument;
7878

79-
let didCancel = false;
79+
let cancels = 0;
8080
const kernel = new class extends TestNotebookKernel {
81+
implementsInterrupt = implementsInterrupt;
82+
8183
constructor() {
8284
super({ languages: ['javascript'] });
8385
}
8486

8587
override async executeNotebookCellsRequest(): Promise<void> { }
8688

87-
override async cancelNotebookCellExecution(): Promise<void> {
88-
didCancel = true;
89+
override async cancelNotebookCellExecution(_uri: URI, handles: number[]): Promise<void> {
90+
cancels += handles.length;
8991
}
9092
};
9193
kernelService.registerKernel(kernel);
9294
kernelService.selectKernelForNotebook(kernel, viewModel.notebookDocument);
9395

9496
const executionStateService: INotebookExecutionStateService = instantiationService.get(INotebookExecutionStateService);
9597

98+
// Should cancel executing and pending cells, when kernel does not implement interrupt
9699
const cell = insertCellAtIndex(viewModel, 0, 'var c = 3', 'javascript', CellKind.Code, {}, [], true, true);
97-
executionStateService.createCellExecution(viewModel.uri, cell.handle);
98-
assert.strictEqual(didCancel, false);
100+
const cell2 = insertCellAtIndex(viewModel, 1, 'var c = 3', 'javascript', CellKind.Code, {}, [], true, true);
101+
const cell3 = insertCellAtIndex(viewModel, 2, 'var c = 3', 'javascript', CellKind.Code, {}, [], true, true);
102+
insertCellAtIndex(viewModel, 3, 'var c = 3', 'javascript', CellKind.Code, {}, [], true, true); // Not deleted
103+
const exe = executionStateService.createCellExecution(viewModel.uri, cell.handle); // Executing
104+
exe.confirm();
105+
exe.update([{ editType: CellExecutionUpdateType.ExecutionState, executionOrder: 1 }]);
106+
const exe2 = executionStateService.createCellExecution(viewModel.uri, cell2.handle); // Pending
107+
exe2.confirm();
108+
executionStateService.createCellExecution(viewModel.uri, cell3.handle); // Unconfirmed
109+
assert.strictEqual(cancels, 0);
99110
viewModel.notebookDocument.applyEdits([{
100-
editType: CellEditType.Replace, index: 0, count: 1, cells: []
111+
editType: CellEditType.Replace, index: 0, count: 3, cells: []
101112
}], true, undefined, () => undefined, undefined, false);
102-
assert.strictEqual(didCancel, true);
113+
assert.strictEqual(cancels, expectedCancels);
103114
});
115+
116+
}
117+
118+
// TODO@roblou Could be a test just for NotebookExecutionListeners, which can be a standalone contribution
119+
test('cancel execution when cell is deleted', async function () {
120+
return testCancelOnDelete(3, false);
121+
});
122+
123+
test('cancel execution when cell is deleted in interrupt-type kernel', async function () {
124+
return testCancelOnDelete(1, true);
104125
});
105126

106127
test('fires onDidChangeCellExecution when cell is completed while deleted', async function () {
@@ -219,7 +240,7 @@ class TestNotebookKernel implements INotebookKernel {
219240
preloadProvides: string[] = [];
220241
supportedLanguages: string[] = [];
221242
async executeNotebookCellsRequest(): Promise<void> { }
222-
async cancelNotebookCellExecution(): Promise<void> { }
243+
async cancelNotebookCellExecution(uri: URI, cellHandles: number[]): Promise<void> { }
223244

224245
constructor(opts?: { languages?: string[]; id?: string }) {
225246
this.supportedLanguages = opts?.languages ?? [PLAINTEXT_LANGUAGE_ID];

‎src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -413,11 +413,6 @@ class TestCellExecution implements INotebookCellExecution {
413413
}
414414

415415
class TestNotebookExecutionStateService implements INotebookExecutionStateService {
416-
417-
getLastFailedCellForNotebook(notebook: URI): number | undefined {
418-
return;
419-
}
420-
421416
_serviceBrand: undefined;
422417

423418
private _executions = new ResourceMap<INotebookCellExecution>();
@@ -428,7 +423,7 @@ class TestNotebookExecutionStateService implements INotebookExecutionStateServic
428423
forceCancelNotebookExecutions(notebookUri: URI): void {
429424
}
430425

431-
getCellExecutionStatesForNotebook(notebook: URI): INotebookCellExecution[] {
426+
getCellExecutionsForNotebook(notebook: URI): INotebookCellExecution[] {
432427
return [];
433428
}
434429

@@ -442,4 +437,12 @@ class TestNotebookExecutionStateService implements INotebookExecutionStateServic
442437
this._executions.set(CellUri.generate(notebook, cellHandle), exe);
443438
return exe;
444439
}
440+
441+
getCellExecutionsByHandleForNotebook(notebook: URI): Map<number, INotebookCellExecution> | undefined {
442+
return;
443+
}
444+
445+
getLastFailedCellForNotebook(notebook: URI): number | undefined {
446+
return;
447+
}
445448
}

0 commit comments

Comments
 (0)