From 79939821885849a19b727aadd04accee687ba8a1 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 12 May 2023 14:00:31 -0700 Subject: [PATCH 01/10] Create utility process pty host on Electron --- src/vs/code/electron-main/app.ts | 3 +- .../electron-main/electronPtyHostStarter.ts | 95 ++++++++++--------- src/vs/platform/terminal/node/ptyHost.ts | 2 +- .../platform/terminal/node/ptyHostService.ts | 31 +++--- 4 files changed, 69 insertions(+), 62 deletions(-) diff --git a/src/vs/code/electron-main/app.ts b/src/vs/code/electron-main/app.ts index 711e8fae66c33..8a5181c3dabd9 100644 --- a/src/vs/code/electron-main/app.ts +++ b/src/vs/code/electron-main/app.ts @@ -931,14 +931,13 @@ export class CodeApplication extends Disposable { graceTime: LocalReconnectConstants.GraceTime, shortGraceTime: LocalReconnectConstants.ShortGraceTime, scrollback: this.configurationService.getValue(TerminalSettingId.PersistentSessionScrollback) ?? 100 - }, this.environmentMainService); + }, this.environmentMainService, this.lifecycleMainService, this.logService); const ptyHostService = new PtyHostService( ptyHostStarter, this.configurationService, this.logService, this.loggerService ); - ptyHostService.initialize(); services.set(ILocalPtyService, ptyHostService); // External terminal diff --git a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts index 29eca7cd6288f..bb5fa9997a3ad 100644 --- a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts +++ b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts @@ -4,60 +4,67 @@ *--------------------------------------------------------------------------------------------*/ import { IEnvironmentService, INativeEnvironmentService } from 'vs/platform/environment/common/environment'; +import { parsePtyHostDebugPort } from 'vs/platform/environment/node/environmentService'; +import { ILifecycleMainService } from 'vs/platform/lifecycle/electron-main/lifecycleMainService'; +import { ILogService } from 'vs/platform/log/common/log'; +import { NullTelemetryService } from 'vs/platform/telemetry/common/telemetryUtils'; import { IReconnectConstants } from 'vs/platform/terminal/common/terminal'; -import { NodePtyHostStarter } from 'vs/platform/terminal/node/nodePtyHostStarter'; import { IPtyHostConnection, IPtyHostStarter } from 'vs/platform/terminal/node/ptyHost'; -// import { FileAccess } from 'vs/base/common/network'; -// import { Client, IIPCOptions } from 'vs/base/parts/ipc/node/ipc.cp'; -// import { parsePtyHostDebugPort } from 'vs/platform/environment/node/environmentService'; -// import { UtilityProcess } from 'vs/platform/utilityProcess/electron-main/utilityProcess'; +import { UtilityProcess } from 'vs/platform/utilityProcess/electron-main/utilityProcess'; +import { Client as MessagePortClient } from 'vs/base/parts/ipc/electron-main/ipc.mp'; export class ElectronPtyHostStarter implements IPtyHostStarter { - // private utilityProcess: UtilityProcess | undefined = undefined; + private utilityProcess: UtilityProcess | undefined = undefined; constructor( private readonly _reconnectConstants: IReconnectConstants, - @IEnvironmentService private readonly _environmentService: INativeEnvironmentService + @IEnvironmentService private readonly _environmentService: INativeEnvironmentService, + @ILifecycleMainService private readonly _lifecycleMainService: ILifecycleMainService, + @ILogService private readonly _logService: ILogService ) { } - start(lastPtyId: number): IPtyHostConnection { - return new NodePtyHostStarter(this._reconnectConstants, this._environmentService).start(lastPtyId); - - // console.log('use utility proc'); - - // // TODO: Convert to use utility process - // const opts: IIPCOptions = { - // serverName: 'Pty Host', - // args: ['--type=ptyHost', '--logsPath', this._environmentService.logsHome.fsPath], - // env: { - // VSCODE_LAST_PTY_ID: lastPtyId, - // VSCODE_PTY_REMOTE: this._isRemote, - // VSCODE_AMD_ENTRYPOINT: 'vs/platform/terminal/node/ptyHostMain', - // VSCODE_PIPE_LOGGING: 'true', - // VSCODE_VERBOSE_LOGGING: 'true', // transmit console logs from server to client, - // VSCODE_RECONNECT_GRACE_TIME: this._reconnectConstants.graceTime, - // VSCODE_RECONNECT_SHORT_GRACE_TIME: this._reconnectConstants.shortGraceTime, - // VSCODE_RECONNECT_SCROLLBACK: this._reconnectConstants.scrollback - // } - // }; - - // const ptyHostDebug = parsePtyHostDebugPort(this._environmentService.args, this._environmentService.isBuilt); - // if (ptyHostDebug) { - // if (ptyHostDebug.break && ptyHostDebug.port) { - // opts.debugBrk = ptyHostDebug.port; - // } else if (!ptyHostDebug.break && ptyHostDebug.port) { - // opts.debug = ptyHostDebug.port; - // } - // } - - // const client = new Client(FileAccess.asFileUri('bootstrap-fork').fsPath, opts); - - // return { - // client, - // dispose: client.dispose, - // onDidProcessExit: client.onDidProcessExit - // }; + async start(lastPtyId: number): Promise { + this.utilityProcess = new UtilityProcess(this._logService, NullTelemetryService, this._lifecycleMainService); + + const inspectParams = parsePtyHostDebugPort(this._environmentService.args, this._environmentService.isBuilt); + let execArgv: string[] | undefined = undefined; + if (inspectParams) { + execArgv = ['--nolazy']; + if (inspectParams.break) { + execArgv.push(`--inspect-brk=${inspectParams.port}`); + } else if (!inspectParams.break) { + execArgv.push(`--inspect=${inspectParams.port}`); + } + } + + this.utilityProcess.start({ + type: 'ptyHost', + entryPoint: 'vs/platform/terminal/node/ptyHostMain', + payload: this._createPtyHostConfiguration(lastPtyId), + execArgv + }); + + const port = this.utilityProcess.connect(); + const client = new MessagePortClient(port, 'ptyHost'); + + return { + client, + dispose: client.dispose, + onDidProcessExit: this.utilityProcess.onExit + }; + } + + private _createPtyHostConfiguration(lastPtyId: number) { + return { + VSCODE_LAST_PTY_ID: lastPtyId, + VSCODE_AMD_ENTRYPOINT: 'vs/platform/terminal/node/ptyHostMain', + VSCODE_PIPE_LOGGING: 'true', + VSCODE_VERBOSE_LOGGING: 'true', // transmit console logs from server to client, + VSCODE_RECONNECT_GRACE_TIME: this._reconnectConstants.graceTime, + VSCODE_RECONNECT_SHORT_GRACE_TIME: this._reconnectConstants.shortGraceTime, + VSCODE_RECONNECT_SCROLLBACK: this._reconnectConstants.scrollback + }; } } diff --git a/src/vs/platform/terminal/node/ptyHost.ts b/src/vs/platform/terminal/node/ptyHost.ts index 918fd15aed2ef..b75696fa3fadf 100644 --- a/src/vs/platform/terminal/node/ptyHost.ts +++ b/src/vs/platform/terminal/node/ptyHost.ts @@ -19,5 +19,5 @@ export interface IPtyHostStarter { * @param lastPtyId Tracks the last terminal ID from the pty host so we can give it to the new * pty host if it's restarted and avoid ID conflicts. */ - start(lastPtyId: number): IPtyHostConnection; + start(lastPtyId: number): IPtyHostConnection | Promise; } diff --git a/src/vs/platform/terminal/node/ptyHostService.ts b/src/vs/platform/terminal/node/ptyHostService.ts index afcd521c84aea..2a757a8aad4ae 100644 --- a/src/vs/platform/terminal/node/ptyHostService.ts +++ b/src/vs/platform/terminal/node/ptyHostService.ts @@ -36,9 +36,10 @@ let lastPtyId = 0; export class PtyHostService extends Disposable implements IPtyService { declare readonly _serviceBrand: undefined; - private _connection: IPtyHostConnection; + // TODO: Avoid ! ? + private _connection!: IPtyHostConnection; // ProxyChannel is not used here because events get lost when forwarding across multiple proxies - private _proxy: IPtyService; + private _proxy!: IPtyService; private readonly _shellEnv: Promise; private readonly _resolveVariablesRequestStore: RequestStore; @@ -93,17 +94,17 @@ export class PtyHostService extends Disposable implements IPtyService { this._resolveVariablesRequestStore = this._register(new RequestStore(undefined, this._logService)); this._resolveVariablesRequestStore.onCreateRequest(this._onPtyHostRequestResolveVariables.fire, this._onPtyHostRequestResolveVariables); - [this._connection, this._proxy] = this._startPtyHost(); + this._startPtyHost().then(value => { + this._connection = value[0]; + this._proxy = value[1]; - this._register(this._configurationService.onDidChangeConfiguration(async e => { - if (e.affectsConfiguration(TerminalSettingId.IgnoreProcessNames)) { - await this._refreshIgnoreProcessNames(); - } - })); - } - - initialize(): void { - this._refreshIgnoreProcessNames(); + this._register(this._configurationService.onDidChangeConfiguration(async e => { + if (e.affectsConfiguration(TerminalSettingId.IgnoreProcessNames)) { + await this._refreshIgnoreProcessNames(); + } + })); + this._refreshIgnoreProcessNames(); + }); } private get _ignoreProcessNames(): string[] { @@ -128,8 +129,8 @@ export class PtyHostService extends Disposable implements IPtyService { } } - private _startPtyHost(): [IPtyHostConnection, IPtyService] { - const connection = this._ptyHostStarter.start(lastPtyId); + private async _startPtyHost(): Promise<[IPtyHostConnection, IPtyService]> { + const connection = await this._ptyHostStarter.start(lastPtyId); const client = connection.client; this._onPtyHostStart.fire(); @@ -317,7 +318,7 @@ export class PtyHostService extends Disposable implements IPtyService { async restartPtyHost(): Promise { this._isResponsive = true; this._disposePtyHost(); - [this._connection, this._proxy] = this._startPtyHost(); + [this._connection, this._proxy] = await this._startPtyHost(); } private _disposePtyHost(): void { From 3b26dc10c3aca90ea5d32ffccdcd007e4fe51080 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 12 May 2023 14:57:33 -0700 Subject: [PATCH 02/10] Connect message port from window to pty host --- src/vs/platform/terminal/common/terminal.ts | 2 ++ .../electron-main/electronPtyHostStarter.ts | 4 +++ src/vs/platform/terminal/node/ptyHost.ts | 3 +++ src/vs/platform/terminal/node/ptyHostMain.ts | 10 +++++-- .../platform/terminal/node/ptyHostService.ts | 23 ++++++++++++++++ .../electron-sandbox/localTerminalBackend.ts | 27 ++++++++++++++++++- 6 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/vs/platform/terminal/common/terminal.ts b/src/vs/platform/terminal/common/terminal.ts index 198b7dda23fe7..b7e5c4dabad0b 100644 --- a/src/vs/platform/terminal/common/terminal.ts +++ b/src/vs/platform/terminal/common/terminal.ts @@ -207,6 +207,8 @@ export enum TerminalIpcChannels { * Communicates between the shared process and the pty host process. */ PtyHost = 'ptyHost', + + PtyHostWindow = 'ptyHostWindow', /** * Deals with logging from the pty host process. */ diff --git a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts index bb5fa9997a3ad..557c3f6c4ae48 100644 --- a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts +++ b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts @@ -12,6 +12,8 @@ import { IReconnectConstants } from 'vs/platform/terminal/common/terminal'; import { IPtyHostConnection, IPtyHostStarter } from 'vs/platform/terminal/node/ptyHost'; import { UtilityProcess } from 'vs/platform/utilityProcess/electron-main/utilityProcess'; import { Client as MessagePortClient } from 'vs/base/parts/ipc/electron-main/ipc.mp'; +import { MessageChannelMain, MessagePortMain } from 'electron'; +import { assertIsDefined } from 'vs/base/common/types'; export class ElectronPtyHostStarter implements IPtyHostStarter { @@ -51,6 +53,8 @@ export class ElectronPtyHostStarter implements IPtyHostStarter { return { client, + port, + connect: () => assertIsDefined(this.utilityProcess).connect(), dispose: client.dispose, onDidProcessExit: this.utilityProcess.onExit }; diff --git a/src/vs/platform/terminal/node/ptyHost.ts b/src/vs/platform/terminal/node/ptyHost.ts index b75696fa3fadf..b853864cb725a 100644 --- a/src/vs/platform/terminal/node/ptyHost.ts +++ b/src/vs/platform/terminal/node/ptyHost.ts @@ -9,6 +9,9 @@ import { IChannelClient } from 'vs/base/parts/ipc/common/ipc'; export interface IPtyHostConnection extends IDisposable { readonly client: IChannelClient; + // TODO: Type + readonly port?: any; + connect?(): any; readonly onDidProcessExit: Event<{ code: number; signal: string }>; } diff --git a/src/vs/platform/terminal/node/ptyHostMain.ts b/src/vs/platform/terminal/node/ptyHostMain.ts index 9414c15bf2f94..c2f5ee0396eb9 100644 --- a/src/vs/platform/terminal/node/ptyHostMain.ts +++ b/src/vs/platform/terminal/node/ptyHostMain.ts @@ -21,8 +21,10 @@ import { HeartbeatService } from 'vs/platform/terminal/node/heartbeatService'; import { PtyService } from 'vs/platform/terminal/node/ptyService'; import { isUtilityProcess } from 'vs/base/parts/sandbox/node/electronTypes'; +const _isUtilityProcess = isUtilityProcess(process); + let server: ChildProcessServer | UtilityProcessServer; -if (isUtilityProcess(process)) { +if (_isUtilityProcess) { server = new UtilityProcessServer(); } else { server = new ChildProcessServer(TerminalIpcChannels.PtyHost); @@ -53,7 +55,11 @@ delete process.env.VSCODE_RECONNECT_SHORT_GRACE_TIME; delete process.env.VSCODE_RECONNECT_SCROLLBACK; const ptyService = new PtyService(lastPtyId, logService, productService, reconnectConstants); -server.registerChannel(TerminalIpcChannels.PtyHost, ProxyChannel.fromService(ptyService)); +const ptyServiceChannel = ProxyChannel.fromService(ptyService); +server.registerChannel(TerminalIpcChannels.PtyHost, ptyServiceChannel); +if (_isUtilityProcess) { + server.registerChannel(TerminalIpcChannels.PtyHostWindow, ptyServiceChannel); +} process.once('exit', () => { logService.dispose(); diff --git a/src/vs/platform/terminal/node/ptyHostService.ts b/src/vs/platform/terminal/node/ptyHostService.ts index 2a757a8aad4ae..63a87a79558c7 100644 --- a/src/vs/platform/terminal/node/ptyHostService.ts +++ b/src/vs/platform/terminal/node/ptyHostService.ts @@ -3,10 +3,12 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ +import { IpcMainEvent, MessagePortMain } from 'electron'; import { Emitter, Event } from 'vs/base/common/event'; import { Disposable, toDisposable } from 'vs/base/common/lifecycle'; import { IProcessEnvironment, OperatingSystem, isWindows } from 'vs/base/common/platform'; import { ProxyChannel } from 'vs/base/parts/ipc/common/ipc'; +import { validatedIpcMain } from 'vs/base/parts/ipc/electron-main/ipcMain'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; import { ILogService, ILoggerService } from 'vs/platform/log/common/log'; import { RemoteLoggerChannelClient } from 'vs/platform/log/common/logIpc'; @@ -94,6 +96,11 @@ export class PtyHostService extends Disposable implements IPtyService { this._resolveVariablesRequestStore = this._register(new RequestStore(undefined, this._logService)); this._resolveVariablesRequestStore.onCreateRequest(this._onPtyHostRequestResolveVariables.fire, this._onPtyHostRequestResolveVariables); + validatedIpcMain.on('vscode:createPtyHostMessageChannel', (e, nonce) => this._onWindowConnection(e, nonce)); + + // TODO: Start on demand on first window connection (see SharedProcess.onWindowConnection + // TODO: Make direct message port connection to each window when requested + this._startPtyHost().then(value => { this._connection = value[0]; this._proxy = value[1]; @@ -107,6 +114,22 @@ export class PtyHostService extends Disposable implements IPtyService { }); } + private _onWindowConnection(e: IpcMainEvent, nonce: string) { + const port = this._connection.connect!() as MessagePortMain; + + + // Check back if the requesting window meanwhile closed + // Since shared process is delayed on startup there is + // a chance that the window close before the shared process + // was ready for a connection. + + if (e.sender.isDestroyed()) { + return port.close(); + } + + e.sender.postMessage('vscode:createPtyHostMessageChannelResult', nonce, [port]); + } + private get _ignoreProcessNames(): string[] { return this._configurationService.getValue(TerminalSettingId.IgnoreProcessNames); } diff --git a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts index a83b7f624ad83..ed3cc65c08db7 100644 --- a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts +++ b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts @@ -14,7 +14,7 @@ import { ILogService } from 'vs/platform/log/common/log'; import { INotificationService } from 'vs/platform/notification/common/notification'; import { Registry } from 'vs/platform/registry/common/platform'; import { IStorageService, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage'; -import { ILocalPtyService, IProcessPropertyMap, IShellLaunchConfig, ITerminalChildProcess, ITerminalEnvironment, ITerminalProcessOptions, ITerminalsLayoutInfo, ITerminalsLayoutInfoById, ProcessPropertyType, TerminalSettingId, TitleEventSource } from 'vs/platform/terminal/common/terminal'; +import { ILocalPtyService, IProcessPropertyMap, IPtyService, IShellLaunchConfig, ITerminalChildProcess, ITerminalEnvironment, ITerminalProcessOptions, ITerminalsLayoutInfo, ITerminalsLayoutInfoById, ProcessPropertyType, TerminalIpcChannels, TerminalSettingId, TitleEventSource } from 'vs/platform/terminal/common/terminal'; import { IGetTerminalLayoutInfoArgs, IProcessDetails, ISetTerminalLayoutInfoArgs } from 'vs/platform/terminal/common/terminalProcess'; import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace'; import { IWorkbenchContribution } from 'vs/workbench/common/contributions'; @@ -30,6 +30,10 @@ import { IProductService } from 'vs/platform/product/common/productService'; import { IEnvironmentVariableService } from 'vs/workbench/contrib/terminal/common/environmentVariable'; import { BaseTerminalBackend } from 'vs/workbench/contrib/terminal/browser/baseTerminalBackend'; import { getWorkspaceForTerminal } from 'vs/workbench/services/configurationResolver/common/terminalResolver'; +import { INativeWorkbenchEnvironmentService } from 'vs/workbench/services/environment/electron-sandbox/environmentService'; +import { Client as MessagePortClient } from 'vs/base/parts/ipc/common/ipc.mp'; +import { acquirePort } from 'vs/base/parts/ipc/electron-sandbox/ipc.mp'; +import { ProxyChannel } from 'vs/base/parts/ipc/common/ipc'; export class LocalTerminalBackendContribution implements IWorkbenchContribution { constructor( @@ -66,9 +70,30 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke @IEnvironmentVariableService private readonly _environmentVariableService: IEnvironmentVariableService, @INotificationService notificationService: INotificationService, @IHistoryService historyService: IHistoryService, + @INativeWorkbenchEnvironmentService environmentService: INativeWorkbenchEnvironmentService ) { super(_localPtyService, logService, notificationService, historyService, _configurationResolverService, workspaceContextService); + + + + (async () => { + // mark('code/willConnectSharedProcess'); + // this.logService.trace('Renderer->SharedProcess#connect: before acquirePort'); + const port = await acquirePort('vscode:createPtyHostMessageChannel', 'vscode:createPtyHostMessageChannelResult'); + // mark('code/didConnectSharedProcess'); + // this.logService.trace('Renderer->SharedProcess#connect: connection established'); + + const client = new MessagePortClient(port, `window:${environmentService.window.id}`); + const proxy = ProxyChannel.toService(client.getChannel(TerminalIpcChannels.PtyHostWindow)); + + logService.info('latency: ', proxy.getLatency(0)); + proxy.onProcessData(e => { + logService.info('message port process data: ' + e.event); + }); + })(); + + // Attach process listeners this._localPtyService.onProcessData(e => this._ptys.get(e.id)?.handleData(e.event)); this._localPtyService.onDidChangeProperty(e => this._ptys.get(e.id)?.handleDidChangeProperty(e.property)); From 3876a21907cf4f92fff538db11bce0972a5a2d1c Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sat, 13 May 2023 07:19:38 -0700 Subject: [PATCH 03/10] Tidy up window<->pty host connection --- .../electron-main/electronPtyHostStarter.ts | 24 +++++++++- src/vs/platform/terminal/node/ptyHost.ts | 2 +- .../platform/terminal/node/ptyHostService.ts | 44 ++++--------------- 3 files changed, 32 insertions(+), 38 deletions(-) diff --git a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts index 557c3f6c4ae48..b6b29118e6691 100644 --- a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts +++ b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts @@ -12,8 +12,9 @@ import { IReconnectConstants } from 'vs/platform/terminal/common/terminal'; import { IPtyHostConnection, IPtyHostStarter } from 'vs/platform/terminal/node/ptyHost'; import { UtilityProcess } from 'vs/platform/utilityProcess/electron-main/utilityProcess'; import { Client as MessagePortClient } from 'vs/base/parts/ipc/electron-main/ipc.mp'; -import { MessageChannelMain, MessagePortMain } from 'electron'; +import { IpcMainEvent } from 'electron'; import { assertIsDefined } from 'vs/base/common/types'; +import { validatedIpcMain } from 'vs/base/parts/ipc/electron-main/ipcMain'; export class ElectronPtyHostStarter implements IPtyHostStarter { @@ -27,7 +28,7 @@ export class ElectronPtyHostStarter implements IPtyHostStarter { ) { } - async start(lastPtyId: number): Promise { + start(lastPtyId: number): IPtyHostConnection { this.utilityProcess = new UtilityProcess(this._logService, NullTelemetryService, this._lifecycleMainService); const inspectParams = parsePtyHostDebugPort(this._environmentService.args, this._environmentService.isBuilt); @@ -51,6 +52,9 @@ export class ElectronPtyHostStarter implements IPtyHostStarter { const port = this.utilityProcess.connect(); const client = new MessagePortClient(port, 'ptyHost'); + // Listen for new windows to establish connection directly to pty host + validatedIpcMain.on('vscode:createPtyHostMessageChannel', (e, nonce) => this._onWindowConnection(e, nonce)); + return { client, port, @@ -71,4 +75,20 @@ export class ElectronPtyHostStarter implements IPtyHostStarter { VSCODE_RECONNECT_SCROLLBACK: this._reconnectConstants.scrollback }; } + + private _onWindowConnection(e: IpcMainEvent, nonce: string) { + const port = this.utilityProcess!.connect(); + + // Check back if the requesting window meanwhile closed + // Since shared process is delayed on startup there is + // a chance that the window close before the shared process + // was ready for a connection. + + if (e.sender.isDestroyed()) { + port.close(); + return; + } + + e.sender.postMessage('vscode:createPtyHostMessageChannelResult', nonce, [port]); + } } diff --git a/src/vs/platform/terminal/node/ptyHost.ts b/src/vs/platform/terminal/node/ptyHost.ts index b853864cb725a..14bc47b05b2d3 100644 --- a/src/vs/platform/terminal/node/ptyHost.ts +++ b/src/vs/platform/terminal/node/ptyHost.ts @@ -22,5 +22,5 @@ export interface IPtyHostStarter { * @param lastPtyId Tracks the last terminal ID from the pty host so we can give it to the new * pty host if it's restarted and avoid ID conflicts. */ - start(lastPtyId: number): IPtyHostConnection | Promise; + start(lastPtyId: number): IPtyHostConnection; } diff --git a/src/vs/platform/terminal/node/ptyHostService.ts b/src/vs/platform/terminal/node/ptyHostService.ts index 63a87a79558c7..4e221db0a4e8f 100644 --- a/src/vs/platform/terminal/node/ptyHostService.ts +++ b/src/vs/platform/terminal/node/ptyHostService.ts @@ -3,12 +3,10 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { IpcMainEvent, MessagePortMain } from 'electron'; import { Emitter, Event } from 'vs/base/common/event'; import { Disposable, toDisposable } from 'vs/base/common/lifecycle'; import { IProcessEnvironment, OperatingSystem, isWindows } from 'vs/base/common/platform'; import { ProxyChannel } from 'vs/base/parts/ipc/common/ipc'; -import { validatedIpcMain } from 'vs/base/parts/ipc/electron-main/ipcMain'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; import { ILogService, ILoggerService } from 'vs/platform/log/common/log'; import { RemoteLoggerChannelClient } from 'vs/platform/log/common/logIpc'; @@ -96,38 +94,14 @@ export class PtyHostService extends Disposable implements IPtyService { this._resolveVariablesRequestStore = this._register(new RequestStore(undefined, this._logService)); this._resolveVariablesRequestStore.onCreateRequest(this._onPtyHostRequestResolveVariables.fire, this._onPtyHostRequestResolveVariables); - validatedIpcMain.on('vscode:createPtyHostMessageChannel', (e, nonce) => this._onWindowConnection(e, nonce)); + [this._connection, this._proxy] = this._startPtyHost(); - // TODO: Start on demand on first window connection (see SharedProcess.onWindowConnection - // TODO: Make direct message port connection to each window when requested - - this._startPtyHost().then(value => { - this._connection = value[0]; - this._proxy = value[1]; - - this._register(this._configurationService.onDidChangeConfiguration(async e => { - if (e.affectsConfiguration(TerminalSettingId.IgnoreProcessNames)) { - await this._refreshIgnoreProcessNames(); - } - })); - this._refreshIgnoreProcessNames(); - }); - } - - private _onWindowConnection(e: IpcMainEvent, nonce: string) { - const port = this._connection.connect!() as MessagePortMain; - - - // Check back if the requesting window meanwhile closed - // Since shared process is delayed on startup there is - // a chance that the window close before the shared process - // was ready for a connection. - - if (e.sender.isDestroyed()) { - return port.close(); - } - - e.sender.postMessage('vscode:createPtyHostMessageChannelResult', nonce, [port]); + this._register(this._configurationService.onDidChangeConfiguration(async e => { + if (e.affectsConfiguration(TerminalSettingId.IgnoreProcessNames)) { + await this._refreshIgnoreProcessNames(); + } + })); + this._refreshIgnoreProcessNames(); } private get _ignoreProcessNames(): string[] { @@ -152,8 +126,8 @@ export class PtyHostService extends Disposable implements IPtyService { } } - private async _startPtyHost(): Promise<[IPtyHostConnection, IPtyService]> { - const connection = await this._ptyHostStarter.start(lastPtyId); + private _startPtyHost(): [IPtyHostConnection, IPtyService] { + const connection = this._ptyHostStarter.start(lastPtyId); const client = connection.client; this._onPtyHostStart.fire(); From b386bae7730cee3b882abad329cc4ce95565f8f2 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sat, 13 May 2023 07:35:49 -0700 Subject: [PATCH 04/10] Use a disposable store --- .../electron-main/electronPtyHostStarter.ts | 16 ++++++++++++---- .../platform/terminal/node/nodePtyHostStarter.ts | 6 +++++- src/vs/platform/terminal/node/ptyHost.ts | 8 +++----- src/vs/platform/terminal/node/ptyHostService.ts | 2 +- .../electron-sandbox/localTerminalBackend.ts | 15 +++++++-------- 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts index b6b29118e6691..17ed44d13bb12 100644 --- a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts +++ b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts @@ -13,8 +13,8 @@ import { IPtyHostConnection, IPtyHostStarter } from 'vs/platform/terminal/node/p import { UtilityProcess } from 'vs/platform/utilityProcess/electron-main/utilityProcess'; import { Client as MessagePortClient } from 'vs/base/parts/ipc/electron-main/ipc.mp'; import { IpcMainEvent } from 'electron'; -import { assertIsDefined } from 'vs/base/common/types'; import { validatedIpcMain } from 'vs/base/parts/ipc/electron-main/ipcMain'; +import { DisposableStore, toDisposable } from 'vs/base/common/lifecycle'; export class ElectronPtyHostStarter implements IPtyHostStarter { @@ -55,11 +55,19 @@ export class ElectronPtyHostStarter implements IPtyHostStarter { // Listen for new windows to establish connection directly to pty host validatedIpcMain.on('vscode:createPtyHostMessageChannel', (e, nonce) => this._onWindowConnection(e, nonce)); + // TODO: Do we need to listen for window close to close the port? + + const store = new DisposableStore(); + store.add(client); + store.add(this.utilityProcess); + store.add(toDisposable(() => { + validatedIpcMain.removeHandler('vscode:createPtyHostMessageChannel'); + this.utilityProcess = undefined; + })); + return { client, - port, - connect: () => assertIsDefined(this.utilityProcess).connect(), - dispose: client.dispose, + store, onDidProcessExit: this.utilityProcess.onExit }; } diff --git a/src/vs/platform/terminal/node/nodePtyHostStarter.ts b/src/vs/platform/terminal/node/nodePtyHostStarter.ts index a102d3101957a..4a589c09a4d3e 100644 --- a/src/vs/platform/terminal/node/nodePtyHostStarter.ts +++ b/src/vs/platform/terminal/node/nodePtyHostStarter.ts @@ -3,6 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ +import { DisposableStore } from 'vs/base/common/lifecycle'; import { FileAccess } from 'vs/base/common/network'; import { Client, IIPCOptions } from 'vs/base/parts/ipc/node/ipc.cp'; import { IEnvironmentService, INativeEnvironmentService } from 'vs/platform/environment/common/environment'; @@ -43,9 +44,12 @@ export class NodePtyHostStarter implements IPtyHostStarter { const client = new Client(FileAccess.asFileUri('bootstrap-fork').fsPath, opts); + const store = new DisposableStore(); + store.add(client); + return { client, - dispose: client.dispose, + store, onDidProcessExit: client.onDidProcessExit }; } diff --git a/src/vs/platform/terminal/node/ptyHost.ts b/src/vs/platform/terminal/node/ptyHost.ts index 14bc47b05b2d3..c6b0013f64b2b 100644 --- a/src/vs/platform/terminal/node/ptyHost.ts +++ b/src/vs/platform/terminal/node/ptyHost.ts @@ -4,14 +4,12 @@ *--------------------------------------------------------------------------------------------*/ import { Event } from 'vs/base/common/event'; -import { IDisposable } from 'vs/base/common/lifecycle'; +import { DisposableStore } from 'vs/base/common/lifecycle'; import { IChannelClient } from 'vs/base/parts/ipc/common/ipc'; -export interface IPtyHostConnection extends IDisposable { +export interface IPtyHostConnection { readonly client: IChannelClient; - // TODO: Type - readonly port?: any; - connect?(): any; + readonly store: DisposableStore; readonly onDidProcessExit: Event<{ code: number; signal: string }>; } diff --git a/src/vs/platform/terminal/node/ptyHostService.ts b/src/vs/platform/terminal/node/ptyHostService.ts index 4e221db0a4e8f..fac306eb60633 100644 --- a/src/vs/platform/terminal/node/ptyHostService.ts +++ b/src/vs/platform/terminal/node/ptyHostService.ts @@ -320,7 +320,7 @@ export class PtyHostService extends Disposable implements IPtyService { private _disposePtyHost(): void { this._proxy.shutdownAll?.(); - this._connection.dispose(); + this._connection.store.dispose(); } private _handleHeartbeat() { diff --git a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts index ed3cc65c08db7..60ce1cb65c615 100644 --- a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts +++ b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts @@ -34,6 +34,7 @@ import { INativeWorkbenchEnvironmentService } from 'vs/workbench/services/enviro import { Client as MessagePortClient } from 'vs/base/parts/ipc/common/ipc.mp'; import { acquirePort } from 'vs/base/parts/ipc/electron-sandbox/ipc.mp'; import { ProxyChannel } from 'vs/base/parts/ipc/common/ipc'; +import { mark } from 'vs/base/common/performance'; export class LocalTerminalBackendContribution implements IWorkbenchContribution { constructor( @@ -74,26 +75,24 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke ) { super(_localPtyService, logService, notificationService, historyService, _configurationResolverService, workspaceContextService); - - - + // TODO: Use the direct connection (async () => { - // mark('code/willConnectSharedProcess'); - // this.logService.trace('Renderer->SharedProcess#connect: before acquirePort'); + mark('code/willConnectPtyHost'); + logService.trace('Renderer->PtyHost#connect: before acquirePort'); const port = await acquirePort('vscode:createPtyHostMessageChannel', 'vscode:createPtyHostMessageChannelResult'); - // mark('code/didConnectSharedProcess'); - // this.logService.trace('Renderer->SharedProcess#connect: connection established'); + mark('code/didConnectPtyHost'); + logService.trace('Renderer->PtyHost#connect: connection established'); const client = new MessagePortClient(port, `window:${environmentService.window.id}`); const proxy = ProxyChannel.toService(client.getChannel(TerminalIpcChannels.PtyHostWindow)); + // Testing logService.info('latency: ', proxy.getLatency(0)); proxy.onProcessData(e => { logService.info('message port process data: ' + e.event); }); })(); - // Attach process listeners this._localPtyService.onProcessData(e => this._ptys.get(e.id)?.handleData(e.event)); this._localPtyService.onDidChangeProperty(e => this._ptys.get(e.id)?.handleDidChangeProperty(e.property)); From b725a17a109465037c077d35e89ffaee4b99cf2f Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sat, 13 May 2023 07:47:17 -0700 Subject: [PATCH 05/10] Prefer direct channel --- .../electron-sandbox/localTerminalBackend.ts | 97 ++++++++++--------- 1 file changed, 52 insertions(+), 45 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts index 60ce1cb65c615..66c8acc836020 100644 --- a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts +++ b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts @@ -50,6 +50,8 @@ export class LocalTerminalBackendContribution implements IWorkbenchContribution class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBackend { private readonly _ptys: Map = new Map(); + private _ptyHostDirectProxy: IPtyService | undefined; + private readonly _onDidRequestDetach = this._register(new Emitter<{ requestId: number; workspaceId: string; instanceId: number }>()); readonly onDidRequestDetach = this._onDidRequestDetach.event; @@ -71,62 +73,66 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke @IEnvironmentVariableService private readonly _environmentVariableService: IEnvironmentVariableService, @INotificationService notificationService: INotificationService, @IHistoryService historyService: IHistoryService, - @INativeWorkbenchEnvironmentService environmentService: INativeWorkbenchEnvironmentService + @INativeWorkbenchEnvironmentService private readonly _environmentService: INativeWorkbenchEnvironmentService ) { super(_localPtyService, logService, notificationService, historyService, _configurationResolverService, workspaceContextService); + // TODO: If we connect async like this, there may be a race condition where messages go missing before the message port is established + this._start(); + } + + private async _start() { + // TODO: Use the direct connection - (async () => { - mark('code/willConnectPtyHost'); - logService.trace('Renderer->PtyHost#connect: before acquirePort'); - const port = await acquirePort('vscode:createPtyHostMessageChannel', 'vscode:createPtyHostMessageChannelResult'); - mark('code/didConnectPtyHost'); - logService.trace('Renderer->PtyHost#connect: connection established'); - - const client = new MessagePortClient(port, `window:${environmentService.window.id}`); - const proxy = ProxyChannel.toService(client.getChannel(TerminalIpcChannels.PtyHostWindow)); - - // Testing - logService.info('latency: ', proxy.getLatency(0)); - proxy.onProcessData(e => { - logService.info('message port process data: ' + e.event); - }); - })(); + mark('code/willConnectPtyHost'); + this._logService.trace('Renderer->PtyHost#connect: before acquirePort'); + const port = await acquirePort('vscode:createPtyHostMessageChannel', 'vscode:createPtyHostMessageChannelResult'); + mark('code/didConnectPtyHost'); + this._logService.trace('Renderer->PtyHost#connect: connection established'); + + const client = new MessagePortClient(port, `window:${this._environmentService.window.id}`); + this._ptyHostDirectProxy = ProxyChannel.toService(client.getChannel(TerminalIpcChannels.PtyHostWindow)); + + // Testing + // this._logService.info('latency: ', this._ptyHostDirectProxy.getLatency(0)); + // this._ptyHostDirectProxy.onProcessData(e => { + // this._logService.info('message port process data: ' + e.event); + // }); // Attach process listeners - this._localPtyService.onProcessData(e => this._ptys.get(e.id)?.handleData(e.event)); - this._localPtyService.onDidChangeProperty(e => this._ptys.get(e.id)?.handleDidChangeProperty(e.property)); - this._localPtyService.onProcessExit(e => { + this._ptyHostDirectProxy!.onProcessData(e => this._ptys.get(e.id)?.handleData(e.event)); + this._ptyHostDirectProxy!.onDidChangeProperty(e => this._ptys.get(e.id)?.handleDidChangeProperty(e.property)); + this._ptyHostDirectProxy!.onProcessExit(e => { const pty = this._ptys.get(e.id); if (pty) { pty.handleExit(e.event); this._ptys.delete(e.id); } }); - this._localPtyService.onProcessReady(e => this._ptys.get(e.id)?.handleReady(e.event)); - this._localPtyService.onProcessReplay(e => this._ptys.get(e.id)?.handleReplay(e.event)); - this._localPtyService.onProcessOrphanQuestion(e => this._ptys.get(e.id)?.handleOrphanQuestion()); - this._localPtyService.onDidRequestDetach(e => this._onDidRequestDetach.fire(e)); + this._ptyHostDirectProxy!.onProcessReady(e => this._ptys.get(e.id)?.handleReady(e.event)); + this._ptyHostDirectProxy!.onProcessReplay(e => this._ptys.get(e.id)?.handleReplay(e.event)); + this._ptyHostDirectProxy!.onProcessOrphanQuestion(e => this._ptys.get(e.id)?.handleOrphanQuestion()); + this._ptyHostDirectProxy!.onDidRequestDetach(e => this._onDidRequestDetach.fire(e)); // Listen for config changes - const initialConfig = configurationService.getValue(TERMINAL_CONFIG_SECTION); + const initialConfig = this._configurationService.getValue(TERMINAL_CONFIG_SECTION); for (const match of Object.keys(initialConfig.autoReplies)) { // Ensure the reply is value const reply = initialConfig.autoReplies[match] as string | null; if (reply) { - this._localPtyService.installAutoReply(match, reply); + this._ptyHostDirectProxy!.installAutoReply(match, reply); } } // TODO: Could simplify update to a single call - this._register(configurationService.onDidChangeConfiguration(async e => { + this._register(this._configurationService.onDidChangeConfiguration(async e => { if (e.affectsConfiguration(TerminalSettingId.AutoReplies)) { - this._localPtyService.uninstallAllAutoReplies(); - const config = configurationService.getValue(TERMINAL_CONFIG_SECTION); + this._ptyHostDirectProxy!.uninstallAllAutoReplies(); + const config = this._configurationService.getValue(TERMINAL_CONFIG_SECTION); for (const match of Object.keys(config.autoReplies)) { // Ensure the reply is value const reply = config.autoReplies[match] as string | null; if (reply) { - await this._localPtyService.installAutoReply(match, reply); + await this._ptyHostDirectProxy!.installAutoReply(match, reply); } } } @@ -134,7 +140,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke } async requestDetachInstance(workspaceId: string, instanceId: number): Promise { - return this._localPtyService.requestDetachInstance(workspaceId, instanceId); + return this._ptyHostDirectProxy!.requestDetachInstance(workspaceId, instanceId); } async acceptDetachInstanceReply(requestId: number, persistentProcessId?: number): Promise { @@ -142,25 +148,25 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke this._logService.warn('Cannot attach to feature terminals, custom pty terminals, or those without a persistentProcessId'); return; } - return this._localPtyService.acceptDetachInstanceReply(requestId, persistentProcessId); + return this._ptyHostDirectProxy!.acceptDetachInstanceReply(requestId, persistentProcessId); } async persistTerminalState(): Promise { const ids = Array.from(this._ptys.keys()); - const serialized = await this._localPtyService.serializeTerminalState(ids); + const serialized = await this._ptyHostDirectProxy!.serializeTerminalState(ids); this._storageService.store(TerminalStorageKeys.TerminalBufferState, serialized, StorageScope.WORKSPACE, StorageTarget.MACHINE); } async updateTitle(id: number, title: string, titleSource: TitleEventSource): Promise { - await this._localPtyService.updateTitle(id, title, titleSource); + await this._ptyHostDirectProxy!.updateTitle(id, title, titleSource); } async updateIcon(id: number, userInitiated: boolean, icon: URI | { light: URI; dark: URI } | { id: string; color?: { id: string } }, color?: string): Promise { - await this._localPtyService.updateIcon(id, userInitiated, icon, color); + await this._ptyHostDirectProxy!.updateIcon(id, userInitiated, icon, color); } updateProperty(id: number, property: ProcessPropertyType, value: IProcessPropertyMap[T]): Promise { - return this._localPtyService.updateProperty(id, property, value); + return this._ptyHostDirectProxy!.updateProperty(id, property, value); } async createProcess( @@ -174,7 +180,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke shouldPersist: boolean ): Promise { const executableEnv = await this._shellEnvironmentService.getShellEnv(); - const id = await this._localPtyService.createProcess(shellLaunchConfig, cwd, cols, rows, unicodeVersion, env, executableEnv, options, shouldPersist, this._getWorkspaceId(), this._getWorkspaceName()); + const id = await this._ptyHostDirectProxy!.createProcess(shellLaunchConfig, cwd, cols, rows, unicodeVersion, env, executableEnv, options, shouldPersist, this._getWorkspaceId(), this._getWorkspaceName()); const pty = this._instantiationService.createInstance(LocalPty, id, shouldPersist); this._ptys.set(id, pty); return pty; @@ -182,7 +188,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke async attachToProcess(id: number): Promise { try { - await this._localPtyService.attachToProcess(id); + await this._ptyHostDirectProxy!.attachToProcess(id); const pty = this._instantiationService.createInstance(LocalPty, id, true); this._ptys.set(id, pty); return pty; @@ -194,7 +200,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke async attachToRevivedProcess(id: number): Promise { try { - const newId = await this._localPtyService.getRevivedPtyNewId(id) ?? id; + const newId = await this._ptyHostDirectProxy!.getRevivedPtyNewId(id) ?? id; return await this.attachToProcess(newId); } catch (e) { this._logService.warn(`Couldn't attach to process ${e.message}`); @@ -203,23 +209,24 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke } async listProcesses(): Promise { - return this._localPtyService.listProcesses(); + return this._ptyHostDirectProxy!.listProcesses(); } async reduceConnectionGraceTime(): Promise { - this._localPtyService.reduceConnectionGraceTime(); + this._ptyHostDirectProxy!.reduceConnectionGraceTime(); } async getDefaultSystemShell(osOverride?: OperatingSystem): Promise { - return this._localPtyService.getDefaultSystemShell(osOverride); + return this._ptyHostDirectProxy!.getDefaultSystemShell(osOverride); } async getProfiles(profiles: unknown, defaultProfile: unknown, includeDetectedProfiles?: boolean) { + // TODO: Differentiate interfaces of direct to pty host and pty host service (or just move them all to pty host) return this._localPtyService.getProfiles?.(this._workspaceContextService.getWorkspace().id, profiles, defaultProfile, includeDetectedProfiles) || []; } async getEnvironment(): Promise { - return this._localPtyService.getEnvironment(); + return this._ptyHostDirectProxy!.getEnvironment(); } async getShellEnvironment(): Promise { @@ -227,7 +234,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke } async getWslPath(original: string, direction: 'unix-to-win' | 'win-to-unix'): Promise { - return this._localPtyService.getWslPath(original, direction); + return this._ptyHostDirectProxy!.getWslPath(original, direction); } async setTerminalLayoutInfo(layoutInfo?: ITerminalsLayoutInfoById): Promise { @@ -235,7 +242,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke workspaceId: this._getWorkspaceId(), tabs: layoutInfo ? layoutInfo.tabs : [] }; - await this._localPtyService.setTerminalLayoutInfo(args); + await this._ptyHostDirectProxy!.setTerminalLayoutInfo(args); // Store in the storage service as well to be used when reviving processes as normally this // is stored in memory on the pty host this._storageService.store(TerminalStorageKeys.TerminalLayoutInfo, JSON.stringify(args), StorageScope.WORKSPACE, StorageTarget.MACHINE); From 7f832ccb3118c3cb7e10189a7354739bde4a207d Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Mon, 15 May 2023 07:03:44 -0700 Subject: [PATCH 06/10] Clean up --- src/vs/platform/terminal/common/terminal.ts | 4 +++- src/vs/platform/terminal/node/ptyHostService.ts | 5 ++--- .../terminal/electron-sandbox/localTerminalBackend.ts | 7 ------- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/vs/platform/terminal/common/terminal.ts b/src/vs/platform/terminal/common/terminal.ts index b7e5c4dabad0b..0b14de96e2cc1 100644 --- a/src/vs/platform/terminal/common/terminal.ts +++ b/src/vs/platform/terminal/common/terminal.ts @@ -207,7 +207,9 @@ export enum TerminalIpcChannels { * Communicates between the shared process and the pty host process. */ PtyHost = 'ptyHost', - + /** + * Communicates between the renderer process and the pty host process. + */ PtyHostWindow = 'ptyHostWindow', /** * Deals with logging from the pty host process. diff --git a/src/vs/platform/terminal/node/ptyHostService.ts b/src/vs/platform/terminal/node/ptyHostService.ts index fac306eb60633..283acb584219b 100644 --- a/src/vs/platform/terminal/node/ptyHostService.ts +++ b/src/vs/platform/terminal/node/ptyHostService.ts @@ -36,10 +36,9 @@ let lastPtyId = 0; export class PtyHostService extends Disposable implements IPtyService { declare readonly _serviceBrand: undefined; - // TODO: Avoid ! ? - private _connection!: IPtyHostConnection; + private _connection: IPtyHostConnection; // ProxyChannel is not used here because events get lost when forwarding across multiple proxies - private _proxy!: IPtyService; + private _proxy: IPtyService; private readonly _shellEnv: Promise; private readonly _resolveVariablesRequestStore: RequestStore; diff --git a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts index 66c8acc836020..be9ed52952f1f 100644 --- a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts +++ b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts @@ -65,7 +65,6 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke @IShellEnvironmentService private readonly _shellEnvironmentService: IShellEnvironmentService, @IStorageService private readonly _storageService: IStorageService, @IConfigurationResolverService private readonly _configurationResolverService: IConfigurationResolverService, - @IConfigurationService configurationService: IConfigurationService, @IConfigurationService private readonly _configurationService: IConfigurationService, @IProductService private readonly _productService: IProductService, @IHistoryService private readonly _historyService: IHistoryService, @@ -93,12 +92,6 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke const client = new MessagePortClient(port, `window:${this._environmentService.window.id}`); this._ptyHostDirectProxy = ProxyChannel.toService(client.getChannel(TerminalIpcChannels.PtyHostWindow)); - // Testing - // this._logService.info('latency: ', this._ptyHostDirectProxy.getLatency(0)); - // this._ptyHostDirectProxy.onProcessData(e => { - // this._logService.info('message port process data: ' + e.event); - // }); - // Attach process listeners this._ptyHostDirectProxy!.onProcessData(e => this._ptys.get(e.id)?.handleData(e.event)); this._ptyHostDirectProxy!.onDidChangeProperty(e => this._ptys.get(e.id)?.handleDidChangeProperty(e.property)); From 2fbfcb1f72d0fd55a2c929de56d1eef61a0ce8c8 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Mon, 15 May 2023 07:12:40 -0700 Subject: [PATCH 07/10] Call ctor with port so proxy is non-undefined --- src/vs/platform/terminal/common/terminal.ts | 2 +- .../platform/terminal/node/ptyHostService.ts | 4 +- .../electron-sandbox/localTerminalBackend.ts | 87 ++++++++++--------- 3 files changed, 47 insertions(+), 46 deletions(-) diff --git a/src/vs/platform/terminal/common/terminal.ts b/src/vs/platform/terminal/common/terminal.ts index 0b14de96e2cc1..ab9be47449384 100644 --- a/src/vs/platform/terminal/common/terminal.ts +++ b/src/vs/platform/terminal/common/terminal.ts @@ -273,7 +273,7 @@ export interface IPtyHostController { readonly onPtyHostResponsive?: Event; readonly onPtyHostRequestResolveVariables?: Event; - restartPtyHost?(): Promise; + restartPtyHost?(): void; acceptPtyHostResolvedVariables?(requestId: number, resolved: string[]): Promise; } diff --git a/src/vs/platform/terminal/node/ptyHostService.ts b/src/vs/platform/terminal/node/ptyHostService.ts index 283acb584219b..79e556bb1399f 100644 --- a/src/vs/platform/terminal/node/ptyHostService.ts +++ b/src/vs/platform/terminal/node/ptyHostService.ts @@ -312,9 +312,9 @@ export class PtyHostService extends Disposable implements IPtyService { } async restartPtyHost(): Promise { - this._isResponsive = true; this._disposePtyHost(); - [this._connection, this._proxy] = await this._startPtyHost(); + this._isResponsive = true; + [this._connection, this._proxy] = this._startPtyHost(); } private _disposePtyHost(): void { diff --git a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts index be9ed52952f1f..47c5be0617d43 100644 --- a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts +++ b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts @@ -39,24 +39,34 @@ import { mark } from 'vs/base/common/performance'; export class LocalTerminalBackendContribution implements IWorkbenchContribution { constructor( @IInstantiationService instantiationService: IInstantiationService, + @ILogService logService: ILogService, @ITerminalInstanceService terminalInstanceService: ITerminalInstanceService ) { - const backend = instantiationService.createInstance(LocalTerminalBackend, undefined); - Registry.as(TerminalExtensions.Backend).registerTerminalBackend(backend); - terminalInstanceService.didRegisterBackend(backend.remoteAuthority); + mark('code/willConnectPtyHost'); + logService.trace('Renderer->PtyHost#connect: before acquirePort'); + acquirePort('vscode:createPtyHostMessageChannel', 'vscode:createPtyHostMessageChannelResult').then(port => { + mark('code/didConnectPtyHost'); + logService.trace('Renderer->PtyHost#connect: connection established'); + + const backend = instantiationService.createInstance(LocalTerminalBackend, port); + Registry.as(TerminalExtensions.Backend).registerTerminalBackend(backend); + terminalInstanceService.didRegisterBackend(backend.remoteAuthority); + }); } } class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBackend { + readonly remoteAuthority = undefined; + private readonly _ptys: Map = new Map(); - private _ptyHostDirectProxy: IPtyService | undefined; + private _ptyHostDirectProxy: IPtyService; private readonly _onDidRequestDetach = this._register(new Emitter<{ requestId: number; workspaceId: string; instanceId: number }>()); readonly onDidRequestDetach = this._onDidRequestDetach.event; constructor( - readonly remoteAuthority: string | undefined, + port: MessagePort, @IInstantiationService private readonly _instantiationService: IInstantiationService, @IWorkspaceContextService workspaceContextService: IWorkspaceContextService, @ILogService logService: ILogService, @@ -76,36 +86,27 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke ) { super(_localPtyService, logService, notificationService, historyService, _configurationResolverService, workspaceContextService); - // TODO: If we connect async like this, there may be a race condition where messages go missing before the message port is established - this._start(); - } - - private async _start() { - - // TODO: Use the direct connection - mark('code/willConnectPtyHost'); - this._logService.trace('Renderer->PtyHost#connect: before acquirePort'); - const port = await acquirePort('vscode:createPtyHostMessageChannel', 'vscode:createPtyHostMessageChannelResult'); - mark('code/didConnectPtyHost'); - this._logService.trace('Renderer->PtyHost#connect: connection established'); - + // There are two connections to the pty host; one to the regular shared process + // _localPtyService, and one directly via message port _ptyHostDirectProxy. The former is + // used for pty host management messages, it would make sense in the future to use a + // separate interface/service for this one. const client = new MessagePortClient(port, `window:${this._environmentService.window.id}`); this._ptyHostDirectProxy = ProxyChannel.toService(client.getChannel(TerminalIpcChannels.PtyHostWindow)); // Attach process listeners - this._ptyHostDirectProxy!.onProcessData(e => this._ptys.get(e.id)?.handleData(e.event)); - this._ptyHostDirectProxy!.onDidChangeProperty(e => this._ptys.get(e.id)?.handleDidChangeProperty(e.property)); - this._ptyHostDirectProxy!.onProcessExit(e => { + this._ptyHostDirectProxy.onProcessData(e => this._ptys.get(e.id)?.handleData(e.event)); + this._ptyHostDirectProxy.onDidChangeProperty(e => this._ptys.get(e.id)?.handleDidChangeProperty(e.property)); + this._ptyHostDirectProxy.onProcessExit(e => { const pty = this._ptys.get(e.id); if (pty) { pty.handleExit(e.event); this._ptys.delete(e.id); } }); - this._ptyHostDirectProxy!.onProcessReady(e => this._ptys.get(e.id)?.handleReady(e.event)); - this._ptyHostDirectProxy!.onProcessReplay(e => this._ptys.get(e.id)?.handleReplay(e.event)); - this._ptyHostDirectProxy!.onProcessOrphanQuestion(e => this._ptys.get(e.id)?.handleOrphanQuestion()); - this._ptyHostDirectProxy!.onDidRequestDetach(e => this._onDidRequestDetach.fire(e)); + this._ptyHostDirectProxy.onProcessReady(e => this._ptys.get(e.id)?.handleReady(e.event)); + this._ptyHostDirectProxy.onProcessReplay(e => this._ptys.get(e.id)?.handleReplay(e.event)); + this._ptyHostDirectProxy.onProcessOrphanQuestion(e => this._ptys.get(e.id)?.handleOrphanQuestion()); + this._ptyHostDirectProxy.onDidRequestDetach(e => this._onDidRequestDetach.fire(e)); // Listen for config changes const initialConfig = this._configurationService.getValue(TERMINAL_CONFIG_SECTION); @@ -113,19 +114,19 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke // Ensure the reply is value const reply = initialConfig.autoReplies[match] as string | null; if (reply) { - this._ptyHostDirectProxy!.installAutoReply(match, reply); + this._ptyHostDirectProxy.installAutoReply(match, reply); } } // TODO: Could simplify update to a single call this._register(this._configurationService.onDidChangeConfiguration(async e => { if (e.affectsConfiguration(TerminalSettingId.AutoReplies)) { - this._ptyHostDirectProxy!.uninstallAllAutoReplies(); + this._ptyHostDirectProxy.uninstallAllAutoReplies(); const config = this._configurationService.getValue(TERMINAL_CONFIG_SECTION); for (const match of Object.keys(config.autoReplies)) { // Ensure the reply is value const reply = config.autoReplies[match] as string | null; if (reply) { - await this._ptyHostDirectProxy!.installAutoReply(match, reply); + await this._ptyHostDirectProxy.installAutoReply(match, reply); } } } @@ -133,7 +134,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke } async requestDetachInstance(workspaceId: string, instanceId: number): Promise { - return this._ptyHostDirectProxy!.requestDetachInstance(workspaceId, instanceId); + return this._ptyHostDirectProxy.requestDetachInstance(workspaceId, instanceId); } async acceptDetachInstanceReply(requestId: number, persistentProcessId?: number): Promise { @@ -141,25 +142,25 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke this._logService.warn('Cannot attach to feature terminals, custom pty terminals, or those without a persistentProcessId'); return; } - return this._ptyHostDirectProxy!.acceptDetachInstanceReply(requestId, persistentProcessId); + return this._ptyHostDirectProxy.acceptDetachInstanceReply(requestId, persistentProcessId); } async persistTerminalState(): Promise { const ids = Array.from(this._ptys.keys()); - const serialized = await this._ptyHostDirectProxy!.serializeTerminalState(ids); + const serialized = await this._ptyHostDirectProxy.serializeTerminalState(ids); this._storageService.store(TerminalStorageKeys.TerminalBufferState, serialized, StorageScope.WORKSPACE, StorageTarget.MACHINE); } async updateTitle(id: number, title: string, titleSource: TitleEventSource): Promise { - await this._ptyHostDirectProxy!.updateTitle(id, title, titleSource); + await this._ptyHostDirectProxy.updateTitle(id, title, titleSource); } async updateIcon(id: number, userInitiated: boolean, icon: URI | { light: URI; dark: URI } | { id: string; color?: { id: string } }, color?: string): Promise { - await this._ptyHostDirectProxy!.updateIcon(id, userInitiated, icon, color); + await this._ptyHostDirectProxy.updateIcon(id, userInitiated, icon, color); } updateProperty(id: number, property: ProcessPropertyType, value: IProcessPropertyMap[T]): Promise { - return this._ptyHostDirectProxy!.updateProperty(id, property, value); + return this._ptyHostDirectProxy.updateProperty(id, property, value); } async createProcess( @@ -173,7 +174,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke shouldPersist: boolean ): Promise { const executableEnv = await this._shellEnvironmentService.getShellEnv(); - const id = await this._ptyHostDirectProxy!.createProcess(shellLaunchConfig, cwd, cols, rows, unicodeVersion, env, executableEnv, options, shouldPersist, this._getWorkspaceId(), this._getWorkspaceName()); + const id = await this._ptyHostDirectProxy.createProcess(shellLaunchConfig, cwd, cols, rows, unicodeVersion, env, executableEnv, options, shouldPersist, this._getWorkspaceId(), this._getWorkspaceName()); const pty = this._instantiationService.createInstance(LocalPty, id, shouldPersist); this._ptys.set(id, pty); return pty; @@ -181,7 +182,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke async attachToProcess(id: number): Promise { try { - await this._ptyHostDirectProxy!.attachToProcess(id); + await this._ptyHostDirectProxy.attachToProcess(id); const pty = this._instantiationService.createInstance(LocalPty, id, true); this._ptys.set(id, pty); return pty; @@ -193,7 +194,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke async attachToRevivedProcess(id: number): Promise { try { - const newId = await this._ptyHostDirectProxy!.getRevivedPtyNewId(id) ?? id; + const newId = await this._ptyHostDirectProxy.getRevivedPtyNewId(id) ?? id; return await this.attachToProcess(newId); } catch (e) { this._logService.warn(`Couldn't attach to process ${e.message}`); @@ -202,15 +203,15 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke } async listProcesses(): Promise { - return this._ptyHostDirectProxy!.listProcesses(); + return this._ptyHostDirectProxy.listProcesses(); } async reduceConnectionGraceTime(): Promise { - this._ptyHostDirectProxy!.reduceConnectionGraceTime(); + this._ptyHostDirectProxy.reduceConnectionGraceTime(); } async getDefaultSystemShell(osOverride?: OperatingSystem): Promise { - return this._ptyHostDirectProxy!.getDefaultSystemShell(osOverride); + return this._ptyHostDirectProxy.getDefaultSystemShell(osOverride); } async getProfiles(profiles: unknown, defaultProfile: unknown, includeDetectedProfiles?: boolean) { @@ -219,7 +220,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke } async getEnvironment(): Promise { - return this._ptyHostDirectProxy!.getEnvironment(); + return this._ptyHostDirectProxy.getEnvironment(); } async getShellEnvironment(): Promise { @@ -227,7 +228,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke } async getWslPath(original: string, direction: 'unix-to-win' | 'win-to-unix'): Promise { - return this._ptyHostDirectProxy!.getWslPath(original, direction); + return this._ptyHostDirectProxy.getWslPath(original, direction); } async setTerminalLayoutInfo(layoutInfo?: ITerminalsLayoutInfoById): Promise { @@ -235,7 +236,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke workspaceId: this._getWorkspaceId(), tabs: layoutInfo ? layoutInfo.tabs : [] }; - await this._ptyHostDirectProxy!.setTerminalLayoutInfo(args); + await this._ptyHostDirectProxy.setTerminalLayoutInfo(args); // Store in the storage service as well to be used when reviving processes as normally this // is stored in memory on the pty host this._storageService.store(TerminalStorageKeys.TerminalLayoutInfo, JSON.stringify(args), StorageScope.WORKSPACE, StorageTarget.MACHINE); From a12c41258f6f05ce5e6388521b5260409c97db9c Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Mon, 15 May 2023 14:02:02 -0700 Subject: [PATCH 08/10] Start pty host when the first window opens --- .../electron-main/electronPtyHostStarter.ts | 11 +++-- src/vs/platform/terminal/node/ptyHost.ts | 2 + .../platform/terminal/node/ptyHostService.ts | 47 ++++++++++++++----- 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts index 17ed44d13bb12..b037d6ef4144d 100644 --- a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts +++ b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts @@ -15,17 +15,23 @@ import { Client as MessagePortClient } from 'vs/base/parts/ipc/electron-main/ipc import { IpcMainEvent } from 'electron'; import { validatedIpcMain } from 'vs/base/parts/ipc/electron-main/ipcMain'; import { DisposableStore, toDisposable } from 'vs/base/common/lifecycle'; +import { Emitter } from 'vs/base/common/event'; export class ElectronPtyHostStarter implements IPtyHostStarter { private utilityProcess: UtilityProcess | undefined = undefined; + private readonly _onBeforeWindowConnection = new Emitter(); + readonly onBeforeWindowConnection = this._onBeforeWindowConnection.event; + constructor( private readonly _reconnectConstants: IReconnectConstants, @IEnvironmentService private readonly _environmentService: INativeEnvironmentService, @ILifecycleMainService private readonly _lifecycleMainService: ILifecycleMainService, @ILogService private readonly _logService: ILogService ) { + // Listen for new windows to establish connection directly to pty host + validatedIpcMain.on('vscode:createPtyHostMessageChannel', (e, nonce) => this._onWindowConnection(e, nonce)); } start(lastPtyId: number): IPtyHostConnection { @@ -52,9 +58,6 @@ export class ElectronPtyHostStarter implements IPtyHostStarter { const port = this.utilityProcess.connect(); const client = new MessagePortClient(port, 'ptyHost'); - // Listen for new windows to establish connection directly to pty host - validatedIpcMain.on('vscode:createPtyHostMessageChannel', (e, nonce) => this._onWindowConnection(e, nonce)); - // TODO: Do we need to listen for window close to close the port? const store = new DisposableStore(); @@ -85,6 +88,8 @@ export class ElectronPtyHostStarter implements IPtyHostStarter { } private _onWindowConnection(e: IpcMainEvent, nonce: string) { + this._onBeforeWindowConnection.fire(); + const port = this.utilityProcess!.connect(); // Check back if the requesting window meanwhile closed diff --git a/src/vs/platform/terminal/node/ptyHost.ts b/src/vs/platform/terminal/node/ptyHost.ts index c6b0013f64b2b..3fa67754722b6 100644 --- a/src/vs/platform/terminal/node/ptyHost.ts +++ b/src/vs/platform/terminal/node/ptyHost.ts @@ -14,6 +14,8 @@ export interface IPtyHostConnection { } export interface IPtyHostStarter { + onBeforeWindowConnection?: Event; + /** * Creates a pty host and connects to it. * diff --git a/src/vs/platform/terminal/node/ptyHostService.ts b/src/vs/platform/terminal/node/ptyHostService.ts index 79e556bb1399f..730635792e2ef 100644 --- a/src/vs/platform/terminal/node/ptyHostService.ts +++ b/src/vs/platform/terminal/node/ptyHostService.ts @@ -36,9 +36,24 @@ let lastPtyId = 0; export class PtyHostService extends Disposable implements IPtyService { declare readonly _serviceBrand: undefined; - private _connection: IPtyHostConnection; + private __connection?: IPtyHostConnection; // ProxyChannel is not used here because events get lost when forwarding across multiple proxies - private _proxy: IPtyService; + private __proxy?: IPtyService; + + private get _connection(): IPtyHostConnection { + this._ensurePtyHost(); + return this.__connection!; + } + private get _proxy(): IPtyService { + this._ensurePtyHost(); + return this.__proxy!; + } + + private _ensurePtyHost() { + if (!this.__connection) { + [this.__connection, this.__proxy] = this._startPtyHost(); + } + } private readonly _shellEnv: Promise; private readonly _resolveVariablesRequestStore: RequestStore; @@ -93,14 +108,13 @@ export class PtyHostService extends Disposable implements IPtyService { this._resolveVariablesRequestStore = this._register(new RequestStore(undefined, this._logService)); this._resolveVariablesRequestStore.onCreateRequest(this._onPtyHostRequestResolveVariables.fire, this._onPtyHostRequestResolveVariables); - [this._connection, this._proxy] = this._startPtyHost(); - - this._register(this._configurationService.onDidChangeConfiguration(async e => { - if (e.affectsConfiguration(TerminalSettingId.IgnoreProcessNames)) { - await this._refreshIgnoreProcessNames(); - } - })); - this._refreshIgnoreProcessNames(); + // Force the pty host to start as the first window is starting if the starter has that + // capability + if (this._ptyHostStarter.onBeforeWindowConnection) { + Event.once(this._ptyHostStarter.onBeforeWindowConnection)(() => this._ensurePtyHost()); + } else { + this._ensurePtyHost(); + } } private get _ignoreProcessNames(): string[] { @@ -126,6 +140,7 @@ export class PtyHostService extends Disposable implements IPtyService { } private _startPtyHost(): [IPtyHostConnection, IPtyService] { + this._logService.info('startPtyHost', new Error().stack); const connection = this._ptyHostStarter.start(lastPtyId); const client = connection.client; @@ -166,6 +181,16 @@ export class PtyHostService extends Disposable implements IPtyService { this._register(new RemoteLoggerChannelClient(this._loggerService, client.getChannel(TerminalIpcChannels.Logger))); }); + this.__connection = connection; + this.__proxy = proxy; + + this._register(this._configurationService.onDidChangeConfiguration(async e => { + if (e.affectsConfiguration(TerminalSettingId.IgnoreProcessNames)) { + await this._refreshIgnoreProcessNames(); + } + })); + this._refreshIgnoreProcessNames(); + return [connection, proxy]; } @@ -314,7 +339,7 @@ export class PtyHostService extends Disposable implements IPtyService { async restartPtyHost(): Promise { this._disposePtyHost(); this._isResponsive = true; - [this._connection, this._proxy] = this._startPtyHost(); + this._startPtyHost(); } private _disposePtyHost(): void { From 78cdc57db20740f87a732962391162d256198d24 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Mon, 15 May 2023 14:04:07 -0700 Subject: [PATCH 09/10] Remove comment about closing window port The clean up is handled by Electron --- .../platform/terminal/electron-main/electronPtyHostStarter.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts index b037d6ef4144d..420a1760fecac 100644 --- a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts +++ b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts @@ -58,8 +58,6 @@ export class ElectronPtyHostStarter implements IPtyHostStarter { const port = this.utilityProcess.connect(); const client = new MessagePortClient(port, 'ptyHost'); - // TODO: Do we need to listen for window close to close the port? - const store = new DisposableStore(); store.add(client); store.add(this.utilityProcess); From 1b5e250f4e9d90b6c612c7d9b9eb3588143ac2ab Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Mon, 15 May 2023 14:16:42 -0700 Subject: [PATCH 10/10] Don't restart pty host when shutting down --- .../terminal/electron-main/electronPtyHostStarter.ts | 3 +++ src/vs/platform/terminal/node/ptyHost.ts | 1 + src/vs/platform/terminal/node/ptyHostService.ts | 7 +++++-- .../contrib/terminal/browser/baseTerminalBackend.ts | 1 - 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts index 420a1760fecac..92342be0f8de1 100644 --- a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts +++ b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts @@ -23,6 +23,8 @@ export class ElectronPtyHostStarter implements IPtyHostStarter { private readonly _onBeforeWindowConnection = new Emitter(); readonly onBeforeWindowConnection = this._onBeforeWindowConnection.event; + private readonly _onWillShutdown = new Emitter(); + readonly onWillShutdown = this._onWillShutdown.event; constructor( private readonly _reconnectConstants: IReconnectConstants, @@ -30,6 +32,7 @@ export class ElectronPtyHostStarter implements IPtyHostStarter { @ILifecycleMainService private readonly _lifecycleMainService: ILifecycleMainService, @ILogService private readonly _logService: ILogService ) { + this._lifecycleMainService.onWillShutdown(() => this._onWillShutdown.fire()); // Listen for new windows to establish connection directly to pty host validatedIpcMain.on('vscode:createPtyHostMessageChannel', (e, nonce) => this._onWindowConnection(e, nonce)); } diff --git a/src/vs/platform/terminal/node/ptyHost.ts b/src/vs/platform/terminal/node/ptyHost.ts index 3fa67754722b6..1d7bb92a1059c 100644 --- a/src/vs/platform/terminal/node/ptyHost.ts +++ b/src/vs/platform/terminal/node/ptyHost.ts @@ -15,6 +15,7 @@ export interface IPtyHostConnection { export interface IPtyHostStarter { onBeforeWindowConnection?: Event; + onWillShutdown?: Event; /** * Creates a pty host and connects to it. diff --git a/src/vs/platform/terminal/node/ptyHostService.ts b/src/vs/platform/terminal/node/ptyHostService.ts index 730635792e2ef..57bd8a787231a 100644 --- a/src/vs/platform/terminal/node/ptyHostService.ts +++ b/src/vs/platform/terminal/node/ptyHostService.ts @@ -57,6 +57,7 @@ export class PtyHostService extends Disposable implements IPtyService { private readonly _shellEnv: Promise; private readonly _resolveVariablesRequestStore: RequestStore; + private _wasQuitRequested = false; private _restartCount = 0; private _isResponsive = true; private _isDisposed = false; @@ -105,6 +106,7 @@ export class PtyHostService extends Disposable implements IPtyService { this._register(toDisposable(() => this._disposePtyHost())); + this._resolveVariablesRequestStore = this._register(new RequestStore(undefined, this._logService)); this._resolveVariablesRequestStore.onCreateRequest(this._onPtyHostRequestResolveVariables.fire, this._onPtyHostRequestResolveVariables); @@ -115,6 +117,8 @@ export class PtyHostService extends Disposable implements IPtyService { } else { this._ensurePtyHost(); } + + this._ptyHostStarter.onWillShutdown?.(() => this._wasQuitRequested = true); } private get _ignoreProcessNames(): string[] { @@ -140,7 +144,6 @@ export class PtyHostService extends Disposable implements IPtyService { } private _startPtyHost(): [IPtyHostConnection, IPtyService] { - this._logService.info('startPtyHost', new Error().stack); const connection = this._ptyHostStarter.start(lastPtyId); const client = connection.client; @@ -154,7 +157,7 @@ export class PtyHostService extends Disposable implements IPtyService { // Handle exit this._register(connection.onDidProcessExit(e => { this._onPtyHostExit.fire(e.code); - if (!this._isDisposed) { + if (!this._wasQuitRequested && !this._isDisposed) { if (this._restartCount <= Constants.MaxRestarts) { this._logService.error(`ptyHost terminated unexpectedly with code ${e.code}`); this._restartCount++; diff --git a/src/vs/workbench/contrib/terminal/browser/baseTerminalBackend.ts b/src/vs/workbench/contrib/terminal/browser/baseTerminalBackend.ts index 415eeb5b0789b..f117afcc5d0bd 100644 --- a/src/vs/workbench/contrib/terminal/browser/baseTerminalBackend.ts +++ b/src/vs/workbench/contrib/terminal/browser/baseTerminalBackend.ts @@ -44,7 +44,6 @@ export abstract class BaseTerminalBackend extends Disposable { let unresponsiveNotification: INotificationHandle | undefined; if (eventSource.onPtyHostStart) { this._register(eventSource.onPtyHostStart(() => { - this._logService.info(`ptyHost restarted`); this._onPtyHostRestart.fire(); unresponsiveNotification?.close(); unresponsiveNotification = undefined;