Skip to content

Commit

Permalink
debug: fix compound integrated terminal launches breaking
Browse files Browse the repository at this point in the history
Fixes #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.
  • Loading branch information
connor4312 authored and meganrogge committed Nov 18, 2020
1 parent 2beefaf commit 1e43f6d
Show file tree
Hide file tree
Showing 3 changed files with 145 additions and 17 deletions.
39 changes: 39 additions & 0 deletions src/vs/base/common/async.ts
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,45 @@ export function first<T>(promiseFactories: ITask<Promise<T>>[], 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<T>(promiseList: Promise<T>[], shouldStop?: (t: T) => boolean, defaultValue?: T | null): Promise<T | null>;
export function firstParallel<T, R extends T>(promiseList: Promise<T>[], shouldStop: (t: T) => t is R, defaultValue?: R | null): Promise<R | null>;
export function firstParallel<T>(promiseList: Promise<T>[], 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<CancelablePromise<T>>).cancel?.();
}
};

return new Promise<T | null>((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<T> {
factory: ITask<Promise<T>>;
c: (value: T | Promise<T>) => void;
Expand Down
61 changes: 60 additions & 1 deletion src/vs/base/test/common/async.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {

Expand Down Expand Up @@ -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');
});
});
62 changes: 46 additions & 16 deletions src/vs/workbench/api/node/extHostDebugService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -74,43 +75,34 @@ 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),
cwd: args.cwd,
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
Expand All @@ -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<vscode.Terminal, number /* last used at */>();

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);
}
}

0 comments on commit 1e43f6d

Please sign in to comment.