Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use new port forwarding API #15243

Merged
merged 2 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/kernels/common/usedPorts.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,45 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

import {
PortAttributes,
type CancellationToken,
type PortAttributesProvider,
PortAutoForwardAction,
type PortAttributesSelector,
workspace
} from 'vscode';
import { DisposableStore } from '../../platform/common/utils/lifecycle';
import { traceError } from '../../platform/logging';

// Keeps track of all ports used by Kernels and other processes spawned by Kernels and related code
export const UsedPorts = new Set<number>();

export function ignorePortForwarding(...ports: number[]) {
const disposableStore = new DisposableStore();
try {
const provider = new (class implements PortAttributesProvider {
async providePortAttributes(
attributes: { port: number; pid?: number; commandLine?: string },
_token: CancellationToken
) {
if (ports.includes(attributes.port)) {
return new PortAttributes(PortAutoForwardAction.Ignore);
}
return undefined;
}
})();

for (const port of ports) {
const portSelector: PortAttributesSelector = {
portRange: port
};
disposableStore.add(workspace.registerPortAttributesProvider(portSelector, provider));
}
} catch (ex) {
// In case proposed API changes.
traceError('Failure in registering port attributes', ex);
}

return disposableStore;
}
8 changes: 6 additions & 2 deletions src/kernels/jupyter/launcher/jupyterServerStarter.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { getFilePath } from '../../../platform/common/platform/fs-paths';
import { JupyterNotebookNotInstalled } from '../../../platform/errors/jupyterNotebookNotInstalled';
import { JupyterCannotBeLaunchedWithRootError } from '../../../platform/errors/jupyterCannotBeLaunchedWithRootError';
import { noop } from '../../../platform/common/utils/misc';
import { UsedPorts } from '../../common/usedPorts';
import { UsedPorts, ignorePortForwarding } from '../../common/usedPorts';

/**
* Responsible for starting a notebook.
Expand Down Expand Up @@ -132,8 +132,12 @@ export class JupyterServerStarter implements IJupyterServerStarter {
try {
const port = parseInt(new URL(connection.baseUrl).port || '0', 10);
if (port && !isNaN(port)) {
const disposable = ignorePortForwarding(port);
if (launchResult.proc) {
launchResult.proc.on('exit', () => UsedPorts.delete(port));
launchResult.proc.on('exit', () => {
UsedPorts.delete(port);
disposable.dispose();
});
}
UsedPorts.add(port);
}
Expand Down
39 changes: 0 additions & 39 deletions src/kernels/portAttributeProvider.node.ts

This file was deleted.

17 changes: 12 additions & 5 deletions src/kernels/raw/launcher/kernelLauncher.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@ import { StopWatch } from '../../../platform/common/utils/stopWatch';
import { getResourceType } from '../../../platform/common/utils';
import { format, splitLines } from '../../../platform/common/helpers';
import { IPythonExecutionFactory } from '../../../platform/interpreter/types.node';
import { UsedPorts } from '../../common/usedPorts';
import { UsedPorts, ignorePortForwarding } from '../../common/usedPorts';
import { isPythonKernelConnection } from '../../helpers';
import { once } from '../../../platform/common/utils/events';

const PortFormatString = `kernelLauncherPortStart_{0}.tmp`;
// Launches and returns a kernel process given a resource or python interpreter.
Expand Down Expand Up @@ -206,7 +207,13 @@ export class KernelLauncher implements IKernelLauncher {
? window.createOutputChannel(DataScience.kernelConsoleOutputChannel(baseName), 'log')
: undefined;
outputChannel?.clear();

const portAttributeProvider = ignorePortForwarding(
connection.control_port,
connection.hb_port,
connection.iopub_port,
connection.shell_port,
connection.stdin_port
);
// Create the process
const kernelProcess = new KernelProcess(
this.processExecutionFactory,
Expand All @@ -223,8 +230,8 @@ export class KernelLauncher implements IKernelLauncher {
this.pythonKernelInterruptDaemon,
this.platformService
);

kernelProcess.exited(() => outputChannel?.dispose(), this, this.disposables);
once(kernelProcess.onDidDispose)(() => portAttributeProvider.dispose(), this, this.disposables);
once(kernelProcess.exited)(() => outputChannel?.dispose(), this, this.disposables);
try {
await raceCancellationError(cancelToken, kernelProcess.launch(workingDirectory, timeout, cancelToken));
} catch (ex) {
Expand All @@ -233,7 +240,7 @@ export class KernelLauncher implements IKernelLauncher {
throw ex;
}

const disposable = kernelProcess.exited(
const disposable = once(kernelProcess.exited)(
({ exitCode, reason }) => {
sendKernelTelemetryEvent(
resource,
Expand Down
36 changes: 27 additions & 9 deletions src/kernels/raw/launcher/kernelLauncher.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,18 @@ import { dispose } from '../../../platform/common/utils/lifecycle';
import { anything, instance, mock, when } from 'ts-mockito';
import { IFileSystemNode } from '../../../platform/common/platform/types.node';
import { PythonKernelConnectionMetadata } from '../../types';
import { CancellationTokenSource, Disposable, EventEmitter, PortAutoForwardAction, Uri } from 'vscode';
import {
CancellationTokenSource,
Disposable,
EventEmitter,
PortAutoForwardAction,
Uri,
type PortAttributesProvider
} from 'vscode';
import { KernelProcess } from './kernelProcess.node';
import { PortAttributesProviders } from '../../portAttributeProvider.node';
import { IPythonExecutionFactory, IPythonExecutionService } from '../../../platform/interpreter/types.node';
import { UsedPorts } from '../../common/usedPorts';
import { mockedVSCodeNamespaces, resetVSCodeMocks } from '../../../test/vscode-mock';

suite('kernel Launcher', () => {
let disposables: IDisposable[] = [];
Expand Down Expand Up @@ -67,7 +74,10 @@ suite('kernel Launcher', () => {
instance(platform)
);
});
teardown(() => (disposables = dispose(disposables)));
teardown(() => {
disposables = dispose(disposables);
resetVSCodeMocks();
});
async function launchKernel() {
const kernelSpec = PythonKernelConnectionMetadata.create({
id: '1',
Expand Down Expand Up @@ -103,17 +113,25 @@ suite('kernel Launcher', () => {
assert.notDeepEqual(Array.from(UsedPorts), oldPorts, 'Ports not updated');
});
test('Verify Kernel ports are not forwarded', async () => {
await launchKernel();
const oldPorts = new Set(UsedPorts);
const providers: PortAttributesProvider[] = [];
when(mockedVSCodeNamespaces.workspace.registerPortAttributesProvider(anything(), anything())).thenCall(
(_, provider) => providers.push(provider)
);

const portAttributeProvider = new PortAttributesProviders(disposables);
await launchKernel();
const cancellation = new CancellationTokenSource();
disposables.push(cancellation);

for (const port of UsedPorts) {
assert.equal(
portAttributeProvider.providePortAttributes({ port }, cancellation.token)?.autoForwardAction,
PortAutoForwardAction.Ignore,
'Kernel Port should not be forwarded'
if (oldPorts.has(port)) {
continue;
}
const results = await Promise.all(
providers.map((p) => Promise.resolve(p.providePortAttributes({ port }, cancellation.token)))
);
const portForwardingIgnored = results.some((r) => r?.autoForwardAction === PortAutoForwardAction.Ignore);
assert.isTrue(portForwardingIgnored, `Kernel Port ${port} should not be forwarded`);
}
});
});
15 changes: 8 additions & 7 deletions src/kernels/raw/launcher/kernelProcess.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,15 @@ import { IPythonExecutionFactory } from '../../../platform/interpreter/types.nod
import { getDisplayPath } from '../../../platform/common/platform/fs-paths';
import { StopWatch } from '../../../platform/common/utils/stopWatch';
import { ServiceContainer } from '../../../platform/ioc/container';
import { ObservableDisposable } from '../../../platform/common/utils/lifecycle';

const kernelOutputWithConnectionFile = 'To connect another client to this kernel, use:';
const kernelOutputToNotLog =
'NOTE: When using the `ipython kernel` entry point, Ctrl-C will not work.\n\nTo exit, you will have to explicitly quit this process, by either sending\n"quit" from a client, or using Ctrl-\\ in UNIX-like environments.\n\nTo read more about this, see https://github.com/ipython/ipython/issues/2049\n\n\n';

// Launches and disposes a kernel process given a kernelspec and a resource or python interpreter.
// Exposes connection information and the process itself.
export class KernelProcess implements IKernelProcess {
export class KernelProcess extends ObservableDisposable implements IKernelProcess {
private _pid?: number;
private _disposingPromise?: Promise<void>;
public get pid() {
Expand Down Expand Up @@ -100,7 +101,6 @@ export class KernelProcess implements IKernelProcess {
private _process?: ChildProcess;
private exitEvent = new EventEmitter<{ exitCode?: number; reason?: string }>();
private launchedOnce?: boolean;
private disposed?: boolean;
private connectionFile?: Uri;
private _launchKernelSpec?: IJupyterKernelSpec;
private interrupter?: Interrupter;
Expand All @@ -122,6 +122,7 @@ export class KernelProcess implements IKernelProcess {
private readonly pythonKernelInterruptDaemon: PythonKernelInterruptDaemon,
private readonly platform: IPlatformService
) {
super();
this._kernelConnectionMetadata = kernelConnectionMetadata;
}
public async interrupt(): Promise<void> {
Expand Down Expand Up @@ -176,7 +177,7 @@ export class KernelProcess implements IKernelProcess {
if (proc) {
proc.on('exit', (exitCode) => {
exitCode = exitCode || providedExitCode;
if (this.disposed) {
if (this.isDisposed) {
traceVerbose(`KernelProcess Exited, Exit Code - ${exitCode}`);
return;
}
Expand Down Expand Up @@ -248,7 +249,7 @@ export class KernelProcess implements IKernelProcess {
this.sendToOutput(output.out);
});
exeObs.out.done.catch((error) => {
if (this.disposed) {
if (this.isDisposed) {
traceWarning('Kernel died', error, stderr);
return;
}
Expand Down Expand Up @@ -337,14 +338,13 @@ export class KernelProcess implements IKernelProcess {
}
}

public async dispose(): Promise<void> {
public override async dispose(): Promise<void> {
if (this._disposingPromise) {
return this._disposingPromise;
}
if (this.disposed) {
if (this.isDisposed) {
return;
}
this.disposed = true;
const pid = this._process?.pid;
traceInfo(`Dispose Kernel process ${pid}.`);
this._disposingPromise = (async () => {
Expand All @@ -370,6 +370,7 @@ export class KernelProcess implements IKernelProcess {
});
traceVerbose(`Disposed Kernel process ${pid}.`);
})();
super.dispose();
}

private async killChildProcesses(pid?: number) {
Expand Down
5 changes: 0 additions & 5 deletions src/kernels/serviceRegistry.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { KernelRefreshIndicator } from './kernelRefreshIndicator.node';
import { KernelStartupCodeProviders } from './kernelStartupCodeProviders.node';
import { KernelStartupTelemetry } from './kernelStartupTelemetry.node';
import { KernelStatusProvider } from './kernelStatusProvider';
import { PortAttributesProviders } from './portAttributeProvider.node';
import { ContributedLocalKernelSpecFinder } from './raw/finder/contributedLocalKernelSpecFinder.node';
import { JupyterPaths } from './raw/finder/jupyterPaths.node';
import { LocalKnownPathKernelSpecFinder } from './raw/finder/localKnownPathKernelSpecFinder.node';
Expand Down Expand Up @@ -54,10 +53,6 @@ import { KernelChatStartupCodeProvider } from './chat/kernelStartupCodeProvider'
export function registerTypes(serviceManager: IServiceManager, isDevMode: boolean) {
serviceManager.addSingleton<IExtensionSyncActivationService>(IExtensionSyncActivationService, Activation);
serviceManager.addSingleton<IExtensionSyncActivationService>(IExtensionSyncActivationService, ServerPreload);
serviceManager.addSingleton<IExtensionSyncActivationService>(
IExtensionSyncActivationService,
PortAttributesProviders
);
serviceManager.addSingleton<IRawNotebookSupportedService>(
IRawNotebookSupportedService,
RawNotebookSupportedService
Expand Down
Loading