Only fire workspace task events for effective scope

[colin-grant-work]

What it does

Fixes a minor, intermittent bug that causes tasks in folder scope to be double listed as occurring in both folder and workspace scope in an unsaved (single-root) workspace.

How to test

The bug depends on a race condition, but the basic issue is this:

• At lines 237-241 below, we enforce that we only fire events for workspace scope if the workspace is saved.
• At line 243, we fire an event for workspace scope unconditionally, which contradicts our intent from 237-241.

theia/packages/task/src/browser/task-configuration-manager.ts

Lines 227 to 246 in 008c2a1

	protected updateWorkspaceModel(): void {
	const newDelegate = this.workspaceService.saved ? this.workspacePreferences : this.folderPreferences;
	const effectiveScope = this.workspaceService.saved ? TaskScope.Workspace : this.workspaceService.tryGetRoots()[0]?.resource.toString();
	if (newDelegate !== this.workspaceDelegate) {
	this.workspaceDelegate = newDelegate;
	this.toDisposeOnDelegateChange.dispose();
	const workspaceModel = new TaskConfigurationModel(effectiveScope, newDelegate);
	this.toDisposeOnDelegateChange.push(workspaceModel);
	// If the delegate is the folder preference provider, its events will be relayed via the folder scope models.
	if (newDelegate === this.workspacePreferences) {
	this.toDisposeOnDelegateChange.push(workspaceModel.onDidChange(() => {
	this.onDidChangeTaskConfigEmitter.fire({ scope: TaskScope.Workspace, type: FileChangeType.UPDATED });
	}));
	}
	this.models.set(TaskScope.Workspace, workspaceModel);
	this.onDidChangeTaskConfigEmitter.fire({ scope: TaskScope.Workspace, type: FileChangeType.UPDATED });
	}
	}
	}

When we fire those events, this listener

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

Lines 97 to 107 in 008c2a1

	this.taskConfigurationManager.onDidChangeTaskConfig(async change => {
	try {
	await this.onDidTaskFileChange([change]);
	if (this.client) {
	this.client.taskConfigurationChanged(this.getTaskLabels());
	}
	} catch (err) {
	console.error(err);
	}
	})
	);

kicks in and populates a list of tasks by scope. We always fire events for folder scope, which is why we only want to fire them for workspace scope if it's different.

The race condition comes in because if the preference provider for newDelegate happens not to have read its configuration when the event on line 243 is fired, then the listener finds no tasks for that scope, and so we don't see any duplicates. If the preference provider has read its configuration, then we get the same tasks listed in both folder and workspace scope.

If you really want to reproduce the bug, replace line 243 with

newDelegate.ready.then(() => this.onDidChangeTaskConfigEmitter.fire({ scope: TaskScope.Workspace, type: FileChangeType.UPDATED }));

which will guarantee that the configuration has been read then

1. Open a single-root, unsaved workspace with one or more tasks defined in a tasks.json
2. Open the Terminal menu and select 'Run task...'
3. Observe that the tasks defined in the tasks.json are all listed twice, once in workspace scope, once in folder scope.

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

Signed-off-by: Colin Grant [email protected]

[vince-fugnitto]

LGTM! 👍

I confirmed the bug exists on master (using the method to reproduce it consistently)

and that the pull-request addresses the problem: