Introduce proper custom execution lifecycle #9559

tsmaeder · 2021-06-07T09:34:01Z

What it does

Fixes "Review CustomExecution Lifecycle" #9418 by introducing a proper lifecycle for custom execution objects. The approach is detailed in the linked issue.

How to test

I've tested with the extensions linked off #8767 and #9366 as well as the npm tasks from the typescript built-in.
I was executing tasks an observing the back end log to make sure custom executions were being created and disposed of.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

colin-grant-work

The comments below are only superficial, as I'm not very familiar with how tasks are passed into the plugin system by plugins. It appears that they come as functions to execute when the task is activated, and in the past (when #8767 was filed) we had no mechanism for handling those, and after creating such a mechanism, we failed to handle the creation and deletion of those objects correctly (--> #9366)? I see some discussion of VSCode's approach in #9416 (very helpful, thanks!), but I don't have any intuition about the occasions Theia accesses these objects or what expectations plugins might have about their durability. Beyond the issues addressed, I wonder if you might be willing to explain the functionality you mean to provide and the problems you mean to solve / avoid?

packages/plugin-ext/src/main/browser/tasks-main.ts

packages/plugin-ext/src/plugin/tasks/tasks.ts

colin-grant-work · 2021-06-10T22:08:54Z

packages/plugin-ext/src/plugin/tasks/tasks.ts

+        const result = this.providedCustomExecutions.get(id);
+        return result ? result : this.oneOffCustomExecutions.get(id);


Is this different from

return this.providedCustomExecutions.get(id) ?? this.oneOffCustomExecutions.get(id)?

Typescript keeps keeping ahead of me.

colin-grant-work · 2021-06-10T22:13:05Z

packages/plugin-ext/src/plugin/tasks/tasks.ts

+            return Promise.resolve(provider.provideTasks(token)).then(tasks => {
                if (tasks) {
-                    for (const task of tasks) {
-                        if (task.taskType === 'customExecution') {
-                            task.executionId = this.addCustomExecution(task.callback);
-                            task.callback = undefined;
+                    return tasks.map(task => {
+                        const dto = converter.fromTask(task);
+                        if (dto && CustomExecution.is(task.execution!)) {
+                            dto.executionId = this.addProvidedCustomExecution(task.execution);
+                            addedExecutions++;
                        }
-                    }
+                        return dto;
+                    }).filter(task => !!task).map(task => task!);
+                } else {
+                    return undefined;
                }
+            }).then(tasks => {
+                console.info(`provideTasks: added ${addedExecutions} executions`);
                return tasks;
            });


This might be clearer as an async function rather than chained promises.
Same immediately below, though that one's less complex.

t.b.h, I'm not a huge fan of async/await in particular when the control flow is complicated. A simple reason for it is when we get stack traces from the compiled javascript and it's a PITA to map it back to the original source. But it also sometimes obscures the true control flow. YMMV, of course.

I agree about the stack traces, most definitely. __awaiter__ __push__ __generator__ is not a very helpful stack at all. I think some of that noise should go away with the move to transpile to ES6, since at least some of it is TypeScript paraphrasing promises, though I'm not 100% certain how the traces will look later. I won't insist on async, but I will present this evidence:

Google for "callback hell": 95,500 results
Google for "async await hell": 3,690 results

:-)

+1 for async-await - it's more readable for me :-)

colin-grant-work · 2021-06-10T22:16:13Z

packages/plugin-ext/src/plugin/tasks/tasks.ts

                        }
-                    }
+                        return dto;
+                    }).filter(task => !!task).map(task => task!);


You can avoid the second .map here with something like this to help TS understand what you mean:

.filter((task): task is TaskDto => !!task)

packages/plugin-ext/src/plugin/type-converters.ts

packages/task/src/browser/provided-task-configurations.ts

tsmaeder · 2021-06-15T05:19:49Z

@vince-fugnitto @RomanNikitenko: @colin-grant-work is doing the best he can, but I'll need a review from someone who has good knowledge of the task system on this one.

tsmaeder · 2021-07-07T14:29:59Z

@RomanNikitenko @vince-fugnitto can I haz review pleeze?

vince-fugnitto · 2021-07-13T13:50:39Z

@tsmaeder I believe there are conflicts and some comments from Colin which are not yet addressed.
I'll be happy to review afterwards, even with my limited knowledge of the specifics of the task system :)

tsmaeder · 2021-07-14T13:44:24Z

@vince-fugnitto

vince-fugnitto

I'm currently in the process of testing if the changes work correctly for different tasks and plugins 👍

vince-fugnitto · 2021-07-14T13:38:10Z

packages/task/src/browser/provided-task-configurations.ts

    private currentToken: number = 0;
    private nextToken = 1;

-    startUserAction(): number {
-        return this.nextToken++;
+    get onStartUserInteraction(): Event<TaskStartUserInteractionEvent> {


minor: should we rename given the guidelines: https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#event_names?

Suggested change

get onStartUserInteraction(): Event<TaskStartUserInteractionEvent> {

get onDidStartUserInteraction(): Event<TaskStartUserInteractionEvent> {

vince-fugnitto

I confirmed that tasks from #9366 works successfully 👍 As for gradle, the tests cannot run due to the following error which seems unrelated?

colin-grant-work · 2021-07-14T14:35:33Z

I confirmed that tasks from #9366 works successfully 👍 As for gradle, the tests cannot run due to the following error which seems unrelated?

@kenneth-marut-work, you've been looking at CWD issues in tasks lately. Any idea what might be going on here?

tsmaeder · 2021-07-15T07:41:02Z

@vince-fugnitto do we have an issue for the cwd failures? Do you get a stack trace somewhere?

vince-fugnitto · 2021-07-15T12:31:56Z

@vince-fugnitto do we have an issue for the cwd failures? Do you get a stack trace somewhere?

Unfortunately there is no stack trace, but the issue is also present on master which makes it difficult to determine if custom execution actually work:

root INFO [hosted-plugin: 30220] resolveTask: added custom execution
task INFO Created new custom task, id: 0, context: file:///home/vince/Desktop/core-vocabularies
root INFO [hosted-plugin: 30220] running custom execution with id 2b3387de-c077-4b30-8cfc-2eb0053fd37e1
task ERROR Error launching task 'assemble': Cannot read property 'cwd' of undefined

kenneth-marut-work · 2021-07-15T14:11:55Z

I confirmed that tasks from #9366 works successfully 👍 As for gradle, the tests cannot run due to the following error which seems unrelated?

@kenneth-marut-work, you've been looking at CWD issues in tasks lately. Any idea what might be going on here?

Definitely looks familiar, but I'll have to refresh my memory, but I'll take a look later today to see if I can trace down anything

kenneth-marut-work · 2021-07-15T14:46:23Z

theia/packages/task/src/browser/task-terminal-widget-manager.ts

Line 199 in 8dd8401

const lastCwd = new URI(taskConfig?.options.cwd);

I had added this line in a recent PR #9695 which looks suspicious. Potential fix would be

const lastCwd = new URI(taskConfig?.options?.cwd);

tsmaeder · 2021-07-15T14:53:45Z

@kenneth-marut-work I've filed #9743.

packages/plugin-ext/src/plugin/type-converters.ts

packages/plugin-ext/src/plugin/tasks/tasks.ts

RomanNikitenko · 2021-08-04T20:22:23Z

@tsmaeder
I have a question about maps for executions:
what's relationship between executions vs providedCustomExecutions and oneOffCustomExecutions?
For example, I see that values from executions map are returned when theia.tasks.taskExecutions or vscode.tasks.taskExecutions API is called. So, I guess, executions map should contain all types of executions. Is it correct? If so - why do we need providedCustomExecutions?

RomanNikitenko · 2021-08-04T20:47:18Z

Use case:

run a long task with CustomExecution
refresh a page
the task is still running
I guess providedCustomExecutions and oneOffCustomExecutions maps are empty after refreshing a page

what's the true way to get execution for the task?

RomanNikitenko · 2021-08-04T20:55:30Z

packages/plugin-ext/src/plugin/tasks/tasks.ts

+                this.proxy.$terminateTask(execution.id);
+            }
+        };
+        if (execution.task.executionId) {


I see that you use executionId field to check if it's CustomExecution task.
So - if only tasks with CustomExecution have this field - should we provide some clear/more readable way for that...

What do you have in mind? I'm not trying to do any typing here.

only tasks with CustomExecution have this field

is it true?

As far as I know a task of any type (not only customExecution) can have execution - the object is created when a task is started ($onDidStartTask() => getTaskExecution()) and is stored to the map with executions using executionId.

I see that:

within one of your previous PRs executionId field was added to the TaskDto interface

within current PR you are using the field as a marker of a task with customExecution
first, you create the field, like:

if (dto && CustomExecution.is(task.execution!)) { dto.executionId = this.addProvidedCustomExecution(task.execution);

and in the line 281 above you check - if the field exists - then result.task.execution = this.getCustomExecution(execution.task.executionId);)

So, in my comment above I meant that for me it's not obvious that executionId field of the TaskDto interface is a marker for customExecution tasks.

for me it's not obvious that executionId field of the TaskDto interface is a marker for customExecution tasks.

If you think the executionId field is named confusingly, I'm all up for renaming it to something more clear. But since it wasn't introduced in this PR, it's not a concern for this PR.

it's not a concern for this PR

sorry, but for me it looks like within current PR you are trying to use common for all tasks field as a marker for customExecution task. My first comment here was about if (execution.task.executionId).

it would be nice to clarify Introduce proper custom execution lifecycle #9559 (comment) first

If you think the executionId field is named confusingly

If I'm not mistaken, this field was introduced within one of your previous PR. If so, it's important to know what you think about it, what intention was...

tsmaeder · 2021-08-05T08:06:28Z

@tsmaeder
I have a question about maps for executions:
what's relationship between executions vs providedCustomExecutions and oneOffCustomExecutions?
For example, I see that values from executions map are returned when theia.tasks.taskExecutions or vscode.tasks.taskExecutions API is called. So, I guess, executions map should contain all types of executions. Is it correct? If so - why do we need providedCustomExecutions?

"executions" just keep track of the running tasks, whereas the other maps hold on to custom executions that clients have provided. The terminology is confusing, but that comes from the use of those terms in the vscode API itself 🤷. "TaskExecution" vs. "CustomExecution".

RomanNikitenko · 2021-08-05T18:31:47Z

@tsmaeder
could you share your opinion about #9559 (comment)

tsmaeder · 2021-08-09T07:33:19Z

Use case:

* run a long task with `CustomExecution`

* refresh a page

* the task is still running

* I guess `providedCustomExecutions` and `oneOffCustomExecutions` maps are empty after refreshing a page

what's the true way to get execution for the task?

That really does not matter: we are holding on to the custom execution objects for when the front end asks us to execute a particular task. When we reload the front end, all tasks will be recreated, and existing custom execution objects are no longer needed.

The whole confusion arises because "task execution" and "custom execution" are totally different things. "Task execution" is the representation of a running task. A "custom execution" is really just a callback that will be invoked when running a certain task.

tsmaeder · 2021-08-09T07:34:01Z

@vince-fugnitto @RomanNikitenko is on vacation now. It's been > 2 months.

vince-fugnitto · 2021-08-09T12:05:10Z

@vince-fugnitto @RomanNikitenko is on vacation now. It's been > 2 months.

@tsmaeder it's not an area I am particularly knowledgeable about but I'll give it a try.

tsmaeder · 2021-09-06T08:12:43Z

@RomanNikitenko ping

RomanNikitenko · 2021-09-06T09:19:14Z

@RomanNikitenko ping

I'm going to revisit the PR in the near few days...

RomanNikitenko · 2021-09-28T07:04:33Z

@tsmaeder
Hello, Thomas, sorry for the delay...

To be honest, I don't have an experience related to customExecution.
I explored the changes and your idea here - it looks good to me!

At the same time I'm still not sure about usage of executionId field, commented above.

Could you rebase your branch - I could test your changes, although, looking at the changes, I don't expect any regressions...
Thanks in advance!

Signed-off-by: Thomas Mäder <[email protected]>

Signed-off-by: Thomas Mäder <[email protected]> Co-authored-by: colin-grant-work <[email protected]>

Signed-off-by: Thomas Mäder <[email protected]>

tsmaeder · 2022-03-07T15:55:38Z

I still think this is a good PR. What's missing is someone to review it: for obvious reasons, we can't expect @RomanNikitenko to do it any time soon.

tsmaeder · 2023-05-30T12:11:00Z

Not likely to get merged anytime soon.

tsmaeder requested review from RomanNikitenko and vince-fugnitto June 7, 2021 09:34

tsmaeder mentioned this pull request Jun 7, 2021

Plugins Sprint 202 eclipse-che/che#19840

Closed

29 tasks

colin-grant-work added the tasks issues related to the task system label Jun 7, 2021

colin-grant-work changed the title ~~Introduce proper custom exceution lifecycle~~ Introduce proper custom execution lifecycle Jun 7, 2021

colin-grant-work reviewed Jun 10, 2021

View reviewed changes

anavarre mentioned this pull request Jun 11, 2021

Git UI plugin only accepts passwordless SSH keys #9575

Closed

ericwill mentioned this pull request Jul 7, 2021

Plugins Sprint 204 eclipse-che/che#20055

Closed

22 tasks

tsmaeder force-pushed the 9418_custom_execution_lifecycle branch from 2300dd9 to 11e2183 Compare July 14, 2021 11:46

vince-fugnitto reviewed Jul 14, 2021

View reviewed changes

svor mentioned this pull request Jul 22, 2021

Plugins Sprint 205 eclipse-che/che#20186

Closed

34 tasks

tsmaeder force-pushed the 9418_custom_execution_lifecycle branch from c70655b to f8d571f Compare July 27, 2021 13:32

azatsarynnyy mentioned this pull request Jul 28, 2021

Editors Sprint 205 eclipse-che/che#20185

Closed

1 task

RomanNikitenko reviewed Jul 31, 2021

View reviewed changes

packages/plugin-ext/src/plugin/type-converters.ts Show resolved Hide resolved

RomanNikitenko reviewed Aug 3, 2021

View reviewed changes

packages/plugin-ext/src/plugin/tasks/tasks.ts Show resolved Hide resolved

RomanNikitenko reviewed Aug 4, 2021

View reviewed changes

packages/plugin-ext/src/plugin/tasks/tasks.ts Show resolved Hide resolved

RomanNikitenko reviewed Aug 4, 2021

View reviewed changes

svor mentioned this pull request Aug 12, 2021

Plugins Sprint 206 eclipse-che/che#20290

Closed

30 tasks

svor mentioned this pull request Sep 7, 2021

Plugins Sprint 207 eclipse-che/che#20433

Closed

26 tasks

svor mentioned this pull request Sep 29, 2021

Plugins Sprint 208 eclipse-che/che#20531

Closed

25 tasks

tsmaeder and others added 4 commits October 4, 2021 11:45

Introduce proper custom exceution lifecycle

f818296

Signed-off-by: Thomas Mäder <[email protected]>

Fix typos

f78bdeb

Signed-off-by: Thomas Mäder <[email protected]> Co-authored-by: colin-grant-work <[email protected]>

Incorporating review suggestions

3d46c7e

Signed-off-by: Thomas Mäder <[email protected]>

Renamed event

606725c

Signed-off-by: Thomas Mäder <[email protected]>

tsmaeder force-pushed the 9418_custom_execution_lifecycle branch from f8d571f to 606725c Compare October 4, 2021 09:53

svor mentioned this pull request Oct 19, 2021

Plugins Sprint 209 eclipse-che/che#20653

Closed

28 tasks

svor mentioned this pull request Nov 9, 2021

Plugins Sprint 210 eclipse-che/che#20752

Closed

18 tasks

svor mentioned this pull request Nov 30, 2021

Plugins Sprint 211 eclipse-che/che#20848

Closed

19 tasks

JonasHelming assigned tsmaeder Mar 4, 2022

JonasHelming added the decision_needed issues that require decisions on ways forward (ex: if they should be updated or closed) label Mar 4, 2022

tsmaeder removed the decision_needed issues that require decisions on ways forward (ex: if they should be updated or closed) label Mar 29, 2022

tsmaeder closed this May 30, 2023

		const result = this.providedCustomExecutions.get(id);
		return result ? result : this.oneOffCustomExecutions.get(id);

	get onStartUserInteraction(): Event<TaskStartUserInteractionEvent> {
	get onDidStartUserInteraction(): Event<TaskStartUserInteractionEvent> {

Introduce proper custom execution lifecycle #9559

Introduce proper custom execution lifecycle #9559

Conversation

tsmaeder commented Jun 7, 2021

What it does

How to test

Review checklist

Reminder for reviewers

colin-grant-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Jun 15, 2021

tsmaeder commented Jul 7, 2021

vince-fugnitto commented Jul 13, 2021

tsmaeder commented Jul 14, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

colin-grant-work commented Jul 14, 2021

tsmaeder commented Jul 15, 2021

vince-fugnitto commented Jul 15, 2021

kenneth-marut-work commented Jul 15, 2021

kenneth-marut-work commented Jul 15, 2021

tsmaeder commented Jul 15, 2021

RomanNikitenko commented Aug 4, 2021

RomanNikitenko commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanNikitenko Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

RomanNikitenko Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Aug 5, 2021

RomanNikitenko commented Aug 5, 2021

tsmaeder commented Aug 9, 2021

tsmaeder commented Aug 9, 2021

vince-fugnitto commented Aug 9, 2021

tsmaeder commented Sep 6, 2021

RomanNikitenko commented Sep 6, 2021

RomanNikitenko commented Sep 28, 2021 • edited Loading

tsmaeder commented Mar 7, 2022

tsmaeder commented May 30, 2023

RomanNikitenko Aug 5, 2021 •

edited

Loading

RomanNikitenko Sep 27, 2021 •

edited

Loading

RomanNikitenko commented Sep 28, 2021 •

edited

Loading