From c399cec9f9c1133c101779b862caabd6c9b9589b Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 28 Oct 2024 13:15:23 +1100 Subject: [PATCH] Remove dead code and add additional logging (#16157) --- src/kernels/kernelProvider.base.ts | 8 +-- src/kernels/kernelProvider.node.ts | 11 +++- src/kernels/kernelProvider.web.ts | 11 +++- .../controllers/controllerRegistration.ts | 53 ++----------------- src/notebooks/controllers/types.ts | 1 - 5 files changed, 28 insertions(+), 56 deletions(-) diff --git a/src/kernels/kernelProvider.base.ts b/src/kernels/kernelProvider.base.ts index 67705529d20..4c60412f704 100644 --- a/src/kernels/kernelProvider.base.ts +++ b/src/kernels/kernelProvider.base.ts @@ -52,7 +52,7 @@ export abstract class BaseCoreKernelProvider implements IKernelProvider { protected disposables: IDisposableRegistry ) { this.asyncDisposables.push(this); - workspace.onDidCloseNotebookDocument((e) => this.disposeOldKernel(e), this, disposables); + workspace.onDidCloseNotebookDocument((e) => this.disposeOldKernel(e, 'notebookClosed'), this, disposables); disposables.push(this._onDidDisposeKernel); disposables.push(this._onDidRestartKernel); disposables.push(this._onKernelStatusChanged); @@ -133,11 +133,13 @@ export abstract class BaseCoreKernelProvider implements IKernelProvider { this.disposables ); } - protected disposeOldKernel(notebook: NotebookDocument) { + protected disposeOldKernel(notebook: NotebookDocument, reason: 'notebookClosed' | 'createNewKernel') { const kernelToDispose = this.kernelsByNotebook.get(notebook); if (kernelToDispose) { logger.debug( - `Disposing kernel associated with ${getDisplayPath(notebook.uri)}, isClosed=${notebook.isClosed}` + `Disposing kernel associated with ${getDisplayPath(notebook.uri)}, isClosed=${ + notebook.isClosed + }, reason = ${reason}` ); this.kernelsById.delete(kernelToDispose.kernel.id); this.pendingDisposables.add(kernelToDispose.kernel); diff --git a/src/kernels/kernelProvider.node.ts b/src/kernels/kernelProvider.node.ts index 7b6849ef9e3..0dd08513ba8 100644 --- a/src/kernels/kernelProvider.node.ts +++ b/src/kernels/kernelProvider.node.ts @@ -27,6 +27,8 @@ import { IJupyterServerUriStorage } from './jupyter/types'; import { createKernelSettings } from './kernelSettings'; import { NotebookKernelExecution } from './kernelExecution'; import { IReplNotebookTrackerService } from '../platform/notebooks/replNotebookTrackerService'; +import { logger } from '../platform/logging'; +import { getDisplayPath } from '../platform/common/platform/fs-paths'; /** * Node version of a kernel provider. Needed in order to create the node version of a kernel. @@ -55,7 +57,14 @@ export class KernelProvider extends BaseCoreKernelProvider { if (existingKernelInfo && existingKernelInfo.options.metadata.id === options.metadata.id) { return existingKernelInfo.kernel; } - this.disposeOldKernel(notebook); + if (existingKernelInfo) { + logger.trace( + `Kernel for ${getDisplayPath(notebook.uri)} with id ${ + existingKernelInfo.options.metadata.id + } is being replaced with ${options.metadata.id}` + ); + } + this.disposeOldKernel(notebook, 'createNewKernel'); const replKernel = this.replTracker.isForReplEditor(notebook); const resourceUri = replKernel ? options.resourceUri : notebook.uri; diff --git a/src/kernels/kernelProvider.web.ts b/src/kernels/kernelProvider.web.ts index 13eef94d370..299ec64b4ec 100644 --- a/src/kernels/kernelProvider.web.ts +++ b/src/kernels/kernelProvider.web.ts @@ -26,6 +26,8 @@ import { import { IJupyterServerUriStorage } from './jupyter/types'; import { createKernelSettings } from './kernelSettings'; import { NotebookKernelExecution } from './kernelExecution'; +import { getDisplayPath } from '../platform/common/platform/fs-paths'; +import { logger } from '../platform/logging'; /** * Web version of a kernel provider. Needed in order to create the web version of a kernel. @@ -52,7 +54,14 @@ export class KernelProvider extends BaseCoreKernelProvider { if (existingKernelInfo && existingKernelInfo.options.metadata.id === options.metadata.id) { return existingKernelInfo.kernel; } - this.disposeOldKernel(notebook); + if (existingKernelInfo) { + logger.trace( + `Kernel for ${getDisplayPath(notebook.uri)} with id ${ + existingKernelInfo.options.metadata.id + } is being replaced with ${options.metadata.id}` + ); + } + this.disposeOldKernel(notebook, 'createNewKernel'); const resourceUri = notebook?.notebookType === InteractiveWindowView ? options.resourceUri : notebook.uri; const settings = createKernelSettings(this.configService, resourceUri); diff --git a/src/notebooks/controllers/controllerRegistration.ts b/src/notebooks/controllers/controllerRegistration.ts index 8e90bf650f0..b50367179c2 100644 --- a/src/notebooks/controllers/controllerRegistration.ts +++ b/src/notebooks/controllers/controllerRegistration.ts @@ -95,32 +95,6 @@ export class ControllerRegistration implements IControllerRegistration, IExtensi this.serverUriStorage.onDidChange(this.onDidChangeFilter, this, this.disposables); this.serverUriStorage.onDidChange(this.onDidChangeUri, this, this.disposables); this.serverUriStorage.onDidRemove(this.onDidRemoveServers, this, this.disposables); - - this.onDidChange( - ({ added }) => { - added.forEach((controller) => { - controller.onNotebookControllerSelectionChanged( - (e) => { - if ( - !e.selected && - this.isFiltered(controller.connection) && - this.canControllerBeDisposed(controller) - ) { - // This item was selected but is no longer allowed in the kernel list. Remove it - logger.warn( - `Removing controller ${controller.id} for ${controller.connection.kind} from kernel list` - ); - controller.dispose(); - } - }, - this, - this.disposables - ); - }); - }, - this, - this.disposables - ); this.loadControllers(); } private loadControllers() { @@ -239,24 +213,10 @@ export class ControllerRegistration implements IControllerRegistration, IExtensi private onDidChangeFilter() { // Give our list of metadata should be up to date, just remove the filtered ones - const metadatas = this.all.filter((item) => !this.isFiltered(item)); + const metadatas = this.all; // Try to re-create the missing controllers. metadatas.forEach((c) => this.addOrUpdate(c, [JupyterNotebookView, InteractiveWindowView])); - - // Go through all controllers that have been created and hide them. - // Unless they are attached to an existing document. - this.registered.forEach((item) => { - // TODO: Don't hide controllers that are already associated with a notebook. - // If we have a notebook opened and its using a kernel. - // Else we end up killing the execution as well. - if (this.isFiltered(item.connection) && this.canControllerBeDisposed(item)) { - logger.warn( - `Deleting controller ${item.id} as it is associated with a connection that has been hidden` - ); - item.dispose(); - } - }); } private batchAdd(metadatas: KernelConnectionMetadata[], types: ('jupyter-notebook' | 'interactive')[]) { const addedList: IVSCodeNotebookController[] = []; @@ -324,10 +284,6 @@ export class ControllerRegistration implements IControllerRegistration, IExtensi `Found existing controller '${controller.id}', not creating a new one just updating it` ); return false; - } else if (this.isFiltered(metadata)) { - // Filter out those in our kernel filter - logger.ci(`Existing controller '${id}' will be excluded as it is filtered`); - return false; } logger.ci(`Existing controller not found for '${id}', hence creating a new one`); return true; @@ -351,7 +307,7 @@ export class ControllerRegistration implements IControllerRegistration, IExtensi const controllerDisposables: IDisposable[] = []; controller.onDidDispose( () => { - logger.ci( + logger.trace( `Deleting controller '${controller.id}' associated with view ${viewType} from registration as it was disposed` ); this.registeredControllers.delete(controller.id); @@ -390,6 +346,7 @@ export class ControllerRegistration implements IControllerRegistration, IExtensi if (isCancellationError(ex, true)) { // This can happen in the tests, and these get bubbled upto VSC and are logged as unhandled exceptions. // Hence swallow cancellation errors. + logger.warn(`Cancel creation of notebook controller for ${metadata.id}`, ex); return { added, existing }; } logger.error(`Failed to create notebook controller for ${metadata.id}`, ex); @@ -404,10 +361,6 @@ export class ControllerRegistration implements IControllerRegistration, IExtensi return this.registeredControllers.get(id); } - public isFiltered(_metadata: KernelConnectionMetadata): boolean { - return false; - } - private getControllerId( metadata: KernelConnectionMetadata, viewType: typeof JupyterNotebookView | typeof InteractiveWindowView diff --git a/src/notebooks/controllers/types.ts b/src/notebooks/controllers/types.ts index eb6b2263115..a5892bc1956 100644 --- a/src/notebooks/controllers/types.ts +++ b/src/notebooks/controllers/types.ts @@ -98,7 +98,6 @@ export interface IControllerRegistration { * Event fired when controllers are added or removed */ onDidChange: vscode.Event; - isFiltered(metadata: KernelConnectionMetadata): boolean; } // Flag enum for the reason why a kernel was logged as an exact match