Skip to content

Commit cf6f4ab

Browse files
committed
fix: dispose a torn down NB controller's kernel
Fixes #17094 It's my first time looking at any of this code and the lifecycle of everything is a little challenging to discover at a glance. This change passed unit tests and manually verifying, it solves the issue. I'll lay out the problem and solution independently below to make reviewing easier. If a server/kernel is removed, then another was created for the active notebook editor, cell executions against the new kernel would fail. The issue with clear repro steps and the corresponding logs: #17094. The root cause is improper resource cleanup during the disposal of a `VSCodeNotebookController`. When a `JupyterServerProvider` is removed, the extension correctly disposes of the associated controllers. However, the `dispose` method for the controller did not explicitly dispose of the underlying `IKernel` instance that it had created and managed. This left the kernel in an orphaned or "zombie" state. When a new server and controller were subsequently created for the same notebook, it'd try to use the old, still-lingering kernel, which was already in a started state, leading to the "Cannot call start again" error. The fix enhances the dispose method within the `VSCodeNotebookController`. The new logic ensures that when a controller is disposed, it actively cleans up any kernels it was responsible for. It iterates through all currently open NB docs and siposes any kernels who's connection metadata IDs match.
1 parent 4efd36f commit cf6f4ab

File tree

2 files changed

+29
-0
lines changed

2 files changed

+29
-0
lines changed

src/notebooks/controllers/vscodeNotebookController.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,13 @@ export class VSCodeNotebookController implements Disposable, IVSCodeNotebookCont
334334
} called from ${new Error('').stack}`
335335
);
336336
this.isDisposed = true;
337+
// Dispose all kernels associated with this controller
338+
workspace.notebookDocuments.forEach((doc) => {
339+
const kernel = this.kernelProvider.get(doc);
340+
if (kernel?.kernelConnectionMetadata.id === this.connection.id) {
341+
kernel.dispose().catch(noop);
342+
}
343+
});
337344
this._onNotebookControllerSelectionChanged.dispose();
338345
this._onConnecting.dispose();
339346
this.controller.dispose();

src/notebooks/controllers/vscodeNotebookController.unit.test.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -544,4 +544,26 @@ suite(`Notebook Controller`, function () {
544544
});
545545
});
546546
});
547+
test('Disposing controller disposes of the kernel', async function () {
548+
const controller = new VSCodeNotebookController(
549+
instance(kernelConnection),
550+
'1',
551+
'jupyter-notebook',
552+
instance(kernelProvider),
553+
instance(context),
554+
disposables,
555+
instance(languageService),
556+
instance(configService),
557+
instance(extensionChecker),
558+
instance(serviceContainer),
559+
displayDataProvider
560+
);
561+
when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]);
562+
when(kernelProvider.get(notebook)).thenReturn(instance(kernel));
563+
when(kernel.dispose()).thenResolve();
564+
565+
controller.dispose();
566+
567+
verify(kernel.dispose()).once();
568+
});
547569
});

0 commit comments

Comments
 (0)