Skip to content

TaskRouter class comment contradicts implementation: IMultiThreadableTask is not a routing signal #14790

Description

@ViktorHofer

Summary

The class-level doc comment on TaskRouter contradicts the implementation, and the contradiction is actively misleading people who are migrating tasks for multithreaded mode.

src/Build/BackEnd/Components/RequestBuilder/TaskRouter.cs lines 10-13:

/// Determines where a task should be executed in multi-threaded mode.
/// In multi-threaded execution mode, tasks implementing IMultiThreadableTask or marked with
/// MSBuildMultiThreadableTaskAttribute run in-process within thread nodes, while legacy tasks
/// are routed to sidecar TaskHost processes for isolation.

But NeedsTaskHostInMultiThreadedMode (line 53) is:

return !HasMultiThreadableTaskAttribute(taskType);

Attribute only. The interface plays no part in routing.

The method-level <remarks> on NeedsTaskHostInMultiThreadedMode is correct — it is specifically the class-level comment that is wrong.

Why this matters beyond a stale comment

The two conditions are not interchangeable, and the interface version cannot be made true:

ToolTask implements IMultiThreadableTask (src/Utilities/ToolTask.cs:61).

So if routing ever honoured the interface, it would opt in every ToolTask-derived task in the ecosystem — thousands, virtually all unmigrated. The attribute-only check is not a bug to be fixed; it is the only workable design. The comment describes a behaviour that would be unshippable.

Meanwhile the interface has a real and separate job: TaskEnvironment injection is gated solely on it (TaskExecutionHost.cs:545):

if (TaskInstance is IMultiThreadableTask multiThreadableTask)
{
    multiThreadableTask.TaskEnvironment = TaskEnvironment;
}

That makes "implements the interface, no attribute" a meaningful and useful state: a task whose path handling has been migrated but whose overall thread-safety is not yet established. It gets correct path resolution while still being isolated in a TaskHost.

Impact

I am migrating Arcade's tasks to multithreaded mode (dotnet/arcade#17381). Eight separate review comments on that PR cited this comment to argue that interface-only tasks are "relying on a router bug" and would be "silently opted in by a future MSBuild fix", and asked me to remove the interface — which would have silently reverted path resolution on those tasks while leaving them exactly as unsafe.

Suggested fix

Reword the class comment to match the implementation and distinguish the two mechanisms, e.g.:

/// Determines where a task should be executed in multi-threaded mode.
/// Tasks marked with MSBuildMultiThreadableTaskAttribute run in-process within thread nodes;
/// all other tasks are routed to sidecar TaskHost processes for isolation.
/// Note that IMultiThreadableTask does not affect routing -- it controls TaskEnvironment
/// injection (see TaskExecutionHost), and ToolTask implements it, so it could not be used
/// as a routing signal without opting in every ToolTask-derived task.

Happy to send a PR if that wording looks right.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions