Notebook cell execution isn't managed in the renderer

[jrieken]

Notebook execution is stored in "internal metadata" That's fine but the problem is this source of truth is held in the extension host and not in the renderer. You can run into all kinds of trouble, e.g

• while a cell executes, select "F1 > restart extension host"
• execute the same cell from different extension host
• have a bad extension that never end an execution

Instead there should be one central place, maybe INotebookKernelService or a new service, that owns the truth about all executions and "projects" that truth onto internal metadata. The mainThreadXYZ parts should interact with that service and handle things like restart ext host etc

[yash112-lang]

hello @jrieken, is this issue is still open, I am new to open source and wanted to contribute.

[roblourens]

@yash112-lang, thanks, this isn't a good issue to start with

@jrieken I don't really see the problem with the current state because to me the real source of truth is the kernel, and in vscode this is reflected in internalMetadata in the renderer process. The execution object just emits edits and isn't a source of truth. If the EH goes down, won't we totally refresh the notebook and call the serializer/provider again?

have a bad extension that never end an execution

If the extension doesn't tell us the execution is done, I don't think we can override this.

[yash112-lang]

@roblourens, can you please suggest me some good issues to start with.

[roblourens]

https://github.com/microsoft/vscode/wiki/How-to-Contribute

[jrieken]

The execution object just emits edits and isn't a source of truth. If the EH goes down, won't we totally refresh the notebook and call the serializer/provider again?

Depends - if one extension host goes down, other extension hosts are unaffected. Therefore extension host can never store truth. And you do that, e.g by checking in the extension host that an execution already exists.

@jrieken I don't really see the problem with the current state because to me the real source of truth is the kernel,

That's only one dimension of it. Take the following scenario

• select kernel A
• select run
• nothing happens, the cell is in pending forever
• select kernel B (because A is obviously broken)
• the cell is forever stuck in pending

[roblourens]

One problem with

vscode/src/vs/workbench/api/common/extHostNotebookKernels.ts

Line 303 in d0cc521

if (this._activeExecutions.has(cellObj.uri)) {

is that we decided that createNotebookCellExecution can't be async and go to the renderer for permission to create an execution, because the kernel is the real source of truth about what cells are actually executing, and you can have a race between clicking run and the cell already being executed through another route inside the kernel. That makes the situation weird.

select kernel B (because A is obviously broken)

I guess we need to reset all the execution states in internalMetadata when changing kernels. We need to keep track of per-kernel executions on the renderer and clear them out when changing the kernel or the EH goes down. I guess that is basically what you propose, one place to do that with more info than exists in internalMetadata. But at the same time we probably need to still be able to throw synchronously in the EH if one controller calls createNotebookCellExecution twice in a row.

[jrieken]

is that we decided that createNotebookCellExecution can't be async and go to the renderer for permission to create an execution,

That's the easy path of blaming the API rules but the truth is that this isn't much different from: user presses run, execution is created, user presses cancel (all of that happening insanely fast). In that case the execution needs to transition into a canceled/discarded state and ignore further updates etc. Now, a duplicate execution would act exactly like that.