From 591039e34c44fd4bfd2807c589ea10933d2feb3e Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Thu, 12 Nov 2020 15:22:25 -0800 Subject: [PATCH] debug: fix compound integrated terminal launches breaking Fixes https://github.com/microsoft/vscode/issues/71850 Adds more resilient handling of multiple debug terminals, reusing terminals and adding a 1-second "lock out" before a terminal is candidate for reuse. --- src/vs/base/common/async.ts | 39 ++++++++++++ src/vs/base/test/common/async.test.ts | 61 +++++++++++++++++- .../workbench/api/node/extHostDebugService.ts | 62 ++++++++++++++----- 3 files changed, 145 insertions(+), 17 deletions(-) diff --git a/src/vs/base/common/async.ts b/src/vs/base/common/async.ts index 632c1989fca..167a47d151b 100644 --- a/src/vs/base/common/async.ts +++ b/src/vs/base/common/async.ts @@ -445,6 +445,45 @@ export function first(promiseFactories: ITask>[], shouldStop: (t: return loop(); } +/** + * Returns the result of the first promise that matches the "shouldStop", + * running all promises in parallel. Supports cancelable promises. + */ +export function firstParallel(promiseList: Promise[], shouldStop?: (t: T) => boolean, defaultValue?: T | null): Promise; +export function firstParallel(promiseList: Promise[], shouldStop: (t: T) => t is R, defaultValue?: R | null): Promise; +export function firstParallel(promiseList: Promise[], shouldStop: (t: T) => boolean = t => !!t, defaultValue: T | null = null) { + if (promiseList.length === 0) { + return Promise.resolve(defaultValue); + } + + let todo = promiseList.length; + const finish = () => { + todo = -1; + for (const promise of promiseList) { + (promise as Partial>).cancel?.(); + } + }; + + return new Promise((resolve, reject) => { + for (const promise of promiseList) { + promise.then(result => { + if (--todo >= 0 && shouldStop(result)) { + finish(); + resolve(result); + } else if (todo === 0) { + resolve(defaultValue); + } + }) + .catch(err => { + if (--todo >= 0) { + finish(); + reject(err); + } + }); + } + }); +} + interface ILimitedTaskFactory { factory: ITask>; c: (value: T | Promise) => void; diff --git a/src/vs/base/test/common/async.test.ts b/src/vs/base/test/common/async.test.ts index a7d505b2116..8ce05e26edd 100644 --- a/src/vs/base/test/common/async.test.ts +++ b/src/vs/base/test/common/async.test.ts @@ -7,7 +7,7 @@ import * as assert from 'assert'; import * as async from 'vs/base/common/async'; import { isPromiseCanceledError } from 'vs/base/common/errors'; import { URI } from 'vs/base/common/uri'; -import { CancellationTokenSource } from 'vs/base/common/cancellation'; +import { CancellationToken, CancellationTokenSource } from 'vs/base/common/cancellation'; suite('Async', () => { @@ -719,4 +719,63 @@ suite('Async', () => { assert.equal(counter.increment(), 2); assert.equal(counter.increment(), 3); }); + + test('firstParallel - simple', async () => { + const a = await async.firstParallel([ + Promise.resolve(1), + Promise.resolve(2), + Promise.resolve(3), + ], v => v === 2); + assert.equal(a, 2); + }); + + test('firstParallel - uses null default', async () => { + assert.equal(await async.firstParallel([Promise.resolve(1)], v => v === 2), null); + }); + + test('firstParallel - uses value default', async () => { + assert.equal(await async.firstParallel([Promise.resolve(1)], v => v === 2, 4), 4); + }); + + test('firstParallel - empty', async () => { + assert.equal(await async.firstParallel([], v => v === 2, 4), 4); + }); + + test('firstParallel - cancels', async () => { + let ct1: CancellationToken; + const p1 = async.createCancelablePromise(async (ct) => { + ct1 = ct; + await async.timeout(200, ct); + return 1; + }); + let ct2: CancellationToken; + const p2 = async.createCancelablePromise(async (ct) => { + ct2 = ct; + await async.timeout(2, ct); + return 2; + }); + + assert.equal(await async.firstParallel([p1, p2], v => v === 2, 4), 2); + assert.equal(ct1!.isCancellationRequested, true, 'should cancel a'); + assert.equal(ct2!.isCancellationRequested, true, 'should cancel b'); + }); + + test('firstParallel - rejection handling', async () => { + let ct1: CancellationToken; + const p1 = async.createCancelablePromise(async (ct) => { + ct1 = ct; + await async.timeout(200, ct); + return 1; + }); + let ct2: CancellationToken; + const p2 = async.createCancelablePromise(async (ct) => { + ct2 = ct; + await async.timeout(2, ct); + throw new Error('oh no'); + }); + + assert.equal(await async.firstParallel([p1, p2], v => v === 2, 4).catch(() => 'ok'), 'ok'); + assert.equal(ct1!.isCancellationRequested, true, 'should cancel a'); + assert.equal(ct2!.isCancellationRequested, true, 'should cancel b'); + }); }); diff --git a/src/vs/workbench/api/node/extHostDebugService.ts b/src/vs/workbench/api/node/extHostDebugService.ts index 9bb1c5f3653..af7f1aed30c 100644 --- a/src/vs/workbench/api/node/extHostDebugService.ts +++ b/src/vs/workbench/api/node/extHostDebugService.ts @@ -23,13 +23,14 @@ import { SignService } from 'vs/platform/sign/node/signService'; import { hasChildProcesses, prepareCommand, runInExternalTerminal } from 'vs/workbench/contrib/debug/node/terminals'; import { IDisposable } from 'vs/base/common/lifecycle'; import { AbstractVariableResolverService } from 'vs/workbench/services/configurationResolver/common/variableResolver'; +import { createCancelablePromise, firstParallel } from 'vs/base/common/async'; export class ExtHostDebugService extends ExtHostDebugServiceBase { readonly _serviceBrand: undefined; - private _integratedTerminalInstance?: vscode.Terminal; + private _integratedTerminalInstances = new DebugTerminalCollection(); private _terminalDisposedListener: IDisposable | undefined; constructor( @@ -74,25 +75,17 @@ export class ExtHostDebugService extends ExtHostDebugServiceBase { if (!this._terminalDisposedListener) { // React on terminal disposed and check if that is the debug terminal #12956 this._terminalDisposedListener = this._terminalService.onDidCloseTerminal(terminal => { - if (this._integratedTerminalInstance && this._integratedTerminalInstance === terminal) { - this._integratedTerminalInstance = undefined; - } + this._integratedTerminalInstances.onTerminalClosed(terminal); }); } - let needNewTerminal = true; // be pessimistic - if (this._integratedTerminalInstance) { - const pid = await this._integratedTerminalInstance.processId; - needNewTerminal = await hasChildProcesses(pid); // if no processes running in terminal reuse terminal - } - + let terminal = await this._integratedTerminalInstances.checkout(); const configProvider = await this._configurationService.getConfigProvider(); const shell = this._terminalService.getDefaultShell(true, configProvider); let cwdForPrepareCommand: string | undefined; let giveShellTimeToInitialize = false; - if (needNewTerminal || !this._integratedTerminalInstance) { - + if (!terminal) { const options: vscode.TerminalOptions = { shellPath: shell, // shellArgs: this._terminalService._getDefaultShellArgs(configProvider), @@ -100,17 +93,16 @@ export class ExtHostDebugService extends ExtHostDebugServiceBase { name: args.title || nls.localize('debug.terminal.title', "debuggee"), }; giveShellTimeToInitialize = true; - this._integratedTerminalInstance = this._terminalService.createTerminalFromOptions(options, true); + terminal = this._terminalService.createTerminalFromOptions(options, true); + this._integratedTerminalInstances.insert(terminal); } else { cwdForPrepareCommand = args.cwd; } - const terminal = this._integratedTerminalInstance; - terminal.show(); - const shellProcessId = await this._integratedTerminalInstance.processId; + const shellProcessId = await terminal.processId; if (giveShellTimeToInitialize) { // give a new terminal some time to initialize the shell @@ -134,3 +126,41 @@ export class ExtHostDebugService extends ExtHostDebugServiceBase { } } +class DebugTerminalCollection { + /** + * Delay before a new terminal is a candidate for reuse. See #71850 + */ + private static minUseDelay = 1000; + + private _terminalInstances = new Map(); + + public async checkout() { + const entries = [...this._terminalInstances.keys()]; + const promises = entries.map((terminal) => createCancelablePromise(async ct => { + const pid = await terminal.processId; + if (await hasChildProcesses(pid)) { + return null; + } + + // important: date check and map operations must be synchronous + const now = Date.now(); + const usedAt = this._terminalInstances.get(terminal); + if (!usedAt || usedAt + DebugTerminalCollection.minUseDelay > now || ct.isCancellationRequested) { + return null; + } + + this._terminalInstances.set(terminal, now); + return terminal; + })); + + return await firstParallel(promises, (t): t is vscode.Terminal => !!t); + } + + public insert(terminal: vscode.Terminal) { + this._terminalInstances.set(terminal, Date.now()); + } + + public onTerminalClosed(terminal: vscode.Terminal) { + this._terminalInstances.delete(terminal); + } +} -- GitLab