"Run tasks" calls same Task Provider repeatedly.

[tsmaeder]

Description

I have written a VS Code extension that registers a task provider for type "foobar" and prints to the console when "provideTasks()" is called.
I have a workspace where I configure 5 tasks of type "npm" in /.theia/tasks.json. When I now run the command "Run Task", the console output shows that "provideTasks" is called 6 times before the dialog for selecting the task to run is opened.
In general, the number of times "provideTasks()" is called is n+1, where n is the number of tasks in "tasks.json".

[tsmaeder]

The culprit seems to be here:

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

Line 146 in 751a3d7

const detected = await this.providedTaskConfigurations.getTaskToCustomize(cus, rootFolder);

getTaskToCustomize() in turn calls getTasks(), which unconditionally fetches tasks from providers. We should really always use cached contributed tasks on only refetch when some action explicitly asks for it.

[akosyakov]

cc @elaihau

[tsmaeder]

Thinking about this (and my approach for a fix in #7633) a bit more...
Obviously, we would like to fetch tasks from task providers as few times as possible. This is because fetching them has a non-negligible performance impact (which brought me to file this issue).
There are two aspects to this issue: first: at which points in time can the set of contributed tasks change. And the second is: what are the consequences if we work with an outdated set of tasks.
The first question is easy to answer: any time. Since there are no restrictions on the implementation of task providers, we could implement one that returns a new set of randomly generated tasks each time we ask it. So we can never be 100% sure that the tasks we're working with are the correct set of tasks or that the task we have in hand have all their prerequisites met. That implies that when we execute a contributed task, we must check the prerequisites and fail the task when they are not met. This does not mean we have to explicitly check the preconditions: building an npm module that has been deleted, for example, may simply return a non-zero exit code.
However, often contributed tasks depend on the state of the project we're working on. That state is typically represented as files on disk. So while Theia is executing action on behalf of the user the set of tasks is unlikely to change. Also, the set of tasks is unlikely to change from second to second: typically the set of tasks changes because of something the user does, like checking out a project from git.
It turns out the second question has already been answered: since we never can be sure to have the correct set of contributed tasks, there are no negative consequences to working with outdated tasks: the only negative consequences are that we are not fulfilling user expectations. These expectations are that the set of tasks he wants to execute is available and that tasks presented by the system execute successfully.
So if there is no right "scope" we can use for keeping tasks ("caching"), what would be a reasonable scope? I would propose that a single user command ("Run Task...") would be that scope: we would fetch the set of tasks once per user action and keep the same set of tasks until that user command completes.

[tsmaeder]

In principle, there are two ways to establish such a scope:

1. Cache invalidation
   This is the approach used in my PoC PR: we keep an implicit scope somewhere and when we start a user action, we invalidate that cache. The advantage is that we don't have to pass the set of tasks around. The disadvantage is that we may miss a call site where we should invalidate the cache so that we work with old tasks.
2. Explicit Scope
   The approach is to never keep the set of tasks as state, but to fetch tasks once and have it as a required parameter to be passed around wherever the tasks are needed. The advantage is that ensure that a token is obtained by all clients of the task service: thus we are sure that no-one works with out-of-date tasks by accident. Downside is that this change is syntactically breaking. Note that with the implicit cache, the change is still behaviourally breaking, event though it still compiles.

[tsmaeder]

A first try was based on cache invalidation: #7633

[tsmaeder]

The second one it based on an explicit token.