Skip to content

Commit

Permalink
Make _activeTasks synchronous wrt _executeTask
Browse files Browse the repository at this point in the history 
Fixes #180578

The problem was that after a call to _executeTask there is a delay (at least
until the next tick, but potentially considerably longer) before the task is
added to _activeTasks. During this period a second instance of the task could
be kicked off, causing problems.

This commit fixes the issue by adding an entry (without a terminal) to
_activeTasks before returning from _executeTask. For the most part though, it
preserves the existing behavior - so for example `getActiveTasks` only returns
the tasks that have terminals attached, just as it did before; and there is no
change to when `TaskEvent` events are fired for the task.
  • Loading branch information
markw65 committed Apr 22, 2023
1 parent ab7c32a commit 39a1a6c
Showing 1 changed file with 104 additions and 93 deletions.
197 changes: 104 additions & 93 deletions src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ interface ITerminalData {
}

interface IActiveTerminalData {
terminal: ITerminalInstance;
terminal?: ITerminalInstance;
task: Task;
promise: Promise<ITaskSummary>;
state?: TaskEventKind;
Expand Down Expand Up @@ -356,7 +356,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem {

public isTaskVisible(task: Task): boolean {
const terminalData = this._activeTasks[task.getMapKey()];
if (!terminalData) {
if (!terminalData || !terminalData.terminal) {
return false;
}
const activeTerminalInstance = this._terminalService.activeInstance;
Expand All @@ -367,7 +367,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem {

public revealTask(task: Task): boolean {
const terminalData = this._activeTasks[task.getMapKey()];
if (!terminalData) {
if (!terminalData || !terminalData.terminal) {
return false;
}
const isTerminalInPanel: boolean = this._viewDescriptorService.getViewLocationById(TERMINAL_VIEW_ID) === ViewContainerLocation.Panel;
Expand Down Expand Up @@ -402,26 +402,21 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem {
}

public isActiveSync(): boolean {
return Object.keys(this._activeTasks).length > 0;
return Object.values(this._activeTasks).some(value => !!value.terminal);
}

public canAutoTerminate(): boolean {
return Object.keys(this._activeTasks).every(key => !this._activeTasks[key].task.configurationProperties.promptOnClose);
return Object.values(this._activeTasks).every(value => !value.task.configurationProperties.promptOnClose);
}

public getActiveTasks(): Task[] {
return Object.keys(this._activeTasks).map(key => this._activeTasks[key].task);
return Object.values(this._activeTasks).flatMap(value => value.terminal ? value.task : []);
}

public getLastInstance(task: Task): Task | undefined {
let lastInstance = undefined;
const recentKey = task.getRecentlyUsedKey();
Object.keys(this._activeTasks).forEach((key) => {
if (recentKey && recentKey === this._activeTasks[key].task.getRecentlyUsedKey()) {
lastInstance = this._activeTasks[key].task;
}
});
return lastInstance;
return Object.values(this._activeTasks).reverse().find(
(value) => recentKey && recentKey === value.task.getRecentlyUsedKey())?.task;
}

public getBusyTasks(): Task[] {
Expand All @@ -430,7 +425,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem {

public customExecutionComplete(task: Task, result: number): Promise<void> {
const activeTerminal = this._activeTasks[task.getMapKey()];
if (!activeTerminal) {
if (!activeTerminal || !activeTerminal.terminal) {
return Promise.reject(new Error('Expected to have a terminal for an custom execution task'));
}

Expand All @@ -452,10 +447,10 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem {

private _removeFromActiveTasks(task: Task | string): void {
const key = typeof task === 'string' ? task : task.getMapKey();
if (!this._activeTasks[key]) {
const taskToRemove = this._activeTasks[key];
if (!taskToRemove) {
return;
}
const taskToRemove = this._activeTasks[key];
delete this._activeTasks[key];
this._removeInstances(taskToRemove.task);
}
Expand All @@ -472,11 +467,11 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem {

public terminate(task: Task): Promise<ITaskTerminateResponse> {
const activeTerminal = this._activeTasks[task.getMapKey()];
if (!activeTerminal) {
if (!activeTerminal || !activeTerminal.terminal) {
return Promise.resolve<ITaskTerminateResponse>({ success: false, task: undefined });
}
const terminal = activeTerminal.terminal;
return new Promise<ITaskTerminateResponse>((resolve, reject) => {
const terminal = activeTerminal.terminal;
terminal.onDisposed(terminal => {
this._fireTaskEvent(TaskEvent.create(TaskEventKind.Terminated, task, terminal.instanceId, terminal.exitReason));
});
Expand All @@ -496,28 +491,30 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem {

public terminateAll(): Promise<ITaskTerminateResponse[]> {
const promises: Promise<ITaskTerminateResponse>[] = [];
Object.keys(this._activeTasks).forEach((key) => {
const terminalData = this._activeTasks[key];
Object.entries(this._activeTasks).forEach(([key, terminalData]) => {
const terminal = terminalData.terminal;
promises.push(new Promise<ITaskTerminateResponse>((resolve, reject) => {
const onExit = terminal.onExit(() => {
const task = terminalData.task;
try {
onExit.dispose();
this._fireTaskEvent(TaskEvent.create(TaskEventKind.Terminated, task, terminal.instanceId, terminal.exitReason));
} catch (error) {
// Do nothing.
}
resolve({ success: true, task: terminalData.task });
});
}));
terminal.dispose();
if (terminal) {
promises.push(new Promise<ITaskTerminateResponse>((resolve, reject) => {
const onExit = terminal.onExit(() => {
const task = terminalData.task;
try {
onExit.dispose();
this._fireTaskEvent(TaskEvent.create(TaskEventKind.Terminated, task, terminal.instanceId, terminal.exitReason));
} catch (error) {
// Do nothing.
}
if (this._activeTasks[key] === terminalData) {
delete this._activeTasks[key];
}
resolve({ success: true, task: terminalData.task });
});
}));
terminal.dispose();
}
});
this._activeTasks = Object.create(null);
return Promise.all<ITaskTerminateResponse>(promises);
}


private _showDependencyCycleMessage(task: Task) {
this._log(nls.localize('dependencyCycle',
'There is a dependency cycle. See task "{0}".',
Expand All @@ -526,76 +523,90 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem {
this._showOutput();
}

private async _executeTask(task: Task, resolver: ITaskResolver, trigger: string, encounteredDependencies: Set<string>, alreadyResolved?: Map<string, string>): Promise<ITaskSummary> {
private _executeTask(task: Task, resolver: ITaskResolver, trigger: string, encounteredDependencies: Set<string>, alreadyResolved?: Map<string, string>): Promise<ITaskSummary> {
if (encounteredDependencies.has(task.getCommonTaskId())) {
this._showDependencyCycleMessage(task);
return {};
return Promise.resolve({});
}

this._showTaskLoadErrors(task);

alreadyResolved = alreadyResolved ?? new Map<string, string>();
const promises: Promise<ITaskSummary>[] = [];
if (task.configurationProperties.dependsOn) {
for (const dependency of task.configurationProperties.dependsOn) {
const dependencyTask = await resolver.resolve(dependency.uri, dependency.task!);
if (dependencyTask) {
this._adoptConfigurationForDependencyTask(dependencyTask, task);
const key = dependencyTask.getMapKey();
let promise = this._activeTasks[key] ? this._getDependencyPromise(this._activeTasks[key]) : undefined;
if (!promise) {
this._fireTaskEvent(TaskEvent.create(TaskEventKind.DependsOnStarted, task));
encounteredDependencies.add(task.getCommonTaskId());
promise = this._executeDependencyTask(dependencyTask, resolver, trigger, encounteredDependencies, alreadyResolved);
}
promises.push(promise);
if (task.configurationProperties.dependsOrder === DependsOrder.sequence) {
const promiseResult = await promise;
if (promiseResult.exitCode === 0) {
promise = Promise.resolve(promiseResult);
} else {
promise = Promise.reject(promiseResult);
break;
const mapKey = task.getMapKey();

// It's important that we add this task's entry to _activeTasks before
// any of the code in the then runs (see #180541 and #180578). Wrapping
// it in Promise.resolve().then() ensures that.
const promise = Promise.resolve().then(async () => {
alreadyResolved = alreadyResolved ?? new Map<string, string>();
const promises: Promise<ITaskSummary>[] = [];
if (task.configurationProperties.dependsOn) {
for (const dependency of task.configurationProperties.dependsOn) {
const dependencyTask = await resolver.resolve(dependency.uri, dependency.task!);
if (dependencyTask) {
this._adoptConfigurationForDependencyTask(dependencyTask, task);
const key = dependencyTask.getMapKey();
let promise = this._activeTasks[key] ? this._getDependencyPromise(this._activeTasks[key]) : undefined;
if (!promise) {
this._fireTaskEvent(TaskEvent.create(TaskEventKind.DependsOnStarted, task));
encounteredDependencies.add(task.getCommonTaskId());
promise = this._executeDependencyTask(dependencyTask, resolver, trigger, encounteredDependencies, alreadyResolved);
}
promises.push(promise);
if (task.configurationProperties.dependsOrder === DependsOrder.sequence) {
const promiseResult = await promise;
if (promiseResult.exitCode === 0) {
promise = Promise.resolve(promiseResult);
} else {
promise = Promise.reject(promiseResult);
break;
}
}
promises.push(promise);
} else {
this._log(nls.localize('dependencyFailed',
'Couldn\'t resolve dependent task \'{0}\' in workspace folder \'{1}\'',
Types.isString(dependency.task) ? dependency.task : JSON.stringify(dependency.task, undefined, 0),
dependency.uri.toString()
));
this._showOutput();
}
promises.push(promise);
} else {
this._log(nls.localize('dependencyFailed',
'Couldn\'t resolve dependent task \'{0}\' in workspace folder \'{1}\'',
Types.isString(dependency.task) ? dependency.task : JSON.stringify(dependency.task, undefined, 0),
dependency.uri.toString()
));
this._showOutput();
}
}
}

if ((ContributedTask.is(task) || CustomTask.is(task)) && (task.command)) {
return Promise.all(promises).then((summaries): Promise<ITaskSummary> | ITaskSummary => {
encounteredDependencies.delete(task.getCommonTaskId());
for (const summary of summaries) {
if (summary.exitCode !== 0) {
this._removeInstances(task);
return { exitCode: summary.exitCode };
if ((ContributedTask.is(task) || CustomTask.is(task)) && (task.command)) {
return Promise.all(promises).then((summaries): Promise<ITaskSummary> | ITaskSummary => {
encounteredDependencies.delete(task.getCommonTaskId());
for (const summary of summaries) {
if (summary.exitCode !== 0) {
this._removeInstances(task);
return { exitCode: summary.exitCode };
}
}
}
if (this._isRerun) {
return this._reexecuteCommand(task, trigger, alreadyResolved!);
} else {
return this._executeCommand(task, trigger, alreadyResolved!);
}
});
} else {
return Promise.all(promises).then((summaries): ITaskSummary => {
encounteredDependencies.delete(task.getCommonTaskId());
for (const summary of summaries) {
if (summary.exitCode !== 0) {
return { exitCode: summary.exitCode };
if (this._isRerun) {
return this._reexecuteCommand(task, trigger, alreadyResolved!);
} else {
return this._executeCommand(task, trigger, alreadyResolved!);
}
}
return { exitCode: 0 };
});
}
});
} else {
return Promise.all(promises).then((summaries): ITaskSummary => {
encounteredDependencies.delete(task.getCommonTaskId());
for (const summary of summaries) {
if (summary.exitCode !== 0) {
return { exitCode: summary.exitCode };
}
}
return { exitCode: 0 };
});
}
}).finally(() => {
if (this._activeTasks[mapKey] === activeTask) {
delete this._activeTasks[mapKey];
}
});
const activeTask = { task, promise };
this._activeTasks[mapKey] = activeTask;
return promise;
}

private _createInactiveDependencyPromise(task: Task): Promise<ITaskSummary> {
Expand Down Expand Up @@ -1068,7 +1079,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem {
this._terminalService.setActiveInstance(terminal);
this._terminalGroupService.showPanel(task.command.presentation.focus);
}
this._activeTasks[task.getMapKey()] = { terminal, task, promise };
this._activeTasks[task.getMapKey()].terminal = terminal;
this._fireTaskEvent(TaskEvent.create(TaskEventKind.Changed));
return promise;
}
Expand Down

0 comments on commit 39a1a6c

Please sign in to comment.