clean up the code.

Artmann · Artmann · commit 4b93ab2aa040 · 2025-11-21T16:55:51.000+01:00
diff --git a/src/kernels/deepnote/deepnoteLspClientManager.node.ts b/src/kernels/deepnote/deepnoteLspClientManager.node.ts
@@ -1,9 +1,7 @@
-// Copyright (c) Microsoft Corporation.
-// Licensed under the MIT License.
-
 import * as vscode from 'vscode';
 import { inject, injectable } from 'inversify';
 import { LanguageClient, LanguageClientOptions, Executable } from 'vscode-languageclient/node';
+
 import { IDisposable, IDisposableRegistry } from '../../platform/common/types';
 import { IExtensionSyncActivationService } from '../../platform/activation/types';
 import { DeepnoteServerInfo, IDeepnoteLspClientManager } from './types';
@@ -24,43 +22,67 @@ interface LspClientInfo {
 export class DeepnoteLspClientManager
     implements IDeepnoteLspClientManager, IExtensionSyncActivationService, IDisposable
 {
-    // Map notebook URIs to their LSP clients
     private readonly clients = new Map<string, LspClientInfo>();
+    private readonly pendingStarts = new Map<string, boolean>();
+
     private disposed = false;
 
     constructor(@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry) {
         this.disposables.push(this);
     }
 
     public activate(): void {
-        // This service is activated synchronously and doesn't need async initialization
         logger.info('DeepnoteLspClientManager activated');
     }
 
     public async startLspClients(
         _serverInfo: DeepnoteServerInfo,
         notebookUri: vscode.Uri,
-        interpreter: PythonEnvironment
+        interpreter: PythonEnvironment,
+        token?: vscode.CancellationToken
     ): Promise<void> {
         if (this.disposed) {
             return;
         }
 
+        // Check for cancellation before starting
+        if (token?.isCancellationRequested) {
+            return;
+        }
+
         const notebookKey = notebookUri.toString();
 
-        // Check if clients already exist for this notebook
+        const pendingStart = this.pendingStarts.get(notebookKey);
+
+        if (pendingStart) {
+            logger.trace(`LSP client is already starting up for ${notebookKey}.`);
+
+            return;
+        }
+
         if (this.clients.has(notebookKey)) {
-            logger.trace(`LSP clients already started for ${notebookKey}`);
+            logger.trace(`LSP clients already started for ${notebookKey}.`);
+
             return;
         }
 
-        logger.info(`Starting LSP clients for ${notebookKey} using interpreter ${interpreter.uri.fsPath}`);
+        logger.info(`Starting LSP clients for ${notebookKey} using interpreter ${interpreter.uri.fsPath}.`);
+
+        this.pendingStarts.set(notebookKey, true);
 
         try {
-            // Start Python LSP client
-            const pythonClient = await this.createPythonLspClient(notebookUri, interpreter);
+            // Check cancellation before expensive operation
+            if (token?.isCancellationRequested) {
+                return;
+            }
+
+            const pythonClient = await this.createPythonLspClient(notebookUri, interpreter, token);
+
+            // Check cancellation after client creation
+            if (token?.isCancellationRequested) {
+                return;
+            }
 
-            // Store the client info
             const clientInfo: LspClientInfo = {
                 pythonClient
                 // TODO: Add SQL client when endpoint is determined
@@ -71,48 +93,76 @@ export class DeepnoteLspClientManager
             logger.info(`LSP clients started successfully for ${notebookKey}`);
         } catch (error) {
             logger.error(`Failed to start LSP clients for ${notebookKey}:`, error);
+
             throw error;
+        } finally {
+            this.pendingStarts.delete(notebookKey);
         }
     }
 
-    public async stopLspClients(notebookUri: vscode.Uri): Promise<void> {
+    public async stopLspClients(notebookUri: vscode.Uri, token?: vscode.CancellationToken): Promise<void> {
         const notebookKey = notebookUri.toString();
         const clientInfo = this.clients.get(notebookKey);
 
         if (!clientInfo) {
             return;
         }
 
+        // Check cancellation before stopping
+        if (token?.isCancellationRequested) {
+            return;
+        }
+
         logger.info(`Stopping LSP clients for ${notebookKey}`);
 
         try {
-            // Stop Python client
             if (clientInfo.pythonClient) {
+                if (token?.isCancellationRequested) {
+                    return;
+                }
                 await clientInfo.pythonClient.stop();
+                await clientInfo.pythonClient.dispose();
             }
 
-            // Stop SQL client
             if (clientInfo.sqlClient) {
+                if (token?.isCancellationRequested) {
+                    return;
+                }
                 await clientInfo.sqlClient.stop();
+                await clientInfo.sqlClient.dispose();
             }
 
             this.clients.delete(notebookKey);
+
             logger.info(`LSP clients stopped for ${notebookKey}`);
         } catch (error) {
             logger.error(`Error stopping LSP clients for ${notebookKey}:`, error);
         }
     }
 
-    public async stopAllClients(): Promise<void> {
+    public async stopAllClients(token?: vscode.CancellationToken): Promise<void> {
+        // Check cancellation before stopping
+        if (token?.isCancellationRequested) {
+            return;
+        }
+
         logger.info('Stopping all LSP clients');
 
         const stopPromises: Promise<void>[] = [];
         for (const [, clientInfo] of this.clients.entries()) {
+            // Check cancellation during iteration
+            if (token?.isCancellationRequested) {
+                break;
+            }
+
             if (clientInfo.pythonClient) {
                 stopPromises.push(clientInfo.pythonClient.stop().catch(noop));
+                stopPromises.push(clientInfo.pythonClient.dispose().catch(noop));
             }
+
             if (clientInfo.sqlClient) {
                 stopPromises.push(clientInfo.sqlClient.stop().catch(noop));
+                stopPromises.push(clientInfo.sqlClient.dispose().catch(noop));
             }
         }
 
@@ -122,20 +172,24 @@ export class DeepnoteLspClientManager
 
     public dispose(): void {
         this.disposed = true;
-        // Stop all clients asynchronously but don't wait
+
         void this.stopAllClients();
     }
 
     private async createPythonLspClient(
         notebookUri: vscode.Uri,
-        interpreter: PythonEnvironment
+        interpreter: PythonEnvironment,
+        token?: vscode.CancellationToken
     ): Promise<LanguageClient> {
-        // Start python-lsp-server as a child process using stdio
+        // Check cancellation before creating client
+        if (token?.isCancellationRequested) {
+            throw new Error('Operation cancelled');
+        }
+
         const pythonPath = interpreter.uri.fsPath;
 
         logger.trace(`Creating Python LSP client using interpreter: ${pythonPath}`);
 
-        // Define the server executable
         const serverOptions: Executable = {
             command: pythonPath,
             args: ['-m', 'pylsp'], // Start python-lsp-server
@@ -145,7 +199,6 @@ export class DeepnoteLspClientManager
         };
 
         const clientOptions: LanguageClientOptions = {
-            // Document selector for Python cells in Deepnote notebooks
             documentSelector: [
                 {
                     scheme: 'vscode-notebook-cell',
@@ -158,25 +211,26 @@ export class DeepnoteLspClientManager
                     pattern: '**/*.deepnote'
                 }
             ],
-            // Synchronization settings
             synchronize: {
-                // Notify the server about file changes to '.py' files in the workspace
-                fileEvents: vscode.workspace.createFileSystemWatcher('**/*.py')
+                fileEvents: vscode.workspace.createFileSystemWatcher('**/*.{py,deepnote}')
             },
-            // Output channel for diagnostics
             outputChannelName: 'Deepnote Python LSP'
         };
 
-        // Create the language client with stdio connection
         const client = new LanguageClient(
             'deepnote-python-lsp',
             'Deepnote Python Language Server',
             serverOptions,
             clientOptions
         );
 
-        // Start the client
+        // Check cancellation before starting client
+        if (token?.isCancellationRequested) {
+            throw new Error('Operation cancelled');
+        }
+
         await client.start();
+
         logger.info(`Python LSP client started for ${notebookUri.toString()}`);
 
         return client;
diff --git a/src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts b/src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts
@@ -44,10 +44,11 @@ suite('DeepnoteLspClientManager Integration Tests', () => {
     test('Should handle starting LSP clients with mock interpreter', async function () {
         this.timeout(10000);
 
-        // Create a mock interpreter (pointing to system Python for test)
+        // Use invalid path to deliberately trigger failure and test error handling
+        // This ensures consistent test behavior across platforms (simulates missing pylsp)
         const mockInterpreter: PythonEnvironment = {
-            id: '/usr/bin/python3',
-            uri: Uri.file('/usr/bin/python3')
+            id: '/nonexistent/path/to/python',
+            uri: Uri.file('/nonexistent/path/to/python')
         } as PythonEnvironment;
 
         const mockServerInfo = {
@@ -94,9 +95,11 @@ suite('DeepnoteLspClientManager Integration Tests', () => {
     test('Should not start duplicate clients for same notebook', async function () {
         this.timeout(10000);
 
+        // Use invalid path to deliberately trigger failure and test error handling
+        // This ensures consistent test behavior across platforms (simulates missing pylsp)
         const mockInterpreter: PythonEnvironment = {
-            id: '/usr/bin/python3',
-            uri: Uri.file('/usr/bin/python3')
+            id: '/nonexistent/path/to/python',
+            uri: Uri.file('/nonexistent/path/to/python')
         } as PythonEnvironment;
 
         const mockServerInfo = {
diff --git a/src/kernels/deepnote/types.ts b/src/kernels/deepnote/types.ts
@@ -324,23 +324,27 @@ export interface IDeepnoteLspClientManager {
      * @param serverInfo Server information
      * @param notebookUri The notebook URI for which to start LSP clients
      * @param interpreter The Python interpreter from the venv
+     * @param token Optional cancellation token to cancel the operation
      */
     startLspClients(
         serverInfo: DeepnoteServerInfo,
         notebookUri: vscode.Uri,
-        interpreter: PythonEnvironment
+        interpreter: PythonEnvironment,
+        token?: vscode.CancellationToken
     ): Promise<void>;
 
     /**
      * Stop LSP clients for a notebook
      * @param notebookUri The notebook URI
+     * @param token Optional cancellation token to cancel the operation
      */
-    stopLspClients(notebookUri: vscode.Uri): Promise<void>;
+    stopLspClients(notebookUri: vscode.Uri, token?: vscode.CancellationToken): Promise<void>;
 
     /**
      * Stop all LSP clients
+     * @param token Optional cancellation token to cancel the operation
      */
-    stopAllClients(): Promise<void>;
+    stopAllClients(token?: vscode.CancellationToken): Promise<void>;
 }
 
 export const DEEPNOTE_TOOLKIT_VERSION = '1.1.0';
diff --git a/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts b/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
@@ -568,28 +568,23 @@ export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector,
         this.serverProvider.registerServer(serverProviderHandle.handle, serverInfo);
         this.projectServerHandles.set(projectKey, serverProviderHandle.handle);
 
-        // Get the venv interpreter for LSP
-        const lspInterpreterUri =
-            process.platform === 'win32'
-                ? Uri.joinPath(configuration.venvPath, 'Scripts', 'python.exe')
-                : Uri.joinPath(configuration.venvPath, 'bin', 'python');
+        const lspInterpreterUri = this.getVenvInterpreterUri(configuration.venvPath);
 
         const lspInterpreter: PythonEnvironment = {
             uri: lspInterpreterUri,
             id: lspInterpreterUri.fsPath
         } as PythonEnvironment;
 
-        // Start LSP clients for code intelligence
         try {
             await this.lspClientManager.startLspClients(serverInfo, notebook.uri, lspInterpreter);
+
             logger.info(`✓ LSP clients started for ${notebookKey}`);
         } catch (error) {
             logger.error(`Failed to start LSP clients for ${notebookKey}:`, error);
-            // Don't fail the kernel selection if LSP fails - it's supplementary
         }
 
-        // Connect to the server and get available kernel specs
         progress.report({ message: 'Connecting to kernel...' });
+
         const connectionInfo = createJupyterConnectionInfo(
             serverProviderHandle,
             {
@@ -620,11 +615,7 @@ export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector,
 
         progress.report({ message: 'Finalizing kernel setup...' });
 
-        // Get the venv Python interpreter (not the base interpreter)
-        const venvInterpreter =
-            process.platform === 'win32'
-                ? Uri.joinPath(configuration.venvPath, 'Scripts', 'python.exe')
-                : Uri.joinPath(configuration.venvPath, 'bin', 'python');
+        const venvInterpreter = this.getVenvInterpreterUri(configuration.venvPath);
 
         logger.info(`Using venv path: ${configuration.venvPath.fsPath}`);
         logger.info(`Venv interpreter path: ${venvInterpreter.fsPath}`);
@@ -803,6 +794,12 @@ export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector,
         }
     }
 
+    private getVenvInterpreterUri(venvPath: Uri): Uri {
+        return process.platform === 'win32'
+            ? Uri.joinPath(venvPath, 'Scripts', 'python.exe')
+            : Uri.joinPath(venvPath, 'bin', 'python');
+    }
+
     /**
      * Handle kernel selection errors with user-friendly messages and actions
      */
diff --git a/src/test/datascience/.vscode/settings.json b/src/test/datascience/.vscode/settings.json
@@ -30,7 +30,7 @@
     "python.experiments.enabled": true,
     // See https://github.com/microsoft/vscode-jupyter/issues/10258
     "deepnote.forceIPyKernelDebugger": false,
-    "python.defaultInterpreterPath": "/Users/artmann/deepnote/vscode-deepnote/.venv/bin/python",
+    "python.defaultInterpreterPath": "${workspaceFolder}/.venv/bin/python",
     "task.problemMatchers.neverPrompt": {
         "shell": true,
         "npm": true
