Introduce a token to scope contributed tasks

[tsmaeder]

Signed-off-by: Thomas Mäder tmader@redhat.com

What it does

This PR fixes "Run tasks" calls same Task Provider repeatedly. #7496
This PR is based on the idea that each user-level operation ("Run Task...", "Run Build Task") needs to obtain a token before using contributed tasks. There is a cache of contributed tasks in "ProvidedTaskConfigurations" that is used for as long as the token passed in is the same as the one used when first fetching the tasks.

How to test

Test the various task-related operations with tasks that are contributed by extensions, like "npm" or "yarn" tasks.

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

[tsmaeder]

Ping @akosyakov @RomanNikitenko

[akosyakov]

I wonder is there a way to restructure that it does not call expensive methods often instead of passing token. It feels somehow like a broken cache abstraction.

We have CancellationToken btw, maybe that could be used to indicate the operation as the last argument, we do it like that in other APIs.

[RomanNikitenko]

Maybe it makes sense to investigate VS Code approach for this problem.
I mean - if VS Code has cache for tasks and how it handled.

[tsmaeder]

@akosyakov the "restructure until we have a set of root methods" was my first approach, however, I could not make it work: the call graph has just a lot of entry points initiated by the user.
Instead of passing a token, we could also fetch the tasks and then pass them around as a parameter. However, that would suggest that the set of tasks can be different for different invocations, but that is not what we want.
I would agree that we could probably clean up the call graph around tasks a little, but the basic problem that we have an essentially arbitrary cache scope will not go away.
But I would love to be proven wrong: what did you have in mind?

[tsmaeder]

@RomanNikitenko VS Code fetches tasks at the beginning of user actions and then passes them around. In order to do that, we would have to do quite a large refactoring. I would totally love that, but it's a major undertaking whereas this PR is s relatively limited bugfix.

[akosyakov]

Maybe it makes sense to investigate VS Code approach for this problem.
I mean - if VS Code has cache for tasks and how it handled.

I could not find caching in VS Code. That seems to be a method which is used by all clients to get tasks: https://github.com/microsoft/vscode/blob/e683ace9e5acadba0e8bde72d793cb2cb83e58a7/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts#L1564 and it always calls provideTasks

Instead of passing a token, we could also fetch the tasks and then pass them around as a parameter. However, that would suggest that the set of tasks can be different for different invocations, but that is not what we want.

Maybe it is what actually VS Code does.

If these APIs are not used often (what I don't expect). We could move with the proposed solution and later reconsider.

[tsmaeder]

Maybe it is what actually VS Code does.

Yes, VS code fetches the tasks once and then passes them around. However, our internal API (TaskService) is very much different, that's why we would have to refactor quite a bit for this to work. It would also entail a breaking change.

[akosyakov]

It would also entail a breaking change.

true, but this PR is a breaking change as well. I do feel though that eventually we will have to rewrite it to fix task identity and caching issues properly, so breaking is fine.

@RomanNikitenko could you test please? if it works fine then we merge it for now?

[RomanNikitenko]

@RomanNikitenko could you test please?

I'm able to check it today.

[RomanNikitenko]

Tested for npm tasks:

• I was able to run a npm task as detected task as well as a configured one.
• Configure Task action worked as well
• I added some logs to check if cached tasks were used properly

The dark side of this approach - we had to add token to multiple tasks related methods...
btw: it would be nice to add some doc to methods which describes what the token is, why we need it and how it should be used.

[tsmaeder]

btw: it would be nice to add some doc to methods which describes what the token is, why we need it and how it should be used.

That is true: documentation in PR's is kinda useless once it's merged.

[RomanNikitenko]

That is true: documentation in PR's is kinda useless once it's merged.

@tsmaeder
token was added to multiple tasks related methods and from my point of view It's not obviously what the token is and how it should be used.
So I meant adding doc to code base to the corresponding methods, not to the PR description.

btw: detailed description in a PR is very helpful for me as well:

• on review step
• at working on another issue close to a PR changes - to understand why some changes were provided, what use cases were covered and so on.

So, I'm sorry, I'm afraid I can't agree with you 😉

[tsmaeder]

@RomanNikitenko I added documention for the token parameter

[RomanNikitenko]

I tested the changes and commented the result here

@tsmaeder
Thank you for the documentation of the corresponding methods - looks good to me!
Please fix some errors before merge: