TaskService.runTask(...) not safe for parallel Execution

[tsmaeder]

Bug Description:

When invoking TaskService.runTask(...) twice in a row for the same task, you can run into a race condition: the second execution of the method will not detect that the task is already running and therefore we end up executing the task twice instead of asking the user to cancel/restart the task.

Steps to Reproduce:

I don't have an upstream reproducing scenario right now, but in Che, it reproduces since we have a tree view that you can just click on to run tasks. Double-clicking reproduces the issue: eclipse-che/che#19821

Additional Information

• Operating System: linux
• Theia Version: master

[tsmaeder]

The run task method looks something like this:

    async runTask(task: TaskConfiguration, option?: RunTaskOption): Promise<TaskInfo | undefined> {
        const runningTasksInfo: TaskInfo[] = await this.getRunningTasks();

        if (findRunningTask(runningTaskInfo, task) { // the task is active
            const selectedAction = await this.messageService.info(`The task '${taskName}' is already active`, 'Terminate Task', 'Restart Task');
            if (selectedAction === 'Terminate Task') {
                await this.terminateTask(matchedRunningTaskInfo);
            } else if (selectedAction === 'Restart Task') {
                return this.restartTask(matchedRunningTaskInfo, option);
            }
        } else { // run task as the task is not active
            return this.doRunTask(task, option);
        }
    }

The trouble is that this code is really equivalent to this one:

runTask(task: TaskConfiguration, option?: RunTaskOption): Promise<TaskInfo | undefined> {
        return this.getRunningTasks().then(runningTaskInfo => {

           if (findRunningTask(runningTaskInfo, task) { // the task is active
               const selectedAction = await this.messageService.info(`The task '${taskName}' is already active`, 'Terminate Task', 'Restart Task');
          ...
      });
}

So the method immediately returns and can process the next mouse click and before we've added the newly started task to the set of running tasks.

[vince-fugnitto]

@tsmaeder do you know if che is using the latest version, I believe it might be fixed by the update to the way we compareTasks:

• Enhancement: Supporting deep comparison of tasks with reference value in custom properties #9647

[tsmaeder]

@vince-fugnitto I don't believe comparisons have anything to do with it, otherwise the problem would not be timing-dependent. The fact that we're not seeing the problem upstream is due to the fact that the UI for starting a task is not immediate like a mouse click.

[tsmaeder]

I haven't been able to reproduce with a pure plugin approach so far: however, I have no trouble when I change TestService.runTask(...) like so:

    protected delay<T>(ms: number): (value: T) => Promise<T> {
        return value => new Promise((resolve, reject) => { setTimeout(() => resolve(value), ms); });
    };

    async runTask(task: TaskConfiguration, option?: RunTaskOption): Promise<TaskInfo | undefined> {
        const runningTasksInfo: TaskInfo[] = await this.getRunningTasks().then(this.delay(700));
        console.info('got running tasks, size is ' + runningTasksInfo.length);

so this correct functioning is dependent on the exact promise we're getting back from getRunningTasks(). Not sure whether the issue is with the delay time or with the fact that we have two chained promises.

@caseyflynn-google this is an exampl of why I'm not a fan of async/wait: it makes it too easy to fall into the trap of thinking the code is synchronous when it can be in fact a complex interleaving of various continuations.

[RomanNikitenko]

About che tasks:
There was attempt to fix/workaround the problem on the tree-view-widget.tsx side: #9546

Also, there is a comment from Colin:

This is another entry in the ongoing struggle to handle tree clicks better. (here and here, among others).

[tsmaeder]

I'm aware of that, but using debounce will fail again when the system is slow and it's a general solution for a specific problem. The tree widget is not the source of the problem.

[tsmaeder]

I was able to reproduce the issue by making the TaskServerImpl.getTasks() method slower like so:

    protected delay<T>(ms: number): (value: T) => Promise<T> {
        return value => new Promise((resolve, reject) => { setTimeout(() => resolve(value), ms); });
    };

    async getTasks(context?: string): Promise<TaskInfo[]> {
        const taskInfo: TaskInfo[] = [];
        const tasks = this.taskManager.getTasks(context);
        if (tasks !== undefined) {
            for (const task of tasks) {
                taskInfo.push(await task.getRuntimeInfo());
            }
        }
        this.logger.debug(`getTasks(): about to return task information for ${taskInfo.length} tasks`);

        return Promise.resolve(taskInfo).then(this.delay(700));
    }

So the issue is real and it's a timing issue:

[tsmaeder]

prefs-explorer-0.0.1.vsix.zip

The attached plugin adds a view to the navigator showing all tasks. With the above delay in the back end, multi-clicking on tasks in the view will run multiple copies of the same task (timing dependent, of course)

[tsmaeder]

The source lives here: https://github.com/tsmaeder/prefs-explorer