Multithreading migration: Group 5 — 5 attribute-only tasks by SimaTian · Pull Request #53116 · dotnet/sdk

SimaTian · 2026-02-22T20:00:34Z

Summary

Migrate 5 attribute-only tasks to support multithreaded MSBuild execution by adding the [MSBuildMultiThreadableTask] attribute and comprehensive tests.

Tasks Migrated (Pattern A — attribute-only)

Task	Change
CollectSDKReferencesDesignTime	Attribute already on main; tests added
CreateWindowsSdkKnownFrameworkReferences	Attribute already on main; tests added
FindItemsFromPackages	Attribute already on main; tests added
GetAssemblyVersion	`[MSBuildMultiThreadableTask]` attribute added
GenerateSupportedTargetFrameworkAlias	Attribute already on main; tests added

All 5 tasks perform pure in-memory transformations with no file I/O, no environment variable access, and no Path.GetFullPath() calls. They qualify for Pattern A (attribute-only) — no IMultiThreadableTask interface or TaskEnvironment needed.

Tests Added

GivenAttributeOnlyTasksGroup5.cs (695 lines): 23 [Fact] tests covering attribute presence and behavioral correctness for all 5 tasks, plus concurrent execution stress tests.

Files Changed

src/Tasks/Microsoft.NET.Build.Tasks/GetAssemblyVersion.cs — added [MSBuildMultiThreadableTask]
src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAttributeOnlyTasksGroup5.cs — new test file

Tasks: CollectSDKReferencesDesignTime, CreateWindowsSdkKnownFrameworkReferences, FindItemsFromPackages, GetAssemblyVersion, GenerateSupportedTargetFrameworkAlias. These tasks already have [MSBuildMultiThreadableTask] attribute. This commit adds TDD tests verifying attribute presence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Add [MSBuildMultiThreadableTask] attribute verification tests for: CollectSDKReferencesDesignTime, CreateWindowsSdkKnownFrameworkReferences, FindItemsFromPackages, GetAssemblyVersion, GenerateSupportedTargetFrameworkAlias. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR advances the multithreading migration of MSBuild tasks (Pattern A: attribute-only) by marking GetAssemblyVersion as multithreadable and adding a new unit test suite that validates [MSBuildMultiThreadableTask] presence, behavior, and concurrent execution for 5 tasks.

Changes:

Added [MSBuildMultiThreadableTask] to GetAssemblyVersion.
Added GivenAttributeOnlyTasksGroup5.cs with attribute-presence tests, behavioral correctness tests, and concurrent-execution stress tests for 5 attribute-only tasks.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File	Description
src/Tasks/Microsoft.NET.Build.Tasks/GetAssemblyVersion.cs	Adds `[MSBuildMultiThreadableTask]` to allow multi-threaded MSBuild execution.
src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAttributeOnlyTasksGroup5.cs	Adds tests validating attribute presence, task behavior, and concurrency behavior for the group.

Copilot · 2026-02-22T20:05:52Z

