From 51bbf51f0f590721ef83207bc896674a79dc86fb Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 16 Jul 2021 08:41:05 -0700 Subject: [PATCH] Add RequestStore helper, adopt in ptyhost and add timeout support (#128793) * Create helper class for event with reply pattern in terminal Part of #128786 * Adopt RequestStore in pty host * Support timeout in RequestStore Fixes #128786 * Docs * Move request store to common * Add request store test * Undo bad changes to unrelated files --- .../platform/terminal/common/requestStore.ts | 68 +++++++++++++++++++ src/vs/platform/terminal/common/terminal.ts | 6 +- .../platform/terminal/node/ptyHostService.ts | 26 +++---- src/vs/platform/terminal/node/ptyService.ts | 30 ++++---- .../terminal/test/common/requestStore.test.ts | 44 ++++++++++++ .../terminal/browser/remoteTerminalService.ts | 4 +- .../terminal/common/remoteTerminalChannel.ts | 6 +- .../contrib/terminal/common/terminal.ts | 2 +- .../electron-sandbox/localTerminalService.ts | 6 +- .../test/browser/workbenchTestServices.ts | 2 +- 10 files changed, 146 insertions(+), 48 deletions(-) create mode 100644 src/vs/platform/terminal/common/requestStore.ts create mode 100644 src/vs/platform/terminal/test/common/requestStore.test.ts diff --git a/src/vs/platform/terminal/common/requestStore.ts b/src/vs/platform/terminal/common/requestStore.ts new file mode 100644 index 00000000000..f998cd064c9 --- /dev/null +++ b/src/vs/platform/terminal/common/requestStore.ts @@ -0,0 +1,68 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { timeout } from 'vs/base/common/async'; +import { CancellationTokenSource } from 'vs/base/common/cancellation'; +import { Emitter } from 'vs/base/common/event'; +import { Disposable, dispose, toDisposable, IDisposable } from 'vs/base/common/lifecycle'; +import { ILogService } from 'vs/platform/log/common/log'; + +/** + * A helper class to track requests that have replies. Using this it's easy to implement an event + * that accepts a reply. + */ +export class RequestStore extends Disposable { + private _lastRequestId = 0; + private readonly _timeout: number; + private _pendingRequests: Map void> = new Map(); + private _pendingRequestDisposables: Map = new Map(); + + private readonly _onCreateRequest = this._register(new Emitter()); + readonly onCreateRequest = this._onCreateRequest.event; + + /** + * @param timeout How long in ms to allow requests to go unanswered for, undefined will use the + * default (15 seconds). + */ + constructor( + timeout: number | undefined, + @ILogService private readonly _logService: ILogService + ) { + super(); + this._timeout = timeout === undefined ? 15000 : timeout; + } + + /** + * Creates a request. + * @param args The arguments to pass to the onCreateRequest event. + */ + createRequest(args: RequestArgs): Promise { + return new Promise((resolve, reject) => { + const requestId = ++this._lastRequestId; + this._pendingRequests.set(requestId, resolve); + this._onCreateRequest.fire({ requestId, ...args }); + const tokenSource = new CancellationTokenSource(); + timeout(this._timeout, tokenSource.token).then(() => reject(`Request ${requestId} timed out (${this._timeout}ms)`)); + this._pendingRequestDisposables.set(requestId, [toDisposable(() => tokenSource.cancel())]); + }); + } + + /** + * Accept a reply to a request. + * @param requestId The request ID originating from the onCreateRequest event. + * @param data The reply data. + */ + acceptReply(requestId: number, data: T) { + const resolveRequest = this._pendingRequests.get(requestId); + if (resolveRequest) { + this._pendingRequests.delete(requestId); + dispose(this._pendingRequestDisposables.get(requestId) || []); + this._pendingRequestDisposables.delete(requestId); + resolveRequest(data); + } else { + this._logService.warn(`RequestStore#acceptReply was called without receiving a matching request ${requestId}`); + } + } +} diff --git a/src/vs/platform/terminal/common/terminal.ts b/src/vs/platform/terminal/common/terminal.ts index 6fea3dafc14..d3a7aeeed38 100644 --- a/src/vs/platform/terminal/common/terminal.ts +++ b/src/vs/platform/terminal/common/terminal.ts @@ -191,7 +191,7 @@ export interface IPtyService { restartPtyHost?(): Promise; shutdownAll?(): Promise; - acceptPtyHostResolvedVariables?(id: number, resolved: string[]): Promise; + acceptPtyHostResolvedVariables?(requestId: number, resolved: string[]): Promise; createProcess( shellLaunchConfig: IShellLaunchConfig, @@ -234,11 +234,11 @@ export interface IPtyService { getTerminalLayoutInfo(args: IGetTerminalLayoutInfoArgs): Promise; reduceConnectionGraceTime(): Promise; requestDetachInstance(workspaceId: string, instanceId: number): Promise; - acceptDetachedInstance(requestId: number, process: number): Promise; + acceptDetachedInstance(requestId: number, process: number): Promise; } export interface IRequestResolveVariablesEvent { - id: number; + requestId: number; workspaceId: string; originalText: string[]; } diff --git a/src/vs/platform/terminal/node/ptyHostService.ts b/src/vs/platform/terminal/node/ptyHostService.ts index 4bf9f0e2ad4..cf40e5d8738 100644 --- a/src/vs/platform/terminal/node/ptyHostService.ts +++ b/src/vs/platform/terminal/node/ptyHostService.ts @@ -17,6 +17,7 @@ import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry'; import { detectAvailableProfiles } from 'vs/platform/terminal/node/terminalProfiles'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; import { registerTerminalPlatformConfiguration } from 'vs/platform/terminal/common/terminalPlatformConfiguration'; +import { RequestStore } from 'vs/platform/terminal/common/requestStore'; enum Constants { MaxRestarts = 5 @@ -28,8 +29,6 @@ enum Constants { */ let lastPtyId = 0; -let lastResolveVariablesRequestId = 0; - /** * This service implements IPtyService by launching a pty host process, forwarding messages to and * from the pty host process and manages the connection. @@ -41,6 +40,7 @@ export class PtyHostService extends Disposable implements IPtyService { // ProxyChannel is not used here because events get lost when forwarding across multiple proxies private _proxy: IPtyService; + private readonly _resolveVariablesRequestStore: RequestStore; private _restartCount = 0; private _isResponsive = true; private _isDisposed = false; @@ -96,6 +96,9 @@ 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); + [this._client, this._proxy] = this._startPtyHost(); } @@ -249,7 +252,7 @@ export class PtyHostService extends Disposable implements IPtyService { return this._proxy.requestDetachInstance(workspaceId, instanceId); } - async acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { + async acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { return this._proxy.acceptDetachedInstance(requestId, persistentProcessId); } @@ -323,21 +326,10 @@ export class PtyHostService extends Disposable implements IPtyService { } } - private _pendingResolveVariablesRequests: Map void> = new Map(); private _resolveVariables(workspaceId: string, text: string[]): Promise { - return new Promise(resolve => { - const id = ++lastResolveVariablesRequestId; - this._pendingResolveVariablesRequests.set(id, resolve); - this._onPtyHostRequestResolveVariables.fire({ id, workspaceId, originalText: text }); - }); + return this._resolveVariablesRequestStore.createRequest({ workspaceId, originalText: text }); } - async acceptPtyHostResolvedVariables(id: number, resolved: string[]) { - const request = this._pendingResolveVariablesRequests.get(id); - if (request) { - request(resolved); - this._pendingResolveVariablesRequests.delete(id); - } else { - this._logService.warn(`Resolved variables received without matching request ${id}`); - } + async acceptPtyHostResolvedVariables(requestId: number, resolved: string[]) { + this._resolveVariablesRequestStore.acceptReply(requestId, resolved); } } diff --git a/src/vs/platform/terminal/node/ptyService.ts b/src/vs/platform/terminal/node/ptyService.ts index 4d088b43c21..3206624f62a 100644 --- a/src/vs/platform/terminal/node/ptyService.ts +++ b/src/vs/platform/terminal/node/ptyService.ts @@ -18,15 +18,16 @@ import { getWindowsBuildNumber } from 'vs/platform/terminal/node/terminalEnviron import { execFile } from 'child_process'; import { escapeNonWindowsPath } from 'vs/platform/terminal/common/terminalEnvironment'; import { URI } from 'vs/base/common/uri'; +import { RequestStore } from 'vs/platform/terminal/common/requestStore'; type WorkspaceId = string; -let lastResolvedInstanceRequestId = 0; export class PtyService extends Disposable implements IPtyService { declare readonly _serviceBrand: undefined; private readonly _ptys: Map = new Map(); private readonly _workspaceLayoutInfos = new Map(); + private readonly _detachInstanceRequestStore: RequestStore; private readonly _onHeartbeat = this._register(new Emitter()); readonly onHeartbeat = this._onHeartbeat.event; @@ -67,29 +68,22 @@ export class PtyService extends Disposable implements IPtyService { } this._ptys.clear(); })); + + this._detachInstanceRequestStore = this._register(new RequestStore(undefined, this._logService)); + this._detachInstanceRequestStore.onCreateRequest(this._onDidRequestDetach.fire, this._onDidRequestDetach); } - private _pendingDetachInstanceRequests: Map void> = new Map(); async requestDetachInstance(workspaceId: string, instanceId: number): Promise { - return new Promise(resolve => { - const requestId = ++lastResolvedInstanceRequestId; - this._pendingDetachInstanceRequests.set(requestId, resolve); - this._onDidRequestDetach.fire({ requestId, workspaceId, instanceId }); - }); + return this._detachInstanceRequestStore.createRequest({ workspaceId, instanceId }); } - async acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { - const request = this._pendingDetachInstanceRequests.get(requestId); - if (request) { - this._pendingDetachInstanceRequests.delete(requestId); - const pty = this._throwIfNoPty(persistentProcessId); - const process = await this._buildProcessDetails(persistentProcessId, pty); - request(process); - return process; - } else { - this._logService.warn(`Accept detached instance was called without receiving a matching request ${requestId} for process with ID: ${persistentProcessId}`); - return undefined; + async acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { + let processDetails: IProcessDetails | undefined = undefined; + const pty = this._ptys.get(persistentProcessId); + if (pty) { + processDetails = await this._buildProcessDetails(persistentProcessId, pty); } + this._detachInstanceRequestStore.acceptReply(requestId, processDetails); } async shutdownAll(): Promise { diff --git a/src/vs/platform/terminal/test/common/requestStore.test.ts b/src/vs/platform/terminal/test/common/requestStore.test.ts new file mode 100644 index 00000000000..369e32adeb5 --- /dev/null +++ b/src/vs/platform/terminal/test/common/requestStore.test.ts @@ -0,0 +1,44 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { fail, strictEqual } from 'assert'; +import { TestInstantiationService } from 'vs/platform/instantiation/test/common/instantiationServiceMock'; +import { ConsoleLogger, ILogService, LogService } from 'vs/platform/log/common/log'; +import { RequestStore } from 'vs/platform/terminal/common/requestStore'; + +suite('RequestStore', () => { + let instantiationService: TestInstantiationService; + + setup(() => { + instantiationService = new TestInstantiationService(); + instantiationService.stub(ILogService, new LogService(new ConsoleLogger())); + }); + + test('should resolve requests', async () => { + const store: RequestStore<{ data: string }, { arg: string }> = instantiationService.createInstance(RequestStore, undefined); + let eventArgs: { requestId: number, arg: string } | undefined; + store.onCreateRequest(e => eventArgs = e); + const request = store.createRequest({ arg: 'foo' }); + strictEqual(typeof eventArgs?.requestId, 'number'); + strictEqual(eventArgs?.arg, 'foo'); + store.acceptReply(eventArgs!.requestId, { data: 'bar' }); + const result = await request; + strictEqual(result.data, 'bar'); + }); + + test('should reject the promise when the request times out', async () => { + const store: RequestStore<{ data: string }, { arg: string }> = instantiationService.createInstance(RequestStore, 1); + const request = store.createRequest({ arg: 'foo' }); + let threw = false; + try { + await request; + } catch (e) { + threw = true; + } + if (!threw) { + fail(); + } + }); +}); diff --git a/src/vs/workbench/contrib/terminal/browser/remoteTerminalService.ts b/src/vs/workbench/contrib/terminal/browser/remoteTerminalService.ts index d0e4e8e05dc..48f4cd79c5a 100644 --- a/src/vs/workbench/contrib/terminal/browser/remoteTerminalService.ts +++ b/src/vs/workbench/contrib/terminal/browser/remoteTerminalService.ts @@ -148,7 +148,7 @@ export class RemoteTerminalService extends Disposable implements IRemoteTerminal return configurationResolverService.resolveAsync(lastActiveWorkspaceRoot, t); }); const result = await Promise.all(resolveCalls); - channel.acceptPtyHostResolvedVariables(e.id, result); + channel.acceptPtyHostResolvedVariables(e.requestId, result); })); } else { this._remoteTerminalChannel = null; @@ -162,7 +162,7 @@ export class RemoteTerminalService extends Disposable implements IRemoteTerminal return this._remoteTerminalChannel.requestDetachInstance(workspaceId, instanceId); } - async acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { + async acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { if (!this._remoteTerminalChannel) { throw new Error(`Cannot accept detached instance when there is no remote!`); } diff --git a/src/vs/workbench/contrib/terminal/common/remoteTerminalChannel.ts b/src/vs/workbench/contrib/terminal/common/remoteTerminalChannel.ts index 35373984be9..62e8fa6ac35 100644 --- a/src/vs/workbench/contrib/terminal/common/remoteTerminalChannel.ts +++ b/src/vs/workbench/contrib/terminal/common/remoteTerminalChannel.ts @@ -204,7 +204,7 @@ export class RemoteTerminalChannelClient { requestDetachInstance(workspaceId: string, instanceId: number): Promise { return this._channel.call('$requestDetachInstance', [workspaceId, instanceId]); } - acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { + acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { return this._channel.call('$acceptDetachedInstance', [requestId, persistentProcessId]); } attachToProcess(id: number): Promise { @@ -256,8 +256,8 @@ export class RemoteTerminalChannelClient { getProfiles(profiles: unknown, defaultProfile: unknown, includeDetectedProfiles?: boolean): Promise { return this._channel.call('$getProfiles', [this._workspaceContextService.getWorkspace().id, profiles, defaultProfile, includeDetectedProfiles]); } - acceptPtyHostResolvedVariables(id: number, resolved: string[]) { - return this._channel.call('$acceptPtyHostResolvedVariables', [id, resolved]); + acceptPtyHostResolvedVariables(requestId: number, resolved: string[]) { + return this._channel.call('$acceptPtyHostResolvedVariables', [requestId, resolved]); } getEnvironment(): Promise { diff --git a/src/vs/workbench/contrib/terminal/common/terminal.ts b/src/vs/workbench/contrib/terminal/common/terminal.ts index 280fc319f01..de15e73476c 100644 --- a/src/vs/workbench/contrib/terminal/common/terminal.ts +++ b/src/vs/workbench/contrib/terminal/common/terminal.ts @@ -96,7 +96,7 @@ export interface IOffProcessTerminalService { getTerminalLayoutInfo(): Promise; reduceConnectionGraceTime(): Promise; requestDetachInstance(workspaceId: string, instanceId: number): Promise; - acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise; + acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise; } export const ILocalTerminalService = createDecorator('localTerminalService'); diff --git a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalService.ts b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalService.ts index 96e7de8cc77..8b76e021826 100644 --- a/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalService.ts +++ b/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalService.ts @@ -121,7 +121,7 @@ export class LocalTerminalService extends Disposable implements ILocalTerminalSe return configurationResolverService.resolveAsync(lastActiveWorkspaceRoot, t); }); const result = await Promise.all(resolveCalls); - this._localPtyService.acceptPtyHostResolvedVariables?.(e.id, result); + this._localPtyService.acceptPtyHostResolvedVariables?.(e.requestId, result); })); } } @@ -130,8 +130,8 @@ export class LocalTerminalService extends Disposable implements ILocalTerminalSe return this._localPtyService.requestDetachInstance(workspaceId, instanceId); } - async acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { - return this._localPtyService.acceptDetachedInstance(requestId, persistentProcessId); + async acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { + await this._localPtyService.acceptDetachedInstance(requestId, persistentProcessId); } async updateTitle(id: number, title: string, titleSource: TitleEventSource): Promise { diff --git a/src/vs/workbench/test/browser/workbenchTestServices.ts b/src/vs/workbench/test/browser/workbenchTestServices.ts index 2c3e22f143f..f104fd83052 100644 --- a/src/vs/workbench/test/browser/workbenchTestServices.ts +++ b/src/vs/workbench/test/browser/workbenchTestServices.ts @@ -1684,7 +1684,7 @@ export class TestLocalTerminalService implements ILocalTerminalService { updateTitle(id: number, title: string): Promise { throw new Error('Method not implemented.'); } updateIcon(id: number, icon: URI | { light: URI; dark: URI } | { id: string, color?: { id: string } }, color?: string): Promise { throw new Error('Method not implemented.'); } requestDetachInstance(workspaceId: string, instanceId: number): Promise { throw new Error('Method not implemented.'); } - acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { throw new Error('Method not implemented.'); } + acceptDetachedInstance(requestId: number, persistentProcessId: number): Promise { throw new Error('Method not implemented.'); } } class TestTerminalChildProcess implements ITerminalChildProcess {