-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Correct MT routing docs and add analyzer rules for mismatched declarations #14809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
b104c55
138adbd
1b3a222
3f7af01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,11 +10,24 @@ Tasks that are not thread-safe can still participate in multithreaded builds. MS | |
|
|
||
| ## Thread-Safe Capability Indicators | ||
|
|
||
| Task authors can declare thread-safe capabilities in two different ways: | ||
| 1. **Interface-Based Thread-Safe Capability Declaration** - Provides access to thread-safe APIs through `TaskEnvironment` to be used in the task code. | ||
| 2. **Attribute-Based Thread-Safe Capability Declaration** - Allows existing tasks to declare its ability run in multithreaded mode without code changes. It is a **compatibility bridge option**. | ||
| Task authors declare thread-safe capabilities through two mechanisms that do **different** jobs. They are not alternatives, and a fully migrated task uses both: | ||
|
|
||
| Tasks that use `TaskEnvironment` cannot load in older MSBuild versions that do not support multithreading features, requiring authors to drop support for older MSBuild versions. To address this challenge, MSBuild provides a compatibility bridge that allows certain tasks targeting older MSBuild versions to participate in multithreaded builds. While correct absolute path resolution can be and should be achieved without accessing `TaskEnvironment` in tasks that use compatibility bridge options, tasks must avoid relying on environment variables or modifying global process state. | ||
| 1. **Attribute-Based Thread-Safe Capability Declaration** (`[MSBuildMultiThreadableTask]`) — the **routing** signal. This is the only thing that opts a task into running in-process; without it the task is routed to an out-of-proc TaskHost sidecar regardless of anything else it declares. | ||
| 2. **Interface-Based Thread-Safe Capability Declaration** (`IMultiThreadableTask`) — the **injection** signal. It gives the task access to thread-safe APIs through `TaskEnvironment`, which the engine assigns only to tasks implementing the interface. | ||
|
|
||
| | Declaration | Effect | Read by | | ||
| | --- | --- | --- | | ||
| | `[MSBuildMultiThreadableTask]` | Runs in-process instead of an out-of-proc TaskHost | `TaskRouter.NeedsTaskHostInMultiThreadedMode` | | ||
| | `IMultiThreadableTask` | Receives a `TaskEnvironment` | `TaskExecutionHost` | | ||
|
|
||
| Declaring one without the other is legal, and each half fails quietly: | ||
|
|
||
| - **Attribute only** — a supported state. The task runs in-process without `TaskEnvironment`, which is correct for a task that does not resolve relative paths or read environment variables. If the task *does* declare a `TaskEnvironment` property, that property is never assigned and silently stays `TaskEnvironment.Fallback`, resolving every path against the shared process working directory. The task-authoring analyzer reports `MSBuildTask0012` for this shape. | ||
|
ViktorHofer marked this conversation as resolved.
Outdated
|
||
| - **Interface only** — a useful intermediate state. The task resolves paths correctly but still pays for a TaskHost. `MSBuildTask0013` reports it, disabled by default. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uff, out of my mind, I am not fully sure whether the msbuild would set the environment in this case. Probably yes. Can we verify that?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it does. Let me see if I can add a test for that.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified — and my "yes it does" above was wrong. Thanks for pushing on it. MSBuild does not assign the property in the TaskHost.
It still works, but through two mechanisms rather than injection:
So the "resolves paths correctly" claim holds, but the reason is process isolation plus the task's own default. The bullet now says that explicitly in 1b3a222. On the test: one already exists and covers exactly this — One corollary worth flagging: an |
||
|
|
||
| The interface cannot also serve as the routing signal, because `ToolTask` implements `IMultiThreadableTask`. Routing on the interface would opt in every `ToolTask`-derived task in the ecosystem, none of which have been reviewed for thread safety. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this information in specs? This is an alternative design question and not information what is happening in the code.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair. Moved to the existing |
||
|
|
||
| Tasks that use `TaskEnvironment` cannot load in older MSBuild versions that do not support multithreading features, requiring authors to drop support for older MSBuild versions. To address this challenge, MSBuild provides a compatibility bridge that allows certain tasks targeting older MSBuild versions to participate in multithreaded builds: the attribute is detected by name, so a task can apply it without referencing a new MSBuild assembly, and correct absolute path resolution can be and should be achieved without accessing `TaskEnvironment`. Tasks using that bridge must still avoid relying on environment variables or modifying global process state. | ||
|
|
||
| So, task authors who need to support older MSBuild versions will have three choices: | ||
| 1. **Maintain separate implementations** - Create and support both thread-safe and legacy versions of the same task. | ||
|
|
@@ -23,7 +36,7 @@ So, task authors who need to support older MSBuild versions will have three choi | |
|
|
||
| ### Interface-Based Thread-Safe Capability Declaration | ||
|
|
||
| Tasks indicate thread-safety capabilities by implementing the `IMultiThreadableTask` interface. | ||
| Tasks gain access to `TaskEnvironment` by implementing the `IMultiThreadableTask` interface. Implementing it does not by itself cause the task to run in-process — `[MSBuildMultiThreadableTask]` is required for that. | ||
|
|
||
| ```csharp | ||
| namespace Microsoft.Build.Framework; | ||
|
|
@@ -43,6 +56,8 @@ public abstract class MultiThreadableTask : Task, IMultiThreadableTask | |
| } | ||
| ``` | ||
|
|
||
| Deriving from `MultiThreadableTask` supplies the interface but not the attribute, so a derived task must still apply `[MSBuildMultiThreadableTask]` to run in-process. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we decided not to provide the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the whole section in 1b3a222 — the prose, the code block, and the sentence I had added to it. Confirmed there is no such type: I kept the following paragraph, since it is about built-in tasks rather than the abstract class and is still accurate:
|
||
|
|
||
| Built-in MSBuild tasks initialize `TaskEnvironment` with a `MultiProcessTaskEnvironmentDriver`-backed default. This ensures tasks have a usable `TaskEnvironment` even when explicitly instantiated outside the engine (e.g., `new Copy()`) or run in the out-of-proc task host. The engine's in-proc path (`TaskExecutionHost.InitializeForBatch`) overwrites the default with the appropriate driver before `Execute()` is called. | ||
|
|
||
| #### Constructor Injection of `TaskEnvironment` | ||
|
|
@@ -87,14 +102,14 @@ Task authors can indicate thread-safety capabilities by marking their task class | |
|
|
||
| ```csharp | ||
| namespace Microsoft.Build.Framework; | ||
| [AttributeUsage(AttributeTargets.Class, AllowMultiple = false)] | ||
| internal class MSBuildMultiThreadableTaskAttribute : Attribute | ||
| [AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = false)] | ||
| public class MSBuildMultiThreadableTaskAttribute : Attribute | ||
| { | ||
| public MSBuildMultiThreadableTaskAttribute() { } | ||
| } | ||
| ``` | ||
|
|
||
| MSBuild detects `MSBuildMultiThreadableTaskAttribute` by its namespace and name only, ignoring the defining assembly, which allows customers to define the attribute in their own assemblies alongside their tasks. Since MSBuild does not ship the attribute, customers using newer MSBuild versions should prefer the Interface-Based Thread-Safe Capability Declaration. | ||
| MSBuild detects `MSBuildMultiThreadableTaskAttribute` by its namespace and name only, ignoring the defining assembly, which allows customers to define the attribute in their own assemblies alongside their tasks. Because the attribute is the routing signal, every task that should run in-process must carry it — including tasks that also implement `IMultiThreadableTask`. The attribute is not inherited (`Inherited = false`, and `TaskRouter` reads it with `inherit: false`), so it must be applied to each concrete task class rather than to a shared base. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Because the attribute is the routing signal, every task that should run in-process must carry it — including tasks that also implement
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed — dropped that sentence in 1b3a222. The paragraph now keeps only the part that is not stated anywhere else, the non-inheritance:
|
||
|
|
||
| For tasks to be eligible for multithreaded execution using this approach, they must satisfy the following conditions: | ||
| - The task must not modify global process state (environment variables, working directory) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,13 +8,20 @@ namespace Microsoft.Build.BackEnd | |
| { | ||
| /// <summary> | ||
| /// 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. | ||
| /// In multi-threaded execution mode, tasks marked with MSBuildMultiThreadableTaskAttribute run | ||
| /// in-process within thread nodes, while all other tasks are routed to sidecar TaskHost processes | ||
| /// for isolation. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This class should only be used when in multi-threaded mode. Traditional multi-proc builds | ||
| /// have different semantics and should not use this routing logic. | ||
| /// <para> | ||
| /// The attribute is the only routing signal. Implementing <see cref="Microsoft.Build.Framework.IMultiThreadableTask"/> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to repeat this point? I think spec and summary should make it clear enough.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would keep it here as well as not everyone reads the spec. The source code is the authority at the end of the day.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taking the verbosity point even while keeping it here — condensed from five lines to four in 1b3a222, dropping the restatement of what the attribute does and keeping only why the interface is deliberately not consulted: /// The attribute is the only routing signal. IMultiThreadableTask is
/// deliberately not consulted here: Microsoft.Build.Utilities.ToolTask implements it, so honoring it
/// would silently opt in every ToolTask-derived task in the ecosystem. The interface instead controls
/// TaskEnvironment injection, which TaskExecutionHost handles separately. |
||
| /// does not opt a task into in-process execution: <c>Microsoft.Build.Utilities.ToolTask</c> implements that | ||
| /// interface, so honoring it here would silently opt in every ToolTask-derived task in the ecosystem, none of which | ||
| /// have been reviewed for thread safety. The interface instead controls whether the engine injects a | ||
| /// TaskEnvironment into the task, which TaskExecutionHost handles separately. | ||
| /// </para> | ||
| /// </remarks> | ||
| internal static class TaskRouter | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not exactly correct, the declaration of mt capabilities is only the attribute. Task then may use the interface. But some very simple tasks may need only the attribute and still be fully and properly migrated. It is explained further and I would like to avoid misunderstanding here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, and I conflated the two. Fixed in 1b3a222 — the section now opens:
I also strengthened the attribute-only bullet, which undersold the same point — it said "a supported state", now: