Skip to content

Commit 9af9956

Browse files
authored
Do not expose kernel process (#11282)
For #10768 The kernel process wasn't used, except in tests. This is required, as when we spin up the kernel process in the daemon, the process returned by the TS code will return the process daemon and not the kernel process. This separation ensure we do not rely on it for anything, after all everything we need is in IKernelConnection (ports, etc)
1 parent 5026e13 commit 9af9956

File tree

3 files changed

+28
-10
lines changed

3 files changed

+28
-10
lines changed

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,6 @@ class KernelProcess implements IKernelProcess {
3939
public get kernelSpec(): Readonly<IJupyterKernelSpec> {
4040
return this._kernelSpec;
4141
}
42-
public get process(): ChildProcess | undefined {
43-
return this._process;
44-
}
4542
public get connection(): Readonly<IKernelConnection> {
4643
return this._connection;
4744
}

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// Licensed under the MIT License.
33
'use strict';
44

5-
import { ChildProcess } from 'child_process';
65
import { IDisposable } from 'monaco-editor';
76
import { Event } from 'vscode';
87
import { InterpreterUri } from '../../common/installer/types';
@@ -27,7 +26,6 @@ export interface IKernelConnection {
2726
}
2827

2928
export interface IKernelProcess extends IDisposable {
30-
process: ChildProcess | undefined;
3129
readonly connection: Readonly<IKernelConnection>;
3230
ready: Promise<void>;
3331
readonly kernelSpec: Readonly<IJupyterKernelSpec>;

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

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
import { assert } from 'chai';
66
import { Uri } from 'vscode';
77

8-
import { ChildProcess } from 'child_process';
98
import { IFileSystem } from '../../client/common/platform/types';
109
import { IPythonExecutionFactory } from '../../client/common/process/types';
1110
import { Resource } from '../../client/common/types';
@@ -14,7 +13,7 @@ import { JupyterZMQBinariesNotFoundError } from '../../client/datascience/jupyte
1413
import { KernelLauncher } from '../../client/datascience/kernel-launcher/kernelLauncher';
1514
import { IKernelConnection, IKernelFinder } from '../../client/datascience/kernel-launcher/types';
1615
import { InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts';
17-
import { PYTHON_PATH, sleep } from '../common';
16+
import { PYTHON_PATH, sleep, waitForCondition } from '../common';
1817
import { DataScienceIocContainer } from './dataScienceIocContainer';
1918

2019
suite('Kernel Launcher', () => {
@@ -50,25 +49,49 @@ suite('Kernel Launcher', () => {
5049
this.skip();
5150
} else {
5251
const kernel = await kernelLauncher.launch(resource, kernelName);
52+
const exited = new Promise<boolean>((resolve) => kernel.exited(() => resolve(true)));
5353

5454
assert.isOk<IKernelConnection | undefined>(kernel.connection, 'Connection not found');
55-
assert.isOk<ChildProcess | undefined>(kernel.process, 'Child Process not found');
5655

56+
// It should not exit.
57+
assert.isRejected(
58+
waitForCondition(() => exited, 5_000, 'Timeout'),
59+
'Timeout'
60+
);
61+
62+
// Upon disposing, we should get an exit event within 100ms or less.
63+
// If this happens, then we know a process existed.
5764
kernel.dispose();
65+
assert.isRejected(
66+
waitForCondition(() => exited, 100, 'Timeout'),
67+
'Timeout'
68+
);
5869
}
59-
});
70+
}).timeout(10_000);
6071

6172
test('Launch from PythonInterpreter', async function () {
6273
if (!process.env.VSCODE_PYTHON_ROLLING) {
6374
// tslint:disable-next-line: no-invalid-this
6475
this.skip();
6576
} else {
6677
const kernel = await kernelLauncher.launch(pythonInterpreter, kernelName);
78+
const exited = new Promise<boolean>((resolve) => kernel.exited(() => resolve(true)));
79+
80+
// It should not exit.
81+
assert.isRejected(
82+
waitForCondition(() => exited, 5_000, 'Timeout'),
83+
'Timeout'
84+
);
6785

6886
assert.isOk<IKernelConnection | undefined>(kernel.connection, 'Connection not found');
69-
assert.isOk<ChildProcess | undefined>(kernel.process, 'Child Process not found');
7087

88+
// Upon disposing, we should get an exit event within 100ms or less.
89+
// If this happens, then we know a process existed.
7190
kernel.dispose();
91+
assert.isRejected(
92+
waitForCondition(() => exited, 100, 'Timeout'),
93+
'Timeout'
94+
);
7295
}
7396
});
7497

0 commit comments

Comments
 (0)