Connect to raw kernel with command line args instead of connection file (#11894)

Author: IanMatthewHuff

.vscode/launch.json

@@ -271,7 +271,7 @@
             ],
             "env": {
                 // Remove `X` prefix to test with real browser to host DS ui (for DS functional tests).
-                "xVSC_PYTHON_DS_UI_BROWSER": "1",
+                "XVSC_PYTHON_DS_UI_BROWSER": "1",
                 // Remove `X` prefix to test with real python (for DS functional tests).
                 "XVSCODE_PYTHON_ROLLING": "1",
                 // Remove 'X' to turn on all logging in the debug output

news/2 Fixes/11883.md

@@ -0,0 +1 @@
+Use command line arguments to launch our raw kernels as opposed to a connection file. The connection file seems to be causing issues in particular on windows CI machines with permissions.

src/client/datascience/kernel-launcher/kernelLauncher.ts

@@ -6,7 +6,6 @@ import { inject, injectable } from 'inversify';
 import * as portfinder from 'portfinder';
 import { promisify } from 'util';
 import * as uuid from 'uuid/v4';
-import { IFileSystem } from '../../common/platform/types';
 import { IProcessServiceFactory } from '../../common/process/types';
 import { Resource } from '../../common/types';
 import { PythonInterpreter } from '../../interpreter/contracts';
@@ -27,7 +26,6 @@ export class KernelLauncher implements IKernelLauncher {
     private static nextFreePortToTryAndUse = PortToStartFrom;
     constructor(
         @inject(IProcessServiceFactory) private processExecutionFactory: IProcessServiceFactory,
-        @inject(IFileSystem) private file: IFileSystem,
         @inject(KernelDaemonPool) private readonly daemonPool: KernelDaemonPool
     ) {}
 
@@ -40,7 +38,6 @@ export class KernelLauncher implements IKernelLauncher {
         const connection = await this.getKernelConnection();
         const kernelProcess = new KernelProcess(
             this.processExecutionFactory,
-            this.file,
             this.daemonPool,
             connection,
             kernelSpec,

src/client/datascience/kernel-launcher/kernelProcess.ts

@@ -7,7 +7,6 @@ import * as tcpPortUsed from 'tcp-port-used';
 import { Event, EventEmitter } from 'vscode';
 import { PYTHON_LANGUAGE } from '../../common/constants';
 import { traceError, traceInfo, traceWarning } from '../../common/logger';
-import { IFileSystem, TemporaryFile } from '../../common/platform/types';
 import { IProcessServiceFactory, ObservableExecutionResult } from '../../common/process/types';
 import { Resource } from '../../common/types';
 import { noop, swallowExceptions } from '../../common/utils/misc';
@@ -39,7 +38,6 @@ export class KernelProcess implements IKernelProcess {
         return this.kernelSpec.language.toLowerCase() === PYTHON_LANGUAGE.toLowerCase();
     }
     private _process?: ChildProcess;
-    private connectionFile?: TemporaryFile;
     private exitEvent = new EventEmitter<{ exitCode?: number; reason?: string }>();
     private pythonKernelLauncher?: PythonKernelLauncherDaemon;
     private launchedOnce?: boolean;
@@ -49,7 +47,6 @@ export class KernelProcess implements IKernelProcess {
     private readonly originalKernelSpec: IJupyterKernelSpec;
     constructor(
         private readonly processExecutionFactory: IProcessServiceFactory,
-        private readonly file: IFileSystem,
         private readonly daemonPool: KernelDaemonPool,
         private readonly _connection: IKernelConnection,
         kernelSpec: IJupyterKernelSpec,
@@ -72,7 +69,8 @@ export class KernelProcess implements IKernelProcess {
         }
         this.launchedOnce = true;
 
-        await this.createAndUpdateConnectionFile();
+        // Update our connection arguments in the kernel spec
+        this.updateConnectionArgs();
 
         const exeObs = await this.launchAsObservable();
 
@@ -127,7 +125,6 @@ export class KernelProcess implements IKernelProcess {
             this.exitEvent.fire();
         });
         swallowExceptions(() => this.pythonKernelLauncher?.dispose());
-        swallowExceptions(() => this.connectionFile?.dispose());
     }
 
     // Make sure that the heartbeat channel is open for connections
@@ -144,21 +141,42 @@ export class KernelProcess implements IKernelProcess {
         }
     }
 
-    private async createAndUpdateConnectionFile() {
-        // Create the connection file so that any user can destroy it. The kernel will
-        // actually read it, delete it, and recreate it with more restricted permissions
-        this.connectionFile = await this.file.createTemporaryFile('.json', 0o777);
-        await this.file.writeFile(this.connectionFile.filePath, JSON.stringify(this._connection), {
-            encoding: 'utf-8',
-            flag: 'w'
-        });
-
-        // Update the args in the kernelspec to include the conenction file.
+    // Instead of having to use a connection file update our local copy of the kernelspec to launch
+    // directly with command line arguments
+    private updateConnectionArgs() {
+        // First check to see if we have a kernelspec that expects a connection file,
+        // Error if we don't have one. We expect '-f', '{connectionfile}' in our launch args
         const indexOfConnectionFile = findIndexOfConnectionFile(this._kernelSpec);
-        if (indexOfConnectionFile === -1) {
+        if (
+            indexOfConnectionFile === -1 ||
+            indexOfConnectionFile === 0 ||
+            this._kernelSpec.argv[indexOfConnectionFile - 1] !== '-f'
+        ) {
             throw new Error(`Connection file not found in kernelspec json args, ${this._kernelSpec.argv.join(' ')}`);
         }
-        this._kernelSpec.argv[indexOfConnectionFile] = this.connectionFile.filePath;
+
+        // Slice out -f and the connection file from the args
+        this._kernelSpec.argv.splice(indexOfConnectionFile - 1, 2);
+
+        // Add in our connection command line args
+        this._kernelSpec.argv.push(...this.addConnectionArgs());
+    }
+
+    // Add the command line arguments
+    private addConnectionArgs(): string[] {
+        const newConnectionArgs: string[] = [];
+
+        newConnectionArgs.push(`--ip=${this._connection.ip}`);
+        newConnectionArgs.push(`--stdin=${this._connection.stdin_port}`);
+        newConnectionArgs.push(`--control=${this._connection.control_port}`);
+        newConnectionArgs.push(`--hb=${this._connection.hb_port}`);
+        newConnectionArgs.push(`--Session.signature_scheme="${this._connection.signature_scheme}"`);
+        newConnectionArgs.push(`--Session.key=b"${this._connection.key}"`); // Note we need the 'b here at the start for a byte string
+        newConnectionArgs.push(`--shell=${this._connection.shell_port}`);
+        newConnectionArgs.push(`--transport="${this._connection.transport}"`);
+        newConnectionArgs.push(`--iopub=${this._connection.iopub_port}`);
+
+        return newConnectionArgs;
     }
 
     private async launchAsObservable() {

src/test/datascience/kernelLauncher.functional.test.ts

@@ -6,7 +6,6 @@ import { assert } from 'chai';
 
 import { KernelMessage } from '@jupyterlab/services';
 import * as uuid from 'uuid/v4';
-import { IFileSystem } from '../../client/common/platform/types';
 import { IProcessServiceFactory } from '../../client/common/process/types';
 import { createDeferred } from '../../client/common/utils/async';
 import { JupyterZMQBinariesNotFoundError } from '../../client/datascience/jupyter/jupyterZMQBinariesNotFoundError';
@@ -39,10 +38,9 @@ suite('DataScience - Kernel Launcher', () => {
         ioc = new DataScienceIocContainer();
         ioc.registerDataScienceTypes();
         kernelFinder = new MockKernelFinder(ioc.serviceContainer.get<IKernelFinder>(IKernelFinder));
-        const file = ioc.serviceContainer.get<IFileSystem>(IFileSystem);
         const processServiceFactory = ioc.serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
         const daemonPool = ioc.serviceContainer.get<KernelDaemonPool>(KernelDaemonPool);
-        kernelLauncher = new KernelLauncher(processServiceFactory, file, daemonPool);
+        kernelLauncher = new KernelLauncher(processServiceFactory, daemonPool);
         await ioc.activate();
         if (!ioc.mockJupyter) {
             pythonInterpreter = await ioc.getJupyterCapableInterpreter();

src/test/datascience/raw-kernel/rawKernel.functional.test.ts

@@ -2,12 +2,8 @@
 // Licensed under the MIT License.
 'use strict';
 import { assert } from 'chai';
-import * as fs from 'fs-extra';
 import { noop } from 'jquery';
-import * as os from 'os';
-import * as path from 'path';
 import * as uuid from 'uuid/v4';
-import { IFileSystem } from '../../../client/common/platform/types';
 import { IProcessServiceFactory } from '../../../client/common/process/types';
 import { createDeferred, sleep } from '../../../client/common/utils/async';
 import { KernelDaemonPool } from '../../../client/datascience/kernel-launcher/kernelDaemonPool';
@@ -21,7 +17,6 @@ import { requestExecute, requestInspect } from './rawKernelTestHelpers';
 // tslint:disable:no-any no-multiline-string max-func-body-length no-console max-classes-per-file trailing-comma
 suite('DataScience raw kernel tests', () => {
     let ioc: DataScienceIocContainer;
-    let connectionFile: string;
     let rawKernel: RawKernel;
     const connectionInfo = {
         shell_port: 57718,
@@ -67,8 +62,6 @@ suite('DataScience raw kernel tests', () => {
             .getInterpreterDetails(ioc.getSettings().pythonPath);
         assert.ok(interpreter, 'No jupyter interpreter found');
         // Start our kernel
-        connectionFile = path.join(os.tmpdir(), `tmp_${Date.now()}_k.json`);
-        await fs.writeFile(connectionFile, JSON.stringify(connectionInfo), { encoding: 'utf-8', flag: 'w' });
         const kernelSpec: IJupyterKernelSpec = {
             argv: [interpreter!.path, '-m', 'ipykernel_launcher', '-f', '{connection_file}'],
             metadata: {
@@ -83,7 +76,6 @@ suite('DataScience raw kernel tests', () => {
         };
         kernelProcess = new KernelProcess(
             ioc.get<IProcessServiceFactory>(IProcessServiceFactory),
-            ioc.get<IFileSystem>(IFileSystem),
             ioc.get<KernelDaemonPool>(KernelDaemonPool),
             connectionInfo as any,
             kernelSpec,
@@ -97,11 +89,6 @@ suite('DataScience raw kernel tests', () => {
     async function disconnectFromKernel() {
         if (kernelProcess) {
             await kernelProcess.dispose().catch(noop);
-            try {
-                await fs.remove(connectionFile);
-            } catch {
-                noop();
-            }
         }
     }