Remove dead code and add additional logging (#16157)

microsoft · Oct 28, 2024 · c399cec · c399cec
1 parent f8493f8
commit c399cec
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 56 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -98,7 +98,6 @@ export interface IControllerRegistration {
      * Event fired when controllers are added or removed
      */
     onDidChange: vscode.Event<IVSCodeNotebookControllerUpdateEvent>;
-    isFiltered(metadata: KernelConnectionMetadata): boolean;
 }
 
 // Flag enum for the reason why a kernel was logged as an exact match