+            var errors = new ConcurrentBag<string>();
+            var barrier = new Barrier(parallelism);
+
+            Parallel.For(0, parallelism, new ParallelOptions { MaxDegreeOfParallelism = parallelism }, i =>
+            {


Same potential deadlock/hang pattern: using a Barrier inside Parallel.For can stall the test if not all iterations start concurrently. Consider using explicit tasks + start gate (or another coordination primitive) and disposing the barrier.

Copilot · 2026-02-22T20:05:52Z

+            var errors = new ConcurrentBag<string>();
+            var barrier = new Barrier(parallelism);
+
+            Parallel.For(0, parallelism, new ParallelOptions { MaxDegreeOfParallelism = parallelism }, i =>
+            {


Same issue here: Barrier(parallelism) inside Parallel.For can deadlock if the loop is executed on fewer worker threads than parallelism (or if an iteration exits before reaching SignalAndWait). Rework the coordination so every participant is guaranteed to reach the gate, and dispose the synchronization primitive.

Copilot · 2026-02-22T20:05:52Z

+            var errors = new ConcurrentBag<string>();
+            var barrier = new Barrier(parallelism);
+
+            Parallel.For(0, parallelism, new ParallelOptions { MaxDegreeOfParallelism = parallelism }, i =>
+            {


Same potential deadlock/hang risk: Barrier + Parallel.For requires all iterations to reach SignalAndWait, but Parallel.For may not run all iterations concurrently. Prefer explicit tasks with a start gate (or similar) and dispose the barrier.

Copilot · 2026-02-22T20:05:52Z

+    /// <summary>
+    /// Behavioral tests for attribute-only tasks in merge-group-5.
+    /// These tasks received only the [MSBuildMultiThreadableTask] attribute — no source
+    /// code changes — so we verify they still produce correct results.
+    /// </summary>


This file adds new behavioral tests for tasks that already have dedicated behavioral test coverage elsewhere (e.g., CollectSDKReferencesDesignTime in GivenUnresolvedSDKProjectItemsAndImplicitPackages.cs, GenerateSupportedTargetFrameworkAlias in GivenThatWeWantToGenerateSupportedTargetFrameworkAlias.cs). To reduce duplication and maintenance cost, consider limiting this group file to attribute-presence + concurrency stress coverage, or reusing/extending the existing task-specific test classes instead of duplicating behavior assertions here.

Copilot · 2026-02-22T20:05:53Z

+            var sdkRef = new TaskItem("Microsoft.NETCore.App");
+            sdkRef.SetMetadata("SDKPackageItemSpec", "");
+
+            var implicitPkg = new MockTaskItem("Microsoft.NETCore.App", new Dictionary<string, string>
+            {
+                { "IsImplicitlyDefined", "true" },
+                { "Version", "8.0.0" }
+            });


These tests set MSBuild metadata keys using string literals (e.g., "SDKPackageItemSpec", "IsImplicitlyDefined", "Version"). The repo already defines these keys in MetadataKeys and other tests use those constants; using the constants here avoids drift/typos if a key name changes.

Copilot · 2026-02-22T20:05:53Z

+                { "NuGetPackageId", "MyPackage" },
+                { "NuGetPackageVersion", "1.0.0" }
+            });
+            var item2 = new MockTaskItem("lib/net8.0/Other.dll", new Dictionary<string, string>
+            {
+                { "NuGetPackageId", "OtherPackage" },
+                { "NuGetPackageVersion", "2.0.0" }
+            });
+
+            var package = new MockTaskItem("MyPackage", new Dictionary<string, string>
+            {
+                { "NuGetPackageId", "MyPackage" },
+                { "NuGetPackageVersion", "1.0.0" }


These tests use literal metadata names ("NuGetPackageId" / "NuGetPackageVersion") instead of the existing MetadataKeys.NuGetPackageId / MetadataKeys.NuGetPackageVersion constants used throughout the tasks/tests. Using the constants reduces duplication and prevents subtle metadata key typos.

Suggested change

{ "NuGetPackageId", "MyPackage" },

{ "NuGetPackageVersion", "1.0.0" }

});

var item2 = new MockTaskItem("lib/net8.0/Other.dll", new Dictionary<string, string>

{

{ "NuGetPackageId", "OtherPackage" },

{ "NuGetPackageVersion", "2.0.0" }

});

var package = new MockTaskItem("MyPackage", new Dictionary<string, string>

{

{ "NuGetPackageId", "MyPackage" },

{ "NuGetPackageVersion", "1.0.0" }

{ global::Microsoft.NET.Build.Tasks.MetadataKeys.NuGetPackageId, "MyPackage" },

{ global::Microsoft.NET.Build.Tasks.MetadataKeys.NuGetPackageVersion, "1.0.0" }

});

var item2 = new MockTaskItem("lib/net8.0/Other.dll", new Dictionary<string, string>

{

{ global::Microsoft.NET.Build.Tasks.MetadataKeys.NuGetPackageId, "OtherPackage" },

{ global::Microsoft.NET.Build.Tasks.MetadataKeys.NuGetPackageVersion, "2.0.0" }

});

var package = new MockTaskItem("MyPackage", new Dictionary<string, string>

{

{ global::Microsoft.NET.Build.Tasks.MetadataKeys.NuGetPackageId, "MyPackage" },

{ global::Microsoft.NET.Build.Tasks.MetadataKeys.NuGetPackageVersion, "1.0.0" }

Copilot · 2026-02-22T20:05:53Z

+            var barrier = new Barrier(parallelism);
+
+            Parallel.For(0, parallelism, new ParallelOptions { MaxDegreeOfParallelism = parallelism }, i =>
+            {
+                try
+                {
+                    var sdkRef = new TaskItem("Microsoft.NETCore.App");
+                    sdkRef.SetMetadata("SDKPackageItemSpec", "");
+
+                    var implicitPkg = new MockTaskItem("Microsoft.NETCore.App", new Dictionary<string, string>
+                    {
+                        { "IsImplicitlyDefined", "true" },
+                        { "Version", "8.0.0" }
+                    });
+
+                    var task = new CollectSDKReferencesDesignTime
+                    {
+                        BuildEngine = new MockBuildEngine(),
+                        SdkReferences = new ITaskItem[] { sdkRef },
+                        PackageReferences = new ITaskItem[] { implicitPkg },
+                        DefaultImplicitPackages = "Microsoft.NETCore.App"
+                    };
+
+                    barrier.SignalAndWait();
+                    task.Execute();
+
+                    if (task.SDKReferencesDesignTime == null || task.SDKReferencesDesignTime.Length != 2)
+                    {
+                        errors.Add($"Thread {i}: Expected 2 items but got {task.SDKReferencesDesignTime?.Length}");
+                    }
+                }
+                catch (Exception ex)
+                {
+                    errors.Add($"Thread {i}: {ex.Message}");
+                }
+            });
+


The Barrier participant count is set to parallelism, but it’s used inside a Parallel.For loop. Parallel.For doesn’t guarantee all iterations run concurrently (thread-pool starvation / fewer worker threads than iterations), so this can deadlock/hang the test waiting for participants that never reach SignalAndWait. Consider switching to explicitly created Task instances (so you have exactly parallelism participants) or using a start-gate (e.g., ManualResetEventSlim) instead of a Barrier inside Parallel.For, and dispose the synchronization primitive.

Suggested change

var barrier = new Barrier(parallelism);

Parallel.For(0, parallelism, new ParallelOptions { MaxDegreeOfParallelism = parallelism }, i =>

{

try

{

var sdkRef = new TaskItem("Microsoft.NETCore.App");

sdkRef.SetMetadata("SDKPackageItemSpec", "");

var implicitPkg = new MockTaskItem("Microsoft.NETCore.App", new Dictionary<string, string>

{

{ "IsImplicitlyDefined", "true" },

{ "Version", "8.0.0" }

});

var task = new CollectSDKReferencesDesignTime

{

BuildEngine = new MockBuildEngine(),

SdkReferences = new ITaskItem[] { sdkRef },

PackageReferences = new ITaskItem[] { implicitPkg },

DefaultImplicitPackages = "Microsoft.NETCore.App"

};

barrier.SignalAndWait();

task.Execute();

if (task.SDKReferencesDesignTime == null || task.SDKReferencesDesignTime.Length != 2)

{

errors.Add($"Thread {i}: Expected 2 items but got {task.SDKReferencesDesignTime?.Length}");

}

}

catch (Exception ex)

{

errors.Add($"Thread {i}: {ex.Message}");

}

});

using (var startGate = new ManualResetEventSlim(false))

{

var tasks = new Task[parallelism];

for (int i = 0; i < parallelism; i++)

{

int localIndex = i;

tasks[localIndex] = Task.Run(() =>

{

startGate.Wait();

try

{

var sdkRef = new TaskItem("Microsoft.NETCore.App");

sdkRef.SetMetadata("SDKPackageItemSpec", "");

var implicitPkg = new MockTaskItem("Microsoft.NETCore.App", new Dictionary<string, string>

{

{ "IsImplicitlyDefined", "true" },

{ "Version", "8.0.0" }

});

var task = new CollectSDKReferencesDesignTime

{

BuildEngine = new MockBuildEngine(),

SdkReferences = new ITaskItem[] { sdkRef },

PackageReferences = new ITaskItem[] { implicitPkg },

DefaultImplicitPackages = "Microsoft.NETCore.App"

};

task.Execute();

if (task.SDKReferencesDesignTime == null || task.SDKReferencesDesignTime.Length != 2)

{

errors.Add($"Thread {localIndex}: Expected 2 items but got {task.SDKReferencesDesignTime?.Length}");

}

}

catch (Exception ex)

{

errors.Add($"Thread {localIndex}: {ex.Message}");

}

});

}

startGate.Set();

Task.WaitAll(tasks);

}

Copilot · 2026-02-22T20:05:53Z

+            var errors = new ConcurrentBag<string>();
+            var barrier = new Barrier(parallelism);
+
+            Parallel.For(0, parallelism, new ParallelOptions { MaxDegreeOfParallelism = parallelism }, i =>
+            {


Same potential deadlock/hang pattern as above: Barrier(parallelism) combined with Parallel.For(0, parallelism, ...) can block forever if the thread pool doesn’t schedule all iterations concurrently. Prefer a start-gate with explicitly created tasks (or ensure SignalAndWait is reached unconditionally) and dispose the barrier.

- Replace string literals with MetadataKeys.* constants - Replace Barrier+Parallel.For with ManualResetEventSlim+Task.Run - Use async test methods with Task.WhenAll - Fix loop variable capture (i -> idx) in Task.Run lambdas Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The combining constructor AbsolutePath(string, AbsolutePath) used Path.Combine without Path.GetFullPath, leaving '..' segments unresolved. This caused output paths like 'dir\..\ClassLib\...' instead of 'ClassLib\...', breaking string-based path comparisons in downstream MSBuild targets and tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

JanProvaznik · 2026-02-24T15:28:36Z

            }

-            Value = Path.Combine(basePath.Value, path);
+            Value = Path.GetFullPath(Path.Combine(basePath.Value, path));


no, when you need the canonicalization you have to do this in caller

On .NET Framework, ProcessStartInfo defaults to UseShellExecute=true, which prevents EnvironmentVariables from being applied. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Normalization is caller's responsibility, matching real MSBuild polyfill. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Per skill guidelines: attribute/interface tests are redundant noise. Removed 5 HasMultiThreadableAttribute tests. All 28 behavioral + concurrent tests retained. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

This task was previously marked with [MSBuildMultiThreadableTask] but did not implement IMultiThreadableTask. After scanning for forbidden APIs (Environment.*, Path.*, File.*, Directory.*), verified that the task is genuinely pure in-memory with no I/O operations or environment dependencies. The task only: - Parses semicolon-delimited strings into HashSets - Iterates over ITaskItem collections - Creates new TaskItem instances with metadata - Performs case-insensitive string matching Applied attribute-only migration with IMultiThreadableTask interface stub. The TaskEnvironment property is added for API consistency (even though unused) per guidance that there should not be any attribute-only tasks left. Tests verify: - Concurrent execution correctness with explicit Task.Run + start-gate pattern - Output equivalence with/without explicit TaskEnvironment - Implicit package identification via both metadata and DefaultImplicitPackages - Case-insensitive package name matching - Metadata precedence over DefaultImplicitPackages list - Empty input handling All metadata keys use MetadataKeys.* constants (no string literals). Addresses prior review findings from PR dotnet#53116: 1. Uses explicit Task.Run + ManualResetEventSlim instead of Barrier + Parallel.For 2. Tests in dedicated GivenACollectSDKReferencesDesignTimeMultiThreading.cs 3. All metadata keys use MetadataKeys.* constants Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

SimaTian · 2026-04-17T09:56:55Z

Closing this PR — the 5 attribute-only tasks have been split into individual PRs for independent review and merge, with each re-verified as pure in-memory before submission:

CollectSDKReferencesDesignTime → Migrate CollectSDKReferencesDesignTime to IMultiThreadableTask #53953
CreateWindowsSdkKnownFrameworkReferences → Migrate CreateWindowsSdkKnownFrameworkReferences to IMultiThreadableTask #53951
FindItemsFromPackages → Migrate FindItemsFromPackages to IMultiThreadableTask #53952
GetAssemblyVersion → Migrate GetAssemblyVersion to IMultiThreadableTask #53948
GenerateSupportedTargetFrameworkAlias → Migrate GenerateSupportedTargetFrameworkAlias to IMultiThreadableTask #53954

Each split addresses the review feedback from this merge group: start-gate pattern (ManualResetEventSlim + Task.Run) instead of Barrier + Parallel.For, dedicated per-task test files (not a shared suite), and MetadataKeys.* constants instead of string literals.

This task was previously marked with [MSBuildMultiThreadableTask] but did not implement IMultiThreadableTask. After scanning for forbidden APIs (Environment.*, Path.*, File.*, Directory.*), verified that the task is genuinely pure in-memory with no I/O operations or environment dependencies. The task only: - Parses semicolon-delimited strings into HashSets - Iterates over ITaskItem collections - Creates new TaskItem instances with metadata - Performs case-insensitive string matching Applied attribute-only migration with IMultiThreadableTask interface stub. The TaskEnvironment property is added for API consistency (even though unused) per guidance that there should not be any attribute-only tasks left. Tests verify: - Concurrent execution correctness with explicit Task.Run + start-gate pattern - Output equivalence with/without explicit TaskEnvironment - Implicit package identification via both metadata and DefaultImplicitPackages - Case-insensitive package name matching - Metadata precedence over DefaultImplicitPackages list - Empty input handling All metadata keys use MetadataKeys.* constants (no string literals). Addresses prior review findings from PR dotnet#53116: 1. Uses explicit Task.Run + ManualResetEventSlim instead of Barrier + Parallel.For 2. Tests in dedicated GivenACollectSDKReferencesDesignTimeMultiThreading.cs 3. All metadata keys use MetadataKeys.* constants Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

SimaTian and others added 3 commits February 22, 2026 16:20

Add concurrent execution tests for Group 5 tasks

493216e

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings February 22, 2026 20:00

Copilot started reviewing on behalf of SimaTian February 22, 2026 20:01 View session

Copilot AI reviewed Feb 22, 2026

View reviewed changes

build-analysis Bot mentioned this pull request Feb 22, 2026

dotnet-watch tests failing #40006

Open

SimaTian self-assigned this Feb 23, 2026

SimaTian requested a review from JanProvaznik February 23, 2026 14:12

JanProvaznik reviewed Feb 24, 2026

View reviewed changes

SimaTian marked this pull request as draft February 24, 2026 17:54

SimaTian and others added 3 commits February 25, 2026 13:20

Add UseShellExecute=false in ProcessTaskEnvironmentDriver

ca8bc20

On .NET Framework, ProcessStartInfo defaults to UseShellExecute=true, which prevents EnvironmentVariables from being applied. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Revert AbsolutePath: remove Path.GetFullPath wrapping

385b4f1

Normalization is caller's responsibility, matching real MSBuild polyfill. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Remove attribute-presence tests from Group 5

794553a

Per skill guidelines: attribute/interface tests are redundant noise. Removed 5 HasMultiThreadableAttribute tests. All 28 behavioral + concurrent tests retained. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

SimaTian force-pushed the merge-group-5 branch from 42545ed to 794553a Compare March 26, 2026 15:59

SimaTian closed this Apr 17, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multithreading migration: Group 5 — 5 attribute-only tasks#53116

Multithreading migration: Group 5 — 5 attribute-only tasks#53116
SimaTian wants to merge 8 commits intodotnet:mainfrom
SimaTian:merge-group-5

SimaTian commented Feb 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

JanProvaznik Feb 24, 2026

Uh oh!

SimaTian commented Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

SimaTian commented Feb 22, 2026

Summary

Tasks Migrated (Pattern A — attribute-only)

Tests Added

Files Changed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

JanProvaznik Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

SimaTian commented Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants