From bcf8379f7adb6e1ec158e20ade63249a8c9dc9c0 Mon Sep 17 00:00:00 2001 From: markw65 Date: Sat, 22 Apr 2023 09:17:46 -0700 Subject: [PATCH] Make _activeTasks synchronous wrt _executeTask 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. --- .../tasks/browser/terminalTaskSystem.ts | 197 +++++++++--------- 1 file changed, 104 insertions(+), 93 deletions(-) diff --git a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts index b0be6807914b7a..0ea4dd2faec3ce 100644 --- a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts +++ b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts @@ -60,7 +60,7 @@ interface ITerminalData { } interface IActiveTerminalData { - terminal: ITerminalInstance; + terminal?: ITerminalInstance; task: Task; promise: Promise; state?: TaskEventKind; @@ -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; @@ -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; @@ -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[] { @@ -430,7 +425,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { public customExecutionComplete(task: Task, result: number): Promise { 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')); } @@ -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); } @@ -472,11 +467,11 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { public terminate(task: Task): Promise { const activeTerminal = this._activeTasks[task.getMapKey()]; - if (!activeTerminal) { + if (!activeTerminal || !activeTerminal.terminal) { return Promise.resolve({ success: false, task: undefined }); } + const terminal = activeTerminal.terminal; return new Promise((resolve, reject) => { - const terminal = activeTerminal.terminal; terminal.onDisposed(terminal => { this._fireTaskEvent(TaskEvent.create(TaskEventKind.Terminated, task, terminal.instanceId, terminal.exitReason)); }); @@ -496,28 +491,30 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { public terminateAll(): Promise { const promises: Promise[] = []; - 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((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((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(promises); } - private _showDependencyCycleMessage(task: Task) { this._log(nls.localize('dependencyCycle', 'There is a dependency cycle. See task "{0}".', @@ -526,76 +523,90 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { this._showOutput(); } - private async _executeTask(task: Task, resolver: ITaskResolver, trigger: string, encounteredDependencies: Set, alreadyResolved?: Map): Promise { + private _executeTask(task: Task, resolver: ITaskResolver, trigger: string, encounteredDependencies: Set, alreadyResolved?: Map): Promise { if (encounteredDependencies.has(task.getCommonTaskId())) { this._showDependencyCycleMessage(task); - return {}; + return Promise.resolve({}); } this._showTaskLoadErrors(task); - alreadyResolved = alreadyResolved ?? new Map(); - const promises: Promise[] = []; - 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(); + const promises: Promise[] = []; + 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 => { - 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 => { + 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 { @@ -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; }