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

Use getDelayedChannel to avoid awaiting promise #182656

Merged
merged 1 commit into from
May 16, 2023
Merged

Use getDelayedChannel to avoid awaiting promise #182656

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
80 changes: 31 additions & 49 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 @@ -33,7 +33,7 @@ import { getWorkspaceForTerminal } from 'vs/workbench/services/configurationReso
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';
import { getDelayedChannel, 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';
Expand All @@ -52,10 +52,8 @@ export class LocalTerminalBackendContribution implements IWorkbenchContribution
class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBackend {
readonly remoteAuthority = undefined;

// HACK: ProxyChannels behave strangely when interacting with promises and seemingly never
// resolve, so _proxyReady is awaited before every proxy use
private _proxy?: IPtyService;
private readonly _proxyReady: DeferredPromise<void>;
private readonly _proxy: IPtyService;
private readonly _clientEventually: DeferredPromise<MessagePortClient> = new DeferredPromise();

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

Expand Down Expand Up @@ -83,7 +81,8 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke
) {
super(_localPtyService, logService, notificationService, historyService, _configurationResolverService, workspaceContextService);

this._proxyReady = new DeferredPromise<void>();
this._proxy = ProxyChannel.toService<IPtyService>(getDelayedChannel(this._clientEventually.p.then(client => client.getChannel(TerminalIpcChannels.PtyHostWindow))));

this._connectToDirectProxy();
}

Expand All @@ -101,44 +100,42 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke
// 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();
this._clientEventually.complete(client);

// 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 => {
this._proxy.onProcessData(e => this._ptys.get(e.id)?.handleData(e.event));
this._proxy.onDidChangeProperty(e => this._ptys.get(e.id)?.handleDidChangeProperty(e.property));
this._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));
this._proxy.onProcessReady(e => this._ptys.get(e.id)?.handleReady(e.event));
this._proxy.onProcessReplay(e => this._ptys.get(e.id)?.handleReplay(e.event));
this._proxy.onProcessOrphanQuestion(e => this._ptys.get(e.id)?.handleOrphanQuestion());
this._proxy.onDidRequestDetach(e => this._onDidRequestDetach.fire(e));
Comment on lines +106 to +118
Copy link
Member

Choose a reason for hiding this comment

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

are these disposables? Do they need to be registered as such?

Copy link
Member Author

Choose a reason for hiding this comment

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

The backend owns the proxy so we could expose that, but we don't currently as the object never actually gets disposed.


// 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);
this._proxy.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();
this._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);
this._proxy.installAutoReply(match, reply);
}
}
}
Expand All @@ -147,39 +144,33 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke
}

async requestDetachInstance(workspaceId: string, instanceId: number): Promise<IProcessDetails | undefined> {
await this._proxyReady.p;
return this._proxy!.requestDetachInstance(workspaceId, instanceId);
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;
}
await this._proxyReady.p;
return this._proxy!.acceptDetachInstanceReply(requestId, persistentProcessId);
return this._proxy.acceptDetachInstanceReply(requestId, persistentProcessId);
}

async persistTerminalState(): Promise<void> {
const ids = Array.from(this._ptys.keys());
await this._proxyReady.p;
const serialized = await this._proxy!.serializeTerminalState(ids);
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._proxyReady.p;
await this._proxy!.updateTitle(id, title, titleSource);
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._proxyReady.p;
await this._proxy!.updateIcon(id, userInitiated, icon, color);
await this._proxy.updateIcon(id, userInitiated, icon, color);
}

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);
return this._proxy.updateProperty(id, property, value);
}

async createProcess(
Expand All @@ -193,17 +184,15 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke
shouldPersist: boolean
): Promise<ITerminalChildProcess> {
const executableEnv = await this._shellEnvironmentService.getShellEnv();
await this._proxyReady.p;
const id = await this._proxy!.createProcess(shellLaunchConfig, cwd, cols, rows, unicodeVersion, env, executableEnv, options, shouldPersist, this._getWorkspaceId(), this._getWorkspaceName());
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._proxyReady.p;
await this._proxy!.attachToProcess(id);
await this._proxy.attachToProcess(id);
const pty = this._instantiationService.createInstance(LocalPty, id, true);
this._ptys.set(id, pty);
return pty;
Expand All @@ -215,8 +204,7 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke

async attachToRevivedProcess(id: number): Promise<ITerminalChildProcess | undefined> {
try {
await this._proxyReady.p;
const newId = await this._proxy!.getRevivedPtyNewId(id) ?? id;
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 @@ -225,18 +213,15 @@ class LocalTerminalBackend extends BaseTerminalBackend implements ITerminalBacke
}

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

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

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

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

async getEnvironment(): Promise<IProcessEnvironment> {
await this._proxyReady.p;
return this._proxy!.getEnvironment();
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> {
await this._proxyReady.p;
return this._proxy!.getWslPath(original, direction);
return this._proxy.getWslPath(original, direction);
}

async setTerminalLayoutInfo(layoutInfo?: ITerminalsLayoutInfoById): Promise<void> {
const args: ISetTerminalLayoutInfoArgs = {
workspaceId: this._getWorkspaceId(),
tabs: layoutInfo ? layoutInfo.tabs : []
};
await this._proxyReady.p;
await this._proxy!.setTerminalLayoutInfo(args);
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