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.

Sign up for GitHub

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

Jump to bottom

Don't connect LocalTerminalBackend until LifecyclePhase.Restored #182631

Merged
merged 1 commit into from
May 16, 2023
Merged

Don't connect LocalTerminalBackend until LifecyclePhase.Restored #182631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
172 changes: 100 additions & 72 deletions src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,40 +35,37 @@ 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';
import { ILifecycleService, LifecyclePhase } from 'vs/workbench/services/lifecycle/common/lifecycle';
import { DeferredPromise } from 'vs/base/common/async';

export class LocalTerminalBackendContribution implements IWorkbenchContribution {
constructor(
@IInstantiationService instantiationService: IInstantiationService,
@ILogService logService: ILogService,
@ITerminalInstanceService terminalInstanceService: ITerminalInstanceService
) {
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<ITerminalBackendRegistry>(TerminalExtensions.Backend).registerTerminalBackend(backend);
terminalInstanceService.didRegisterBackend(backend.remoteAuthority);
});
const backend = instantiationService.createInstance(LocalTerminalBackend);
Registry.as<ITerminalBackendRegistry>(TerminalExtensions.Backend).registerTerminalBackend(backend);
terminalInstanceService.didRegisterBackend(backend.remoteAuthority);
}
}

class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBackend {
readonly remoteAuthority = undefined;

private readonly _ptys: Map<number, LocalPty> = new Map();
// HACK: ProxyChannels behave strangely when interacting with promises and seemingly never
// resolve, so _proxyReady is awaited before every proxy use
private _proxy?: IPtyService;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably follow up on this, I am curious what the issue is.

private readonly _proxyReady: DeferredPromise<void>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could return the proxy so that you do not have to do all the proxy! calls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I did initially, the promise resolve function was definitely called, but .p was always pending. This happened with a regular promise as well. It would be nice to figure out what exactly was going on but I wasted quite a lot of time on it already

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shared process is not using a deferred promise but just this if you want to copy:

vscode/src/vs/workbench/services/sharedProcess/electron-sandbox/sharedProcessService.ts

Lines 19 to 52 in 7a7d457

private readonly withSharedProcessConnection: Promise<MessagePortClient>;
private readonly restoredBarrier = new Barrier();
constructor(
readonly windowId: number,
@ILogService private readonly logService: ILogService
) {
super();
this.withSharedProcessConnection = this.connect();
}
private async connect(): Promise<MessagePortClient> {
this.logService.trace('Renderer->SharedProcess#connect');
// Our performance tests show that a connection to the shared
// process can have significant overhead to the startup time
// of the window because the shared process could be created
// as a result. As such, make sure we await the `Restored`
// phase before making a connection attempt, but also add a
// timeout to be safe against possible deadlocks.
await Promise.race([this.restoredBarrier.wait(), timeout(2000)]);
// Acquire a message port connected to the shared process
mark('code/willConnectSharedProcess');
this.logService.trace('Renderer->SharedProcess#connect: before acquirePort');
const port = await acquirePort('vscode:createSharedProcessMessageChannel', 'vscode:createSharedProcessMessageChannelResult');
mark('code/didConnectSharedProcess');
this.logService.trace('Renderer->SharedProcess#connect: connection established');
return this._register(new MessagePortClient(port, `window:${this.windowId}`));
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah the getDelayedChannel will probably be a fix for my problem

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes!


private _ptyHostDirectProxy: IPtyService;
private readonly _ptys: Map<number, LocalPty> = new Map();

private readonly _onDidRequestDetach = this._register(new Emitter<{ requestId: number; workspaceId: string; instanceId: number }>());
readonly onDidRequestDetach = this._onDidRequestDetach.event;

constructor(
port: MessagePort,
@IInstantiationService private readonly _instantiationService: IInstantiationService,
@IWorkspaceContextService workspaceContextService: IWorkspaceContextService,
@ILifecycleService private readonly _lifecycleService: ILifecycleService,
@ILogService logService: ILogService,
@ILocalPtyService private readonly _localPtyService: ILocalPtyService,
@ILabelService private readonly _labelService: ILabelService,
Expand All @@ -86,81 +83,103 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke
) {
super(_localPtyService, logService, notificationService, historyService, _configurationResolverService, workspaceContextService);

// 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<IPtyService>(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 => {
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));

// Listen for config changes
const initialConfig = this._configurationService.getValue<ITerminalConfiguration>(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._ptyHostDirectProxy.installAutoReply(match, reply);
this._proxyReady = new DeferredPromise<void>();
this._connectToDirectProxy();
}

private async _connectToDirectProxy(): Promise<void> {
// The pty host should not get launched until the first window restored phase
await this._lifecycleService.when(LifecyclePhase.Restored);

mark('code/willConnectPtyHost');
this._logService.trace('Renderer->PtyHost#connect: before acquirePort');
acquirePort('vscode:createPtyHostMessageChannel', 'vscode:createPtyHostMessageChannelResult').then(port => {
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}`);
const proxy = ProxyChannel.toService<IPtyService>(client.getChannel(TerminalIpcChannels.PtyHostWindow));
this._proxy = proxy;
this._proxyReady.complete();

// Attach process listeners
proxy.onProcessData(e => this._ptys.get(e.id)?.handleData(e.event));
proxy.onDidChangeProperty(e => this._ptys.get(e.id)?.handleDidChangeProperty(e.property));
proxy.onProcessExit(e => {
const pty = this._ptys.get(e.id);
if (pty) {
pty.handleExit(e.event);
this._ptys.delete(e.id);
}
});
proxy.onProcessReady(e => this._ptys.get(e.id)?.handleReady(e.event));
proxy.onProcessReplay(e => this._ptys.get(e.id)?.handleReplay(e.event));
proxy.onProcessOrphanQuestion(e => this._ptys.get(e.id)?.handleOrphanQuestion());
proxy.onDidRequestDetach(e => this._onDidRequestDetach.fire(e));

// Listen for config changes
const initialConfig = this._configurationService.getValue<ITerminalConfiguration>(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) {
proxy.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();
const config = this._configurationService.getValue<ITerminalConfiguration>(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);
// TODO: Could simplify update to a single call
this._register(this._configurationService.onDidChangeConfiguration(async e => {
if (e.affectsConfiguration(TerminalSettingId.AutoReplies)) {
proxy.uninstallAllAutoReplies();
const config = this._configurationService.getValue<ITerminalConfiguration>(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) {
proxy.installAutoReply(match, reply);
}
}
}
}
}));
}));
});
}

async requestDetachInstance(workspaceId: string, instanceId: number): Promise<IProcessDetails | undefined> {
return this._ptyHostDirectProxy.requestDetachInstance(workspaceId, instanceId);
await this._proxyReady.p;
return this._proxy!.requestDetachInstance(workspaceId, instanceId);
}

async acceptDetachInstanceReply(requestId: number, persistentProcessId?: number): Promise<void> {
if (!persistentProcessId) {
this._logService.warn('Cannot attach to feature terminals, custom pty terminals, or those without a persistentProcessId');
return;
}
return this._ptyHostDirectProxy.acceptDetachInstanceReply(requestId, persistentProcessId);
await this._proxyReady.p;
return this._proxy!.acceptDetachInstanceReply(requestId, persistentProcessId);
}

async persistTerminalState(): Promise<void> {
const ids = Array.from(this._ptys.keys());
const serialized = await this._ptyHostDirectProxy.serializeTerminalState(ids);
await this._proxyReady.p;
const serialized = await this._proxy!.serializeTerminalState(ids);
this._storageService.store(TerminalStorageKeys.TerminalBufferState, serialized, StorageScope.WORKSPACE, StorageTarget.MACHINE);
}

async updateTitle(id: number, title: string, titleSource: TitleEventSource): Promise<void> {
await this._ptyHostDirectProxy.updateTitle(id, title, titleSource);
await this._proxyReady.p;
await this._proxy!.updateTitle(id, title, titleSource);
}

async updateIcon(id: number, userInitiated: boolean, icon: URI | { light: URI; dark: URI } | { id: string; color?: { id: string } }, color?: string): Promise<void> {
await this._ptyHostDirectProxy.updateIcon(id, userInitiated, icon, color);
await this._proxyReady.p;
await this._proxy!.updateIcon(id, userInitiated, icon, color);
}

updateProperty<T extends ProcessPropertyType>(id: number, property: ProcessPropertyType, value: IProcessPropertyMap[T]): Promise<void> {
return this._ptyHostDirectProxy.updateProperty(id, property, value);
async updateProperty<T extends ProcessPropertyType>(id: number, property: ProcessPropertyType, value: IProcessPropertyMap[T]): Promise<void> {
await this._proxyReady.p;
return this._proxy!.updateProperty(id, property, value);
}

async createProcess(
Expand All @@ -174,15 +193,17 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke
shouldPersist: boolean
): Promise<ITerminalChildProcess> {
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());
await this._proxyReady.p;
const id = await this._proxy!.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;
}

async attachToProcess(id: number): Promise<ITerminalChildProcess | undefined> {
try {
await this._ptyHostDirectProxy.attachToProcess(id);
await this._proxyReady.p;
await this._proxy!.attachToProcess(id);
const pty = this._instantiationService.createInstance(LocalPty, id, true);
this._ptys.set(id, pty);
return pty;
Expand All @@ -194,7 +215,8 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke

async attachToRevivedProcess(id: number): Promise<ITerminalChildProcess | undefined> {
try {
const newId = await this._ptyHostDirectProxy.getRevivedPtyNewId(id) ?? id;
await this._proxyReady.p;
const newId = await this._proxy!.getRevivedPtyNewId(id) ?? id;
return await this.attachToProcess(newId);
} catch (e) {
this._logService.warn(`Couldn't attach to process ${e.message}`);
Expand All @@ -203,15 +225,18 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke
}

async listProcesses(): Promise<IProcessDetails[]> {
return this._ptyHostDirectProxy.listProcesses();
await this._proxyReady.p;
return this._proxy!.listProcesses();
}

async reduceConnectionGraceTime(): Promise<void> {
this._ptyHostDirectProxy.reduceConnectionGraceTime();
await this._proxyReady.p;
this._proxy!.reduceConnectionGraceTime();
}

async getDefaultSystemShell(osOverride?: OperatingSystem): Promise<string> {
return this._ptyHostDirectProxy.getDefaultSystemShell(osOverride);
await this._proxyReady;
return this._proxy!.getDefaultSystemShell(osOverride);
}

async getProfiles(profiles: unknown, defaultProfile: unknown, includeDetectedProfiles?: boolean) {
Expand All @@ -220,23 +245,26 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke
}

async getEnvironment(): Promise<IProcessEnvironment> {
return this._ptyHostDirectProxy.getEnvironment();
await this._proxyReady.p;
return this._proxy!.getEnvironment();
}

async getShellEnvironment(): Promise<IProcessEnvironment> {
return this._shellEnvironmentService.getShellEnv();
}

async getWslPath(original: string, direction: 'unix-to-win' | 'win-to-unix'): Promise<string> {
return this._ptyHostDirectProxy.getWslPath(original, direction);
await this._proxyReady.p;
return this._proxy!.getWslPath(original, direction);
}

async setTerminalLayoutInfo(layoutInfo?: ITerminalsLayoutInfoById): Promise<void> {
const args: ISetTerminalLayoutInfoArgs = {
workspaceId: this._getWorkspaceId(),
tabs: layoutInfo ? layoutInfo.tabs : []
};
await this._ptyHostDirectProxy.setTerminalLayoutInfo(args);
await this._proxyReady.p;
await this._proxy!.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);
Expand Down
2 changes: 0 additions & 2 deletions src/vs/workbench/contrib/terminal/electron-sandbox/terminalProfileResolverService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { BaseTerminalProfileResolverService } from 'vs/workbench/contrib/termina
import { ITerminalProfileService } from 'vs/workbench/contrib/terminal/common/terminal';
import { IConfigurationResolverService } from 'vs/workbench/services/configurationResolver/common/configurationResolver';
import { IHistoryService } from 'vs/workbench/services/history/common/history';
import { ILifecycleService } from 'vs/workbench/services/lifecycle/common/lifecycle';
import { IRemoteAgentService } from 'vs/workbench/services/remote/common/remoteAgentService';

export class ElectronTerminalProfileResolverService extends BaseTerminalProfileResolverService {
Expand All @@ -23,7 +22,6 @@ export class ElectronTerminalProfileResolverService extends BaseTerminalProfileR
@IConfigurationResolverService configurationResolverService: IConfigurationResolverService,
@IConfigurationService configurationService: IConfigurationService,
@IHistoryService historyService: IHistoryService,
@ILifecycleService lifecycleService: ILifecycleService,
@ILogService logService: ILogService,
@IWorkspaceContextService workspaceContextService: IWorkspaceContextService,
@ITerminalProfileService terminalProfileService: ITerminalProfileService,
Expand Down