Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make _activeTasks synchronous wrt _executeTask #180617

Merged
merged 14 commits into from
May 4, 2023
Merged

Make _activeTasks synchronous wrt _executeTask #180617

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c9fb898
Make _activeTasks synchronous wrt _executeTask
Apr 22, 2023
fe67dd6
Fix dependency cycle detection
Apr 24, 2023
fda6f9c
Remove some redundant code handling sequential dependencies
Apr 24, 2023
38ac734
Never launch more than one copy of a dependent task
Apr 25, 2023
73ea851
polish
meganrogge May 1, 2023
08929a2
rm from liveDependencies/encounteredTasks
meganrogge May 2, 2023
ff7d86a
rm accidental additions
meganrogge May 2, 2023
0476301
don't update encounteredTasks
meganrogge May 2, 2023
3b80fb2
Make it clear that the modification to liveDependencies is local
May 2, 2023
05dce49
apply suggestions
meganrogge May 2, 2023
9b32200
Merge branch 'main' into synchronous-execute-task
meganrogge May 3, 2023
4ef0c3f
Merge branch 'main' into synchronous-execute-task
meganrogge May 3, 2023
4c69f53
separate issues
meganrogge May 3, 2023
0a2835a
revert change
meganrogge May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +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;
const tasks = (await this.getActiveTasks()).filter(t => t.configurationProperties.isBackground);
await Promise.all(tasks.map(t => this.terminate(t)));
meganrogge marked this conversation as resolved.
Show resolved Hide resolved
});
this._register(this.onDidStateChange(e => {
if ((this._willRestart || e.exitReason === TerminalExitReason.User) && e.taskId) {
Expand Down
217 changes: 114 additions & 103 deletions src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts
View file
Open in desktop
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 @@ -294,13 +294,13 @@ 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) {
Copy link
Contributor Author

@markw65 markw65 May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong? Now a task with dependencies can be run as many times as you like, regardless of its instanceLimit.

There is another bug here, which is that a task with an instance limit can be launched as a dependent task without adding to its instance count, and then launched as a top level task, possibly exceeding its intstancelimit. The fix for that is to move the instance limit checking into _executeTask; and I started a diff to do that. But I wanted to let this settle first...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now a task with dependencies can be run as many times as you like, regardless of its instanceLimit.

When I tested, that was not the case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably because the ones that are already in encounteredTasks are returned here and not rerun https://github.com/markw65/vscode/blob/0476301a02badbe7d3a4ba2aa96599c66960e4d6/src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts#L552

Copy link
Contributor Author

@markw65 markw65 May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it does. Try this with a current build:

{
  "version": "2.0.0",
  "tasks": [
    {
      "type": "shell",
      "command": "echo dependent",
      "label": "dependent",
      "problemMatcher": []
    },
    {
      "type": "shell",
      "command": "echo Running top level task...;sleep 5;echo done",
      "label": "toplevel",
      "dependsOn": ["dependent"],
      "group": {
        "kind": "build",
        "isDefault": true
      },
      "problemMatcher": []
    }
  ]
}

Now hit Cmd-Shift-B a few times; each one will start a new task, rather than telling you the task is already running. It was already somewhat broken (#180541) before this commit; but without this specific change, this commit would have fixed #180541. Now its completely broken it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, sorry, I didn't get notified about your response until this morning for some reason...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markw65 In insider's, with these changes and that task, I cannot reproduce the issue you describe. I see a notification that the task is already active and am asked if i'd like to terminate or restart it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meganrogge Are you sure its in the insiders you're testing with? In order to get that popup, run has to return { kind: TaskExecuteKind.Active, ... }, and after the change above, it can only do that if the task has no dependents. Tasks with dependents simply ignore their instanceLimit.

If I checkout d980921 (main after this was merged), and build, the problem above repros. If I remove the && !task.configurationProperties.dependsOn?.length the problem goes away; and if I checkout main as of now, the problem has been fixed by #180546.

this._lastTask = this._currentTask;
return { kind: TaskExecuteKind.Active, task: terminalData.task, active: { same: true, background: task.configurationProperties.isBackground! }, promise: terminalData.promise };
}

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;
});
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?.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?.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,8 +425,8 @@ export class TerminalTaskSystem extends Disposable implements ITaskSystem {

public customExecutionComplete(task: Task, result: number): Promise<void> {
const activeTerminal = this._activeTasks[task.getMapKey()];
if (!activeTerminal) {
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<void>((resolve) => {
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) {
const terminal = activeTerminal.terminal;
if (!terminal) {
return Promise.resolve<ITaskTerminateResponse>({ success: false, task: undefined });
}
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];
for (const [key, terminalData] of Object.entries(this._activeTasks)) {
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();
});
this._activeTasks = Object.create(null);
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();
}
}
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> {
if (encounteredDependencies.has(task.getCommonTaskId())) {
this._showDependencyCycleMessage(task);
return {};
}

private _executeTask(task: Task, resolver: ITaskResolver, trigger: string, liveDependencies: Set<string>, encounteredTasks: Map<string, Promise<ITaskSummary>>, alreadyResolved?: Map<string, string>): Promise<ITaskSummary> {
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);
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) {
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 (nextLiveDependencies.has(commonKey)) {
this._showDependencyCycleMessage(dependencyTask);
taskResult = Promise.resolve<ITaskSummary>({});
} else {
promise = Promise.reject(promiseResult);
break;
taskResult = encounteredTasks.get(commonKey);
if (!taskResult) {
const key = dependencyTask.getMapKey();
taskResult = this._activeTasks[key] ? this._getDependencyPromise(this._activeTasks[key]) : undefined;
}
}
if (!taskResult) {
this._fireTaskEvent(TaskEvent.create(TaskEventKind.DependsOnStarted, task));
taskResult = this._executeDependencyTask(dependencyTask, resolver, trigger, nextLiveDependencies, encounteredTasks, alreadyResolved);
}
encounteredTasks.set(commonKey, taskResult);
promises.push(taskResult);
if (task.configurationProperties.dependsOrder === DependsOrder.sequence) {
const promiseResult = await taskResult;
if (promiseResult.exitCode !== 0) {
break;
}
}
} 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());
meganrogge marked this conversation as resolved.
Show resolved Hide resolved
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 => {
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 => {
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 @@ -637,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<string>, alreadyResolved?: Map<string, string>): Promise<ITaskSummary> {
private async _executeDependencyTask(task: Task, resolver: ITaskResolver, trigger: string, liveDependencies: Set<string>, encounteredTasks: Map<string, Promise<ITaskSummary>>, alreadyResolved?: Map<string, string>): Promise<ITaskSummary> {
// 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<string> {
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