development/vscode
development/vscode
1
0
Fork 0
mirror of https://github.com/Microsoft/vscode synced 2024-10-30 11:10:48 +00:00
Code Issues Projects Releases Packages Wiki Activity

Don't interrupt when deleting pending cells (#163609)

Browse source 
Fix #163133
This commit is contained in:
Rob Lourens 2022-10-14 00:05:59 -07:00 committed by GitHub
parent 7ef8e6b87a
commit 493d27bf48
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 81 additions and 39 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/vs/workbench/contrib/notebook/browser/contrib/execute/executionEditorProgress.ts
View file

@ -38,7 +38,7 @@ export class ExecutionEditorProgressController extends Disposable implements INo
return;
}
const executing = this._notebookExecutionStateService.getCellExecutionStatesForNotebook(this._notebookEditor.textModel?.uri)
const executing = this._notebookExecutionStateService.getCellExecutionsForNotebook(this._notebookEditor.textModel?.uri)
.filter(exe => exe.state === NotebookCellExecutionState.Executing);
const executionIsVisible = (exe: INotebookCellExecution) => {
for (const range of this._notebookEditor.visibleRanges) {

2
src/vs/workbench/contrib/notebook/browser/controller/executeActions.ts
View file

@ -618,7 +618,7 @@ registerAction2(class RevealRunningCellAction extends NotebookAction {
async runWithContext(accessor: ServicesAccessor, context: INotebookActionContext): Promise<void> {
const notebookExecutionStateService = accessor.get(INotebookExecutionStateService);
const notebook = context.notebookEditor.textModel.uri;
const executingCells = notebookExecutionStateService.getCellExecutionStatesForNotebook(notebook);
const executingCells = notebookExecutionStateService.getCellExecutionsForNotebook(notebook);
if (executingCells[0]) {
const cell = context.notebookEditor.getCellByHandle(executingCells[0].cellHandle);
if (cell) {

57
src/vs/workbench/contrib/notebook/browser/services/notebookExecutionStateServiceImpl.ts
View file

@ -7,6 +7,7 @@ import { Emitter } from 'vs/base/common/event';
import { combinedDisposable, Disposable, IDisposable } from 'vs/base/common/lifecycle';
import { ResourceMap } from 'vs/base/common/map';
import { isEqual } from 'vs/base/common/resources';
import { withNullAsUndefined } from 'vs/base/common/types';
import { URI } from 'vs/base/common/uri';
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
import { ILogService } from 'vs/platform/log/common/log';
@ -14,6 +15,7 @@ import { NotebookTextModel } from 'vs/workbench/contrib/notebook/common/model/no
import { CellEditType, CellUri, ICellEditOperation, NotebookCellExecutionState, NotebookCellInternalMetadata, NotebookTextModelWillAddRemoveEvent } from 'vs/workbench/contrib/notebook/common/notebookCommon';
import { CellExecutionUpdateType, INotebookExecutionService } from 'vs/workbench/contrib/notebook/common/notebookExecutionService';
import { ICellExecuteUpdate, ICellExecutionComplete, ICellExecutionStateChangedEvent, ICellExecutionStateUpdate, IFailedCellInfo, INotebookCellExecution, INotebookExecutionStateService, INotebookFailStateChangedEvent } from 'vs/workbench/contrib/notebook/common/notebookExecutionStateService';
import { INotebookKernelService } from 'vs/workbench/contrib/notebook/common/notebookKernelService';
import { INotebookService } from 'vs/workbench/contrib/notebook/common/notebookService';
export class NotebookExecutionStateService extends Disposable implements INotebookExecutionStateService {
@ -68,11 +70,16 @@ export class NotebookExecutionStateService extends Disposable implements INotebo
return undefined;
}
getCellExecutionStatesForNotebook(notebook: URI): INotebookCellExecution[] {
getCellExecutionsForNotebook(notebook: URI): INotebookCellExecution[] {
const exeMap = this._executions.get(notebook);
return exeMap ? Array.from(exeMap.values()) : [];
}
getCellExecutionsByHandleForNotebook(notebook: URI): Map<number, INotebookCellExecution> | undefined {
const exeMap = this._executions.get(notebook);
return withNullAsUndefined(exeMap);
}
private _onCellExecutionDidChange(notebookUri: URI, cellHandle: number, exe: CellExecution): void {
this._onDidChangeCellExecution.fire(new NotebookExecutionEvent(notebookUri, cellHandle, exe));
}
@ -244,6 +251,7 @@ class NotebookExecutionListeners extends Disposable {
constructor(
notebook: URI,
@INotebookService private readonly _notebookService: INotebookService,
@INotebookKernelService private readonly _notebookKernelService: INotebookKernelService,
@INotebookExecutionService private readonly _notebookExecutionService: INotebookExecutionService,
@INotebookExecutionStateService private readonly _notebookExecutionStateService: INotebookExecutionStateService,
@ILogService private readonly _logService: ILogService,
@ -263,7 +271,7 @@ class NotebookExecutionListeners extends Disposable {
private cancelAll(): void {
this._logService.debug(`NotebookExecutionListeners#cancelAll`);
const exes = this._notebookExecutionStateService.getCellExecutionStatesForNotebook(this._notebookModel.uri);
const exes = this._notebookExecutionStateService.getCellExecutionsForNotebook(this._notebookModel.uri);
this._notebookExecutionService.cancelNotebookCellHandles(this._notebookModel, exes.map(exe => exe.cellHandle));
}
@ -273,25 +281,34 @@ class NotebookExecutionListeners extends Disposable {
}
private onWillAddRemoveCells(e: NotebookTextModelWillAddRemoveEvent): void {
const notebookExes = this._notebookExecutionStateService.getCellExecutionStatesForNotebook(this._notebookModel.uri);
const handles = new Set(notebookExes.map(exe => exe.cellHandle));
const myDeletedHandles = new Set<number>();
e.rawEvent.changes.forEach(([start, deleteCount]) => {
if (deleteCount) {
const deletedHandles = this._notebookModel.cells.slice(start, start + deleteCount).map(c => c.handle);
deletedHandles.forEach(h => {
if (handles.has(h)) {
myDeletedHandles.add(h);
}
});
const notebookExes = this._notebookExecutionStateService.getCellExecutionsByHandleForNotebook(this._notebookModel.uri);
const executingDeletedHandles = new Set<number>();
const pendingDeletedHandles = new Set<number>();
if (notebookExes) {
e.rawEvent.changes.forEach(([start, deleteCount]) => {
if (deleteCount) {
const deletedHandles = this._notebookModel.cells.slice(start, start + deleteCount).map(c => c.handle);
deletedHandles.forEach(h => {
const exe = notebookExes.get(h);
if (exe?.state === NotebookCellExecutionState.Executing) {
executingDeletedHandles.add(h);
} else if (exe) {
pendingDeletedHandles.add(h);
}
});
}
});
}
if (executingDeletedHandles.size || pendingDeletedHandles.size) {
const kernel = this._notebookKernelService.getSelectedOrSuggestedKernel(this._notebookModel);
if (kernel) {
const implementsInterrupt = kernel.implementsInterrupt;
const handlesToCancel = implementsInterrupt ? [...executingDeletedHandles] : [...executingDeletedHandles, ...pendingDeletedHandles];
this._logService.debug(`NotebookExecution#onWillAddRemoveCells, ${JSON.stringify([...handlesToCancel])}`);
kernel.cancelNotebookCellExecution(this._notebookModel.uri, handlesToCancel);
}
return false;
});
if (myDeletedHandles.size) {
this._logService.debug(`NotebookExecution#onWillAddRemoveCells, ${JSON.stringify([...myDeletedHandles])}`);
this._notebookExecutionService.cancelNotebookCellHandles(this._notebookModel, myDeletedHandles);
}
}
}

2
src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorWidgetContextKeys.ts
View file

@ -136,7 +136,7 @@ export class NotebookEditorContextKeys {
private _updateForCellExecution(): void {
if (this._editor.textModel) {
const notebookExe = this._notebookExecutionStateService.getCellExecutionStatesForNotebook(this._editor.textModel.uri);
const notebookExe = this._notebookExecutionStateService.getCellExecutionsForNotebook(this._editor.textModel.uri);
this._someCellRunning.set(notebookExe.length > 0);
} else {
this._someCellRunning.set(false);

3
src/vs/workbench/contrib/notebook/common/notebookExecutionStateService.ts
View file

@ -60,7 +60,8 @@ export interface INotebookExecutionStateService {
onDidChangeLastRunFailState: Event<INotebookFailStateChangedEvent>;
forceCancelNotebookExecutions(notebookUri: URI): void;
getCellExecutionStatesForNotebook(notebook: URI): INotebookCellExecution[];
getCellExecutionsForNotebook(notebook: URI): INotebookCellExecution[];
getCellExecutionsByHandleForNotebook(notebook: URI): Map<number, INotebookCellExecution> | undefined;
getCellExecution(cellUri: URI): INotebookCellExecution | undefined;
createCellExecution(notebook: URI, cellHandle: number): INotebookCellExecution;
getLastFailedCellForNotebook(notebook: URI): number | undefined;

39
src/vs/workbench/contrib/notebook/test/browser/notebookExecutionStateService.test.ts
View file

@ -72,20 +72,22 @@ suite('NotebookExecutionStateService', () => {
return _withTestNotebook(cells, (editor, viewModel) => callback(viewModel, viewModel.notebookDocument));
}
test('cancel execution when cell is deleted', async function () { // TODO@roblou Should be a test for NotebookExecutionListeners, which can be a standalone contribution
function testCancelOnDelete(expectedCancels: number, implementsInterrupt: boolean) {
return withTestNotebook([], async viewModel => {
testNotebookModel = viewModel.notebookDocument;
let didCancel = false;
let cancels = 0;
const kernel = new class extends TestNotebookKernel {
implementsInterrupt = implementsInterrupt;
constructor() {
super({ languages: ['javascript'] });
}
override async executeNotebookCellsRequest(): Promise<void> { }
override async cancelNotebookCellExecution(): Promise<void> {
didCancel = true;
override async cancelNotebookCellExecution(_uri: URI, handles: number[]): Promise<void> {
cancels += handles.length;
}
};
kernelService.registerKernel(kernel);
@ -93,14 +95,33 @@ suite('NotebookExecutionStateService', () => {
const executionStateService: INotebookExecutionStateService = instantiationService.get(INotebookExecutionStateService);
// Should cancel executing and pending cells, when kernel does not implement interrupt
const cell = insertCellAtIndex(viewModel, 0, 'var c = 3', 'javascript', CellKind.Code, {}, [], true, true);
executionStateService.createCellExecution(viewModel.uri, cell.handle);
assert.strictEqual(didCancel, false);
const cell2 = insertCellAtIndex(viewModel, 1, 'var c = 3', 'javascript', CellKind.Code, {}, [], true, true);
const cell3 = insertCellAtIndex(viewModel, 2, 'var c = 3', 'javascript', CellKind.Code, {}, [], true, true);
insertCellAtIndex(viewModel, 3, 'var c = 3', 'javascript', CellKind.Code, {}, [], true, true); // Not deleted
const exe = executionStateService.createCellExecution(viewModel.uri, cell.handle); // Executing
exe.confirm();
exe.update([{ editType: CellExecutionUpdateType.ExecutionState, executionOrder: 1 }]);
const exe2 = executionStateService.createCellExecution(viewModel.uri, cell2.handle); // Pending
exe2.confirm();
executionStateService.createCellExecution(viewModel.uri, cell3.handle); // Unconfirmed
assert.strictEqual(cancels, 0);
viewModel.notebookDocument.applyEdits([{
editType: CellEditType.Replace, index: 0, count: 1, cells: []
editType: CellEditType.Replace, index: 0, count: 3, cells: []
}], true, undefined, () => undefined, undefined, false);
assert.strictEqual(didCancel, true);
assert.strictEqual(cancels, expectedCancels);
});
}
// TODO@roblou Could be a test just for NotebookExecutionListeners, which can be a standalone contribution
test('cancel execution when cell is deleted', async function () {
return testCancelOnDelete(3, false);
});
test('cancel execution when cell is deleted in interrupt-type kernel', async function () {
return testCancelOnDelete(1, true);
});
test('fires onDidChangeCellExecution when cell is completed while deleted', async function () {
@ -219,7 +240,7 @@ class TestNotebookKernel implements INotebookKernel {
preloadProvides: string[] = [];
supportedLanguages: string[] = [];
async executeNotebookCellsRequest(): Promise<void> { }
async cancelNotebookCellExecution(): Promise<void> { }
async cancelNotebookCellExecution(uri: URI, cellHandles: number[]): Promise<void> { }
constructor(opts?: { languages?: string[]; id?: string }) {
this.supportedLanguages = opts?.languages ?? [PLAINTEXT_LANGUAGE_ID];

15
src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts
View file

@ -413,11 +413,6 @@ class TestCellExecution implements INotebookCellExecution {
}
class TestNotebookExecutionStateService implements INotebookExecutionStateService {
getLastFailedCellForNotebook(notebook: URI): number | undefined {
return;
}
_serviceBrand: undefined;
private _executions = new ResourceMap<INotebookCellExecution>();
@ -428,7 +423,7 @@ class TestNotebookExecutionStateService implements INotebookExecutionStateServic
forceCancelNotebookExecutions(notebookUri: URI): void {
}
getCellExecutionStatesForNotebook(notebook: URI): INotebookCellExecution[] {
getCellExecutionsForNotebook(notebook: URI): INotebookCellExecution[] {
return [];
}
@ -442,4 +437,12 @@ class TestNotebookExecutionStateService implements INotebookExecutionStateServic
this._executions.set(CellUri.generate(notebook, cellHandle), exe);
return exe;
}
getCellExecutionsByHandleForNotebook(notebook: URI): Map<number, INotebookCellExecution> | undefined {
return;
}
getLastFailedCellForNotebook(notebook: URI): number | undefined {
return;
}
}