Migrate CollectSDKReferencesDesignTime to IMultiThreadableTask#53953
Migrate CollectSDKReferencesDesignTime to IMultiThreadableTask#53953SimaTian wants to merge 1 commit intodotnet:mainfrom
Conversation
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>
There was a problem hiding this comment.
Pull request overview
Migrate CollectSDKReferencesDesignTime to participate in MSBuild’s multithreaded task execution model by implementing IMultiThreadableTask (with a TaskEnvironment property) and adding targeted multithreading/behavioral unit tests to validate correctness under concurrent execution.
Changes:
- Update
CollectSDKReferencesDesignTimeto implementIMultiThreadableTaskand exposeTaskEnvironment(with NETFRAMEWORK-specific handling). - Replace metadata string literals with
MetadataKeys.*constants within the task. - Add a dedicated unit test file with concurrency stress coverage and behavioral cases for implicit package detection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Tasks/Microsoft.NET.Build.Tasks/CollectSDKReferencesDesignTime.cs | Implements IMultiThreadableTask and adds TaskEnvironment plumbing for multithreaded MSBuild execution. |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenACollectSDKReferencesDesignTimeMultiThreading.cs | Adds dedicated concurrency + behavior tests validating output correctness across parallel executions and implicit-package rules. |
| #if NETFRAMEWORK | ||
| private TaskEnvironment _taskEnvironment; | ||
| public TaskEnvironment TaskEnvironment | ||
| { | ||
| get => _taskEnvironment ??= new TaskEnvironment(new ProcessTaskEnvironmentDriver(Directory.GetCurrentDirectory())); | ||
| set => _taskEnvironment = value; |
There was a problem hiding this comment.
On NETFRAMEWORK, the lazy default TaskEnvironment creates a ProcessTaskEnvironmentDriver(Directory.GetCurrentDirectory()), which enumerates process environment variables and captures the current working directory. That adds side effects/overhead (and contradicts the PR’s stated “zero env-var reads”) even though this task is otherwise pure in-memory. Consider making TaskEnvironment a simple settable property (letting MSBuild supply it) or using a lightweight in-memory driver that doesn’t snapshot process environment/CWD when a fallback is needed.
| var taskInstances = new CollectSDKReferencesDesignTime[concurrency]; | ||
| var executeTasks = new Task<bool>[concurrency]; | ||
| var startGate = new ManualResetEventSlim(false); | ||
|
|
There was a problem hiding this comment.
ManualResetEventSlim is disposed manually after Task.WhenAll. If the test fails earlier with an exception, the event won’t be disposed. Use a using var startGate = new ManualResetEventSlim(false); (or a try/finally) to guarantee disposal and keep the test from leaking resources when it fails.
| public void TaskProducesSameOutputsWithAndWithoutExplicitTaskEnvironment() | ||
| { | ||
| // Run without explicitly setting TaskEnvironment (uses default) | ||
| var engine1 = new MockBuildEngine(); | ||
| var task1 = CreateTask(1, engine1); | ||
| task1.Execute().Should().BeTrue(); | ||
|
|
||
| // Run with explicitly set TaskEnvironment | ||
| var engine2 = new MockBuildEngine(); | ||
| var task2 = CreateTask(1, engine2); | ||
| task2.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); | ||
| task2.Execute().Should().BeTrue(); |
There was a problem hiding this comment.
This test name/comment implies the task has a meaningful “default” TaskEnvironment, but the task never reads TaskEnvironment at all (and on non-NETFRAMEWORK it stays null unless MSBuild/tests set it). As written, the test only proves outputs don’t depend on TaskEnvironment, not that a default is correctly configured. Consider renaming the test to reflect that, or explicitly set TaskEnvironment in both runs and/or add an assertion that TaskEnvironment isn’t accessed/required for execution.
Migrates
CollectSDKReferencesDesignTimeto supportIMultiThreadableTask.Changes
ITaskItemcollections and parses metadata; zero file I/O, zero env-var reads.MetadataKeys.*constants (IsImplicitlyDefined,SDKPackageItemSpec,Name,Version).ManualResetEventSlim+Task.Run— no Barrier + Parallel.For).Addresses review feedback from #53116
Supersedes
Part of the split of stuck merge-group PR #53116. This is the 1-of-5 split for
CollectSDKReferencesDesignTime.