Make _activeTasks synchronous wrt _executeTask #180617

markw65 · 2023-04-22T19:44:57Z

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.

markw65 · 2023-04-25T00:41:01Z

This still isn't right. This version once again reports false dependency cycles. I'm working on a fix.

Fixes microsoft#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.

The way dependency cycles were detected no longer works now that activeTasks is populated early. Fix it.

markw65 · 2023-04-25T02:50:04Z

Sorry for the churn. This version fixes both issues:

it won't report false dependencies when multiple tasks depend on a single subtask (A depends on B and C, and B and C each depend on D)
it will report actual cycles (A depends on B depends on A).

meganrogge · 2023-04-25T02:52:16Z

Thanks so much for looking into this. We have endgame this week, so i'll test your PRs after that.

Fixes microsoft#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.

markw65 · 2023-04-25T22:20:10Z

I just added a commit that fixes #180850 - but I can't see a way to link this pull request to that issue...

meganrogge · 2023-05-01T20:25:59Z

This works well, thank you!

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts

markw65

This change breaks 180850. I'm not allowed to request changes apparently...

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts

markw65 · 2023-05-02T14:44:16Z

@meganrogge you marked this as fixing #180243 - but it doesn't (it doesn't even address it). It does fix #180850 though.

meganrogge · 2023-05-02T14:45:51Z

@markw65 it does fix that with this change

https://github.com/microsoft/vscode/pull/180617/files#diff-f77e08d48fa5a43f6cd385c0bac2c04ee2123565ead5f0f4b60e23c8af821b76R329-R335

markw65 · 2023-05-02T14:55:25Z

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 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...

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.

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

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...

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

@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.

@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.

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts

markw65 · 2023-05-02T15:05:34Z

@markw65 it does fix that with this change

Ah - I missed that.

Tyriar

@meganrogge I'm not totally clear what's going on in this change, shall we talk through it tomorrow?

src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts

src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts

meganrogge · 2023-05-02T14:59:58Z

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) {


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.

meganrogge · 2023-05-02T15:02:32Z

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) {


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

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts

src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts

markw65 mentioned this pull request Apr 22, 2023

Properly synchronise terminalTaskSystem._activeTasks #180581

Closed

markw65 force-pushed the synchronous-execute-task branch from 5413180 to 39a1a6c Compare April 22, 2023 19:57

vscodenpa assigned meganrogge Apr 22, 2023

meganrogge added this to the May 2023 milestone Apr 24, 2023

markw65 force-pushed the synchronous-execute-task branch 2 times, most recently from edde211 to a473d38 Compare April 24, 2023 22:25

markw65 added 3 commits April 24, 2023 19:42

Fix dependency cycle detection

fe67dd6

The way dependency cycles were detected no longer works now that activeTasks is populated early. Fix it.

Remove some redundant code handling sequential dependencies

fda6f9c

markw65 force-pushed the synchronous-execute-task branch from 8b4ca12 to fda6f9c Compare April 25, 2023 02:43

markw65 mentioned this pull request Apr 25, 2023

Dependency tasks can run more than once #180850

Closed

polish

73ea851

meganrogge previously approved these changes May 1, 2023

View reviewed changes

meganrogge requested a review from alexr00 May 1, 2023 20:26

alexr00 reviewed May 2, 2023

View reviewed changes

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts Outdated Show resolved Hide resolved

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts Show resolved Hide resolved

meganrogge requested a review from alexr00 May 2, 2023 12:38

meganrogge dismissed their stale review via 4cfa4f8 May 2, 2023 12:38

rm from liveDependencies/encounteredTasks

08929a2

meganrogge force-pushed the synchronous-execute-task branch from 4cfa4f8 to 08929a2 Compare May 2, 2023 12:39

rm accidental additions

ff7d86a

meganrogge force-pushed the synchronous-execute-task branch from da9676e to ff7d86a Compare May 2, 2023 12:50

meganrogge requested a review from Tyriar May 2, 2023 13:00

markw65 commented May 2, 2023

View reviewed changes

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts Outdated Show resolved Hide resolved

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts Outdated Show resolved Hide resolved

don't update encounteredTasks

0476301

markw65 commented May 2, 2023

View reviewed changes

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts Outdated Show resolved Hide resolved

Make it clear that the modification to liveDependencies is local

3b80fb2

Tyriar reviewed May 2, 2023

View reviewed changes

src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts Outdated Show resolved Hide resolved

src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts Outdated Show resolved Hide resolved

src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts Outdated Show resolved Hide resolved

apply suggestions

05dce49

markw65 commented May 2, 2023

View reviewed changes

src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts Outdated Show resolved Hide resolved

meganrogge and others added 4 commits May 3, 2023 07:28

Merge branch 'main' into synchronous-execute-task

9b32200

Merge branch 'main' into synchronous-execute-task

4ef0c3f

separate issues

4c69f53

revert change

0a2835a

meganrogge requested a review from Tyriar May 4, 2023 01:29

meganrogge approved these changes May 4, 2023

View reviewed changes

alexr00 approved these changes May 4, 2023

View reviewed changes

meganrogge merged commit d980921 into microsoft:main May 4, 2023

markw65 mentioned this pull request May 4, 2023

Fix a task startup race #180546

Merged

markw65 deleted the synchronous-execute-task branch May 10, 2023 21:41

github-actions bot locked and limited conversation to collaborators Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make _activeTasks synchronous wrt _executeTask #180617

Make _activeTasks synchronous wrt _executeTask #180617

markw65 commented Apr 22, 2023 •

edited by meganrogge

Loading

markw65 commented Apr 25, 2023

markw65 commented Apr 25, 2023

meganrogge commented Apr 25, 2023

markw65 commented Apr 25, 2023

meganrogge commented May 1, 2023

markw65 left a comment

markw65 commented May 2, 2023

meganrogge commented May 2, 2023

markw65 May 2, 2023 •

edited

Loading

meganrogge May 2, 2023

meganrogge May 2, 2023

markw65 May 4, 2023 •

edited

Loading

markw65 May 4, 2023

meganrogge May 4, 2023

markw65 May 4, 2023

markw65 commented May 2, 2023

Tyriar left a comment

meganrogge May 2, 2023

meganrogge May 2, 2023

Make _activeTasks synchronous wrt _executeTask #180617

Make _activeTasks synchronous wrt _executeTask #180617

Conversation

markw65 commented Apr 22, 2023 • edited by meganrogge Loading

markw65 commented Apr 25, 2023

markw65 commented Apr 25, 2023

meganrogge commented Apr 25, 2023

markw65 commented Apr 25, 2023

meganrogge commented May 1, 2023

markw65 left a comment

Choose a reason for hiding this comment

markw65 commented May 2, 2023

meganrogge commented May 2, 2023

markw65 May 2, 2023 • edited Loading

Choose a reason for hiding this comment

meganrogge May 2, 2023

Choose a reason for hiding this comment

meganrogge May 2, 2023

Choose a reason for hiding this comment

markw65 May 4, 2023 • edited Loading

Choose a reason for hiding this comment

markw65 May 4, 2023

Choose a reason for hiding this comment

meganrogge May 4, 2023

Choose a reason for hiding this comment

markw65 May 4, 2023

Choose a reason for hiding this comment

markw65 commented May 2, 2023

Tyriar left a comment

Choose a reason for hiding this comment

meganrogge May 2, 2023

Choose a reason for hiding this comment

meganrogge May 2, 2023

Choose a reason for hiding this comment

markw65 commented Apr 22, 2023 •

edited by meganrogge

Loading

markw65 May 2, 2023 •

edited

Loading

markw65 May 4, 2023 •

edited

Loading