From c9fb898dd7fe319d50dddd7b25a3e6ad06527262 Mon Sep 17 00:00:00 2001 From: markw65 Date: Sat, 22 Apr 2023 09:17:46 -0700 Subject: [PATCH 01/12] 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 b0be6807914b7..0ea4dd2faec3c 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; } From fe67dd6e1e21f58c2788c7acdf791d45644b664c Mon Sep 17 00:00:00 2001 From: markw65 Date: Mon, 24 Apr 2023 15:05:26 -0700 Subject: [PATCH 02/12] Fix dependency cycle detection The way dependency cycles were detected no longer works now that activeTasks is populated early. Fix it. --- .../contrib/tasks/browser/terminalTaskSystem.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts index 0ea4dd2faec3c..f460b6455efcc 100644 --- a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts +++ b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts @@ -524,11 +524,6 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { } private _executeTask(task: Task, resolver: ITaskResolver, trigger: string, encounteredDependencies: Set, alreadyResolved?: Map): Promise { - if (encounteredDependencies.has(task.getCommonTaskId())) { - this._showDependencyCycleMessage(task); - return Promise.resolve({}); - } - this._showTaskLoadErrors(task); const mapKey = task.getMapKey(); @@ -540,15 +535,21 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { alreadyResolved = alreadyResolved ?? new Map(); const promises: Promise[] = []; if (task.configurationProperties.dependsOn) { + encounteredDependencies = new Set(encounteredDependencies).add(task.getCommonTaskId()); 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; + let promise; + if (encounteredDependencies.has(dependencyTask.getCommonTaskId())) { + this._showDependencyCycleMessage(dependencyTask); + promise = Promise.resolve({}); + } else { + 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); @@ -575,7 +576,6 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { 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); @@ -590,7 +590,6 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { }); } 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 }; From fda6f9c79c41de4ddd58c10a51f8176f4d92c094 Mon Sep 17 00:00:00 2001 From: markw65 Date: Mon, 24 Apr 2023 15:55:01 -0700 Subject: [PATCH 03/12] Remove some redundant code handling sequential dependencies --- .../workbench/contrib/tasks/browser/terminalTaskSystem.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts index f460b6455efcc..008653458fb25 100644 --- a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts +++ b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts @@ -555,14 +555,10 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { 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); + if (promiseResult.exitCode !== 0) { break; } } - promises.push(promise); } else { this._log(nls.localize('dependencyFailed', 'Couldn\'t resolve dependent task \'{0}\' in workspace folder \'{1}\'', From 38ac734058645c2be099760045673fd4b8565894 Mon Sep 17 00:00:00 2001 From: markw65 Date: Tue, 25 Apr 2023 14:21:58 -0700 Subject: [PATCH 04/12] Never launch more than one copy of a dependent task Fixes #180850 If a task appears multiple times within a dependency hierarchy, it will usually still be running for each subsequent visit, and won't be launched again. But if it finishes before the next instance is visited, it will be run a second time. This can easily happen if a parent task's `dependsOrder` is set to `"sequence"`. This fixes the problem by keeping track of the promises associated with all the tasks that have been started so far. --- .../tasks/browser/terminalTaskSystem.ts | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts index 008653458fb25..71a0be45bfb63 100644 --- a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts +++ b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts @@ -300,7 +300,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { } try { - const executeResult = { kind: TaskExecuteKind.Started, task, started: {}, promise: this._executeTask(task, resolver, trigger, new Set(), undefined) }; + const executeResult = { kind: TaskExecuteKind.Started, task, started: {}, promise: this._executeTask(task, resolver, trigger, new Set(), new Map(), undefined) }; executeResult.promise.then(summary => { this._lastTask = this._currentTask; }); @@ -523,7 +523,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { this._showOutput(); } - private _executeTask(task: Task, resolver: ITaskResolver, trigger: string, encounteredDependencies: Set, alreadyResolved?: Map): Promise { + private _executeTask(task: Task, resolver: ITaskResolver, trigger: string, liveDependencies: Set, encounteredTasks: Map>, alreadyResolved?: Map): Promise { this._showTaskLoadErrors(task); const mapKey = task.getMapKey(); @@ -535,23 +535,28 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { alreadyResolved = alreadyResolved ?? new Map(); const promises: Promise[] = []; if (task.configurationProperties.dependsOn) { - encounteredDependencies = new Set(encounteredDependencies).add(task.getCommonTaskId()); + liveDependencies = new Set(liveDependencies).add(task.getCommonTaskId()); 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; - if (encounteredDependencies.has(dependencyTask.getCommonTaskId())) { + const commonKey = dependencyTask.getCommonTaskId(); + if (liveDependencies.has(commonKey)) { this._showDependencyCycleMessage(dependencyTask); promise = Promise.resolve({}); } else { - promise = this._activeTasks[key] ? this._getDependencyPromise(this._activeTasks[key]) : undefined; + promise = encounteredTasks.get(commonKey); + if (!promise) { + const key = dependencyTask.getMapKey(); + promise = this._activeTasks[key] ? this._getDependencyPromise(this._activeTasks[key]) : undefined; + } } if (!promise) { this._fireTaskEvent(TaskEvent.create(TaskEventKind.DependsOnStarted, task)); - promise = this._executeDependencyTask(dependencyTask, resolver, trigger, encounteredDependencies, alreadyResolved); + promise = this._executeDependencyTask(dependencyTask, resolver, trigger, liveDependencies, encounteredTasks, alreadyResolved); } + encounteredTasks.set(commonKey, promise); promises.push(promise); if (task.configurationProperties.dependsOrder === DependsOrder.sequence) { const promiseResult = await promise; @@ -643,15 +648,15 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { return this._createInactiveDependencyPromise(task.task); } - private async _executeDependencyTask(task: Task, resolver: ITaskResolver, trigger: string, encounteredDependencies: Set, alreadyResolved?: Map): Promise { + private async _executeDependencyTask(task: Task, resolver: ITaskResolver, trigger: string, liveDependencies: Set, encounteredTasks: Map>, alreadyResolved?: Map): Promise { // If the task is a background task with a watching problem matcher, we don't wait for the whole task to finish, // just for the problem matcher to go inactive. if (!task.configurationProperties.isBackground) { - return this._executeTask(task, resolver, trigger, encounteredDependencies, alreadyResolved); + return this._executeTask(task, resolver, trigger, liveDependencies, encounteredTasks, alreadyResolved); } const inactivePromise = this._createInactiveDependencyPromise(task); - return Promise.race([inactivePromise, this._executeTask(task, resolver, trigger, encounteredDependencies, alreadyResolved)]); + return Promise.race([inactivePromise, this._executeTask(task, resolver, trigger, liveDependencies, encounteredTasks, alreadyResolved)]); } private async _resolveAndFindExecutable(systemInfo: ITaskSystemInfo | undefined, workspaceFolder: IWorkspaceFolder | undefined, task: CustomTask | ContributedTask, cwd: string | undefined, envPath: string | undefined): Promise { From 73ea8510efbdc22d2cc18fefe96994ca30bc0f88 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Mon, 1 May 2023 13:20:50 -0700 Subject: [PATCH 05/12] polish --- .../contrib/tasks/browser/terminalTaskSystem.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts index 71a0be45bfb63..5eac75c7c95e9 100644 --- a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts +++ b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts @@ -356,7 +356,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { public isTaskVisible(task: Task): boolean { const terminalData = this._activeTasks[task.getMapKey()]; - if (!terminalData || !terminalData.terminal) { + if (!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 || !terminalData.terminal) { + if (!terminalData?.terminal) { return false; } const isTerminalInPanel: boolean = this._viewDescriptorService.getViewLocationById(TERMINAL_VIEW_ID) === ViewContainerLocation.Panel; @@ -425,8 +425,8 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { public customExecutionComplete(task: Task, result: number): Promise { const activeTerminal = this._activeTasks[task.getMapKey()]; - if (!activeTerminal || !activeTerminal.terminal) { - return Promise.reject(new Error('Expected to have a terminal for an custom execution task')); + if (!activeTerminal?.terminal) { + return Promise.reject(new Error('Expected to have a terminal for a custom execution task')); } return new Promise((resolve) => { @@ -467,10 +467,10 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { public terminate(task: Task): Promise { const activeTerminal = this._activeTasks[task.getMapKey()]; - if (!activeTerminal || !activeTerminal.terminal) { + const terminal = activeTerminal.terminal; + if (!terminal) { return Promise.resolve({ success: false, task: undefined }); } - const terminal = activeTerminal.terminal; return new Promise((resolve, reject) => { terminal.onDisposed(terminal => { this._fireTaskEvent(TaskEvent.create(TaskEventKind.Terminated, task, terminal.instanceId, terminal.exitReason)); From 08929a298f39fee7eb936bfef30b57621f16d256 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Tue, 2 May 2023 05:38:12 -0700 Subject: [PATCH 06/12] rm from liveDependencies/encounteredTasks --- .../tasks/browser/abstractTaskService.ts | 7 ++ .../tasks/browser/terminalTaskSystem.ts | 25 ++++--- test/.vscode/tasks.json | 68 +++++++++++++++++++ test/test/.vscode/tasks.json | 68 +++++++++++++++++++ test/test/test/.vscode/tasks.json | 68 +++++++++++++++++++ 5 files changed, 225 insertions(+), 11 deletions(-) create mode 100644 test/.vscode/tasks.json create mode 100644 test/test/.vscode/tasks.json create mode 100644 test/test/test/.vscode/tasks.json diff --git a/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts b/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts index b0e9ee2cbc069..e670705d52479 100644 --- a/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts +++ b/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts @@ -326,6 +326,13 @@ export abstract class AbstractTaskService extends Disposable implements ITaskSer }); this._lifecycleService.onBeforeShutdown(e => { this._willRestart = e.reason !== ShutdownReason.RELOAD; + this.getActiveTasks().then(async (tasks) => { + for (const task of tasks) { + if (!task.configurationProperties.isBackground) { + await this.terminate(task); + } + } + }); }); this._register(this.onDidStateChange(e => { if ((this._willRestart || e.exitReason === TerminalExitReason.User) && e.taskId) { diff --git a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts index 5eac75c7c95e9..a8ca158717a69 100644 --- a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts +++ b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts @@ -294,7 +294,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { } const lastTaskInstance = this.getLastInstance(task); const terminalData = lastTaskInstance ? this._activeTasks[lastTaskInstance.getMapKey()] : undefined; - if (terminalData && terminalData.promise && !validInstance) { + if (terminalData && terminalData.promise && !validInstance && !task.configurationProperties.dependsOn?.length) { this._lastTask = this._currentTask; return { kind: TaskExecuteKind.Active, task: terminalData.task, active: { same: true, background: task.configurationProperties.isBackground! }, promise: terminalData.promise }; } @@ -540,26 +540,26 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { const dependencyTask = await resolver.resolve(dependency.uri, dependency.task!); if (dependencyTask) { this._adoptConfigurationForDependencyTask(dependencyTask, task); - let promise; + let taskResult; const commonKey = dependencyTask.getCommonTaskId(); if (liveDependencies.has(commonKey)) { this._showDependencyCycleMessage(dependencyTask); - promise = Promise.resolve({}); + taskResult = Promise.resolve({}); } else { - promise = encounteredTasks.get(commonKey); - if (!promise) { + taskResult = encounteredTasks.get(commonKey); + if (!taskResult) { const key = dependencyTask.getMapKey(); - promise = this._activeTasks[key] ? this._getDependencyPromise(this._activeTasks[key]) : undefined; + taskResult = this._activeTasks[key] ? this._getDependencyPromise(this._activeTasks[key]) : undefined; } } - if (!promise) { + if (!taskResult) { this._fireTaskEvent(TaskEvent.create(TaskEventKind.DependsOnStarted, task)); - promise = this._executeDependencyTask(dependencyTask, resolver, trigger, liveDependencies, encounteredTasks, alreadyResolved); + taskResult = this._executeDependencyTask(dependencyTask, resolver, trigger, liveDependencies, encounteredTasks, alreadyResolved); } - encounteredTasks.set(commonKey, promise); - promises.push(promise); + encounteredTasks.set(commonKey, taskResult); + promises.push(taskResult); if (task.configurationProperties.dependsOrder === DependsOrder.sequence) { - const promiseResult = await promise; + const promiseResult = await taskResult; if (promiseResult.exitCode !== 0) { break; } @@ -600,6 +600,9 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { }); } }).finally(() => { + const commonKey = task.getCommonTaskId(); + liveDependencies.delete(commonKey); + encounteredTasks.delete(commonKey); if (this._activeTasks[mapKey] === activeTask) { delete this._activeTasks[mapKey]; } diff --git a/test/.vscode/tasks.json b/test/.vscode/tasks.json new file mode 100644 index 0000000000000..0f58a805dc719 --- /dev/null +++ b/test/.vscode/tasks.json @@ -0,0 +1,68 @@ +{ + "version": "2.0.0", + "tasks": [ + { + "type": "shell", + "command": "sleep 2", + "label": "sleeper", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task1", + "dependsOn": ["sleeper"], + "label": "task1", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task2", + "dependsOn": ["task1"], + "label": "task2", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task3", + "dependsOn": ["task1"], + "label": "task3", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task4... && sleep 3 && echo ...done", + "dependsOn": ["task2", "task3"], + "label": "task4", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + }, + "group": { + "kind": "build", + "isDefault": true + } + } + ] +} diff --git a/test/test/.vscode/tasks.json b/test/test/.vscode/tasks.json new file mode 100644 index 0000000000000..0f58a805dc719 --- /dev/null +++ b/test/test/.vscode/tasks.json @@ -0,0 +1,68 @@ +{ + "version": "2.0.0", + "tasks": [ + { + "type": "shell", + "command": "sleep 2", + "label": "sleeper", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task1", + "dependsOn": ["sleeper"], + "label": "task1", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task2", + "dependsOn": ["task1"], + "label": "task2", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task3", + "dependsOn": ["task1"], + "label": "task3", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task4... && sleep 3 && echo ...done", + "dependsOn": ["task2", "task3"], + "label": "task4", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + }, + "group": { + "kind": "build", + "isDefault": true + } + } + ] +} diff --git a/test/test/test/.vscode/tasks.json b/test/test/test/.vscode/tasks.json new file mode 100644 index 0000000000000..0f58a805dc719 --- /dev/null +++ b/test/test/test/.vscode/tasks.json @@ -0,0 +1,68 @@ +{ + "version": "2.0.0", + "tasks": [ + { + "type": "shell", + "command": "sleep 2", + "label": "sleeper", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task1", + "dependsOn": ["sleeper"], + "label": "task1", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task2", + "dependsOn": ["task1"], + "label": "task2", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task3", + "dependsOn": ["task1"], + "label": "task3", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + } + }, + { + "type": "shell", + "command": "echo task4... && sleep 3 && echo ...done", + "dependsOn": ["task2", "task3"], + "label": "task4", + "problemMatcher": [], + "presentation": { + "reveal": "always", + "group": "sleepers", + "close": false + }, + "group": { + "kind": "build", + "isDefault": true + } + } + ] +} From ff7d86a41c80395c417b6a627d639658879d1b2f Mon Sep 17 00:00:00 2001 From: meganrogge Date: Tue, 2 May 2023 05:49:56 -0700 Subject: [PATCH 07/12] rm accidental additions --- test/.vscode/tasks.json | 68 ------------------------------- test/test/.vscode/tasks.json | 68 ------------------------------- test/test/test/.vscode/tasks.json | 68 ------------------------------- 3 files changed, 204 deletions(-) delete mode 100644 test/.vscode/tasks.json delete mode 100644 test/test/.vscode/tasks.json delete mode 100644 test/test/test/.vscode/tasks.json diff --git a/test/.vscode/tasks.json b/test/.vscode/tasks.json deleted file mode 100644 index 0f58a805dc719..0000000000000 --- a/test/.vscode/tasks.json +++ /dev/null @@ -1,68 +0,0 @@ -{ - "version": "2.0.0", - "tasks": [ - { - "type": "shell", - "command": "sleep 2", - "label": "sleeper", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task1", - "dependsOn": ["sleeper"], - "label": "task1", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task2", - "dependsOn": ["task1"], - "label": "task2", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task3", - "dependsOn": ["task1"], - "label": "task3", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task4... && sleep 3 && echo ...done", - "dependsOn": ["task2", "task3"], - "label": "task4", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - }, - "group": { - "kind": "build", - "isDefault": true - } - } - ] -} diff --git a/test/test/.vscode/tasks.json b/test/test/.vscode/tasks.json deleted file mode 100644 index 0f58a805dc719..0000000000000 --- a/test/test/.vscode/tasks.json +++ /dev/null @@ -1,68 +0,0 @@ -{ - "version": "2.0.0", - "tasks": [ - { - "type": "shell", - "command": "sleep 2", - "label": "sleeper", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task1", - "dependsOn": ["sleeper"], - "label": "task1", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task2", - "dependsOn": ["task1"], - "label": "task2", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task3", - "dependsOn": ["task1"], - "label": "task3", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task4... && sleep 3 && echo ...done", - "dependsOn": ["task2", "task3"], - "label": "task4", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - }, - "group": { - "kind": "build", - "isDefault": true - } - } - ] -} diff --git a/test/test/test/.vscode/tasks.json b/test/test/test/.vscode/tasks.json deleted file mode 100644 index 0f58a805dc719..0000000000000 --- a/test/test/test/.vscode/tasks.json +++ /dev/null @@ -1,68 +0,0 @@ -{ - "version": "2.0.0", - "tasks": [ - { - "type": "shell", - "command": "sleep 2", - "label": "sleeper", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task1", - "dependsOn": ["sleeper"], - "label": "task1", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task2", - "dependsOn": ["task1"], - "label": "task2", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task3", - "dependsOn": ["task1"], - "label": "task3", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - } - }, - { - "type": "shell", - "command": "echo task4... && sleep 3 && echo ...done", - "dependsOn": ["task2", "task3"], - "label": "task4", - "problemMatcher": [], - "presentation": { - "reveal": "always", - "group": "sleepers", - "close": false - }, - "group": { - "kind": "build", - "isDefault": true - } - } - ] -} From 0476301a02badbe7d3a4ba2aa96599c66960e4d6 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Tue, 2 May 2023 07:51:02 -0700 Subject: [PATCH 08/12] don't update encounteredTasks --- src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts index a8ca158717a69..663c7fee1bdb3 100644 --- a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts +++ b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts @@ -600,9 +600,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { }); } }).finally(() => { - const commonKey = task.getCommonTaskId(); - liveDependencies.delete(commonKey); - encounteredTasks.delete(commonKey); + liveDependencies.delete(task.getCommonTaskId()); if (this._activeTasks[mapKey] === activeTask) { delete this._activeTasks[mapKey]; } From 3b80fb233d6d92c20528a543526eceaaf99a6d2d Mon Sep 17 00:00:00 2001 From: markw65 Date: Tue, 2 May 2023 08:26:58 -0700 Subject: [PATCH 09/12] Make it clear that the modification to liveDependencies is local --- .../workbench/contrib/tasks/browser/terminalTaskSystem.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts index 663c7fee1bdb3..6bc6781d66018 100644 --- a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts +++ b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts @@ -535,14 +535,14 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { alreadyResolved = alreadyResolved ?? new Map(); const promises: Promise[] = []; if (task.configurationProperties.dependsOn) { - liveDependencies = new Set(liveDependencies).add(task.getCommonTaskId()); + const nextLiveDependencies = new Set(liveDependencies).add(task.getCommonTaskId()); for (const dependency of task.configurationProperties.dependsOn) { const dependencyTask = await resolver.resolve(dependency.uri, dependency.task!); if (dependencyTask) { this._adoptConfigurationForDependencyTask(dependencyTask, task); let taskResult; const commonKey = dependencyTask.getCommonTaskId(); - if (liveDependencies.has(commonKey)) { + if (nextLiveDependencies.has(commonKey)) { this._showDependencyCycleMessage(dependencyTask); taskResult = Promise.resolve({}); } else { @@ -554,7 +554,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { } if (!taskResult) { this._fireTaskEvent(TaskEvent.create(TaskEventKind.DependsOnStarted, task)); - taskResult = this._executeDependencyTask(dependencyTask, resolver, trigger, liveDependencies, encounteredTasks, alreadyResolved); + taskResult = this._executeDependencyTask(dependencyTask, resolver, trigger, nextLiveDependencies, encounteredTasks, alreadyResolved); } encounteredTasks.set(commonKey, taskResult); promises.push(taskResult); @@ -600,7 +600,6 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { }); } }).finally(() => { - liveDependencies.delete(task.getCommonTaskId()); if (this._activeTasks[mapKey] === activeTask) { delete this._activeTasks[mapKey]; } From 05dce4918275b5e568a52c8c4e318f36dde0bf1f Mon Sep 17 00:00:00 2001 From: meganrogge Date: Tue, 2 May 2023 14:31:11 -0700 Subject: [PATCH 10/12] apply suggestions --- .../contrib/tasks/browser/abstractTaskService.ts | 11 +++-------- .../contrib/tasks/browser/terminalTaskSystem.ts | 4 ++-- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts b/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts index e670705d52479..81f934a6131cd 100644 --- a/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts +++ b/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts @@ -324,15 +324,10 @@ export abstract class AbstractTaskService extends Disposable implements ITaskSer } return task._label; }); - this._lifecycleService.onBeforeShutdown(e => { + this._lifecycleService.onBeforeShutdown(async e => { this._willRestart = e.reason !== ShutdownReason.RELOAD; - this.getActiveTasks().then(async (tasks) => { - for (const task of tasks) { - if (!task.configurationProperties.isBackground) { - await this.terminate(task); - } - } - }); + const tasks = (await this.getActiveTasks()).filter(t => t.configurationProperties.isBackground); + await Promise.all(tasks.map(t => this.terminate(t))); }); this._register(this.onDidStateChange(e => { if ((this._willRestart || e.exitReason === TerminalExitReason.User) && e.taskId) { diff --git a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts index 6bc6781d66018..7c3974cfb62ab 100644 --- a/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts +++ b/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts @@ -491,7 +491,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { public terminateAll(): Promise { const promises: Promise[] = []; - Object.entries(this._activeTasks).forEach(([key, terminalData]) => { + for (const [key, terminalData] of Object.entries(this._activeTasks)) { const terminal = terminalData.terminal; if (terminal) { promises.push(new Promise((resolve, reject) => { @@ -511,7 +511,7 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem { })); terminal.dispose(); } - }); + } return Promise.all(promises); } From 4c69f53dcc9752ede01dd305f0d9cad1bf8c6fb5 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Wed, 3 May 2023 10:05:11 -0700 Subject: [PATCH 11/12] separate issues --- src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts b/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts index 81f934a6131cd..6361489829af0 100644 --- a/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts +++ b/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts @@ -326,8 +326,6 @@ export abstract class AbstractTaskService extends Disposable implements ITaskSer }); this._lifecycleService.onBeforeShutdown(async e => { this._willRestart = e.reason !== ShutdownReason.RELOAD; - const tasks = (await this.getActiveTasks()).filter(t => t.configurationProperties.isBackground); - await Promise.all(tasks.map(t => this.terminate(t))); }); this._register(this.onDidStateChange(e => { if ((this._willRestart || e.exitReason === TerminalExitReason.User) && e.taskId) { From 0a2835a73490a0173d3705bc1916e50af801412b Mon Sep 17 00:00:00 2001 From: meganrogge Date: Wed, 3 May 2023 18:25:44 -0700 Subject: [PATCH 12/12] revert change --- src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts b/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts index 6361489829af0..b0e9ee2cbc069 100644 --- a/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts +++ b/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts @@ -324,7 +324,7 @@ export abstract class AbstractTaskService extends Disposable implements ITaskSer } return task._label; }); - this._lifecycleService.onBeforeShutdown(async e => { + this._lifecycleService.onBeforeShutdown(e => { this._willRestart = e.reason !== ShutdownReason.RELOAD; }); this._register(this.onDidStateChange(e => {