Multithreading migration: Group 7 — 4 attribute-only + 2 Pattern B tasks#53118
Multithreading migration: Group 7 — 4 attribute-only + 2 Pattern B tasks#53118SimaTian wants to merge 13 commits intodotnet:mainfrom
Conversation
Tasks: SelectRuntimeIdentifierSpecificItems, SetGeneratedAppConfigMetadata, ValidateExecutableReferences, FilterResolvedFiles, RemoveDuplicatePackageReferences. FilterResolvedFiles includes absolutization of AssetsFilePath. Others are attribute-only with test additions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… (Group 7) Add [MSBuildMultiThreadableTask] attribute verification for 4 attribute-only tasks plus FilterResolvedFiles. Replace reflection-based TaskEnvironment assignment with direct property access. Add parity test for FilterResolvedFiles verifying CWD-independent behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…file The test was misplaced (FilterResolvedFiles is Pattern B, not attribute-only) and had a latent NullReferenceException bug due to missing TaskEnvironment assignment. The correct version exists in GivenAFilterResolvedFilesMultiThreading.cs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Task calls RuntimeGraphCache.GetRuntimeGraph(RuntimeIdentifierGraphPath) which does file I/O (JsonRuntimeFormat.ReadRuntimeGraph). The path must be absolutized via TaskEnvironment for multithreading safety. Changes: - Add IMultiThreadableTask interface and TaskEnvironment property - Absolutize RuntimeIdentifierGraphPath before passing to RuntimeGraphCache - Update tests to provide TaskEnvironment via TaskEnvironmentHelper.CreateForTest Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Migrates a set of Microsoft.NET.Build.Tasks MSBuild tasks to support multithreaded execution by adding the multithreadable task attribute and, for Pattern B tasks, implementing IMultiThreadableTask and resolving file paths via TaskEnvironment.
Changes:
- Added
[MSBuildMultiThreadableTask]to attribute-only tasks (SetGeneratedAppConfigMetadata,ValidateExecutableReferences). - Updated Pattern B tasks (
SelectRuntimeIdentifierSpecificItems,FilterResolvedFiles) to implementIMultiThreadableTaskand resolve input file paths usingTaskEnvironment.GetAbsolutePath. - Added new unit tests validating attribute presence, CWD-independence/parity, and concurrent execution behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tasks/Microsoft.NET.Build.Tasks/ValidateExecutableReferences.cs | Marks the task as multithreadable (attribute-only). |
| src/Tasks/Microsoft.NET.Build.Tasks/SetGeneratedAppConfigMetadata.cs | Marks the task as multithreadable (attribute-only). |
| src/Tasks/Microsoft.NET.Build.Tasks/SelectRuntimeIdentifierSpecificItems.cs | Implements IMultiThreadableTask and resolves RuntimeIdentifierGraphPath via TaskEnvironment. |
| src/Tasks/Microsoft.NET.Build.Tasks/FilterResolvedFiles.cs | Implements IMultiThreadableTask and resolves AssetsFilePath via TaskEnvironment. |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAttributeOnlyTasksGroup7.cs | Adds attribute presence, behavioral/parity, and concurrency tests for the migrated tasks. |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAFilterResolvedFilesMultiThreading.cs | Adds tests for FilterResolvedFiles attribute presence, path resolution, parity, and concurrency. |
| { | ||
| BuildEngine = new MockBuildEngine(), | ||
| AssetsFilePath = "obj\\project.assets.json", | ||
| ResolvedFiles = Array.Empty<ITaskItem>(), | ||
| PackagesToPrune = Array.Empty<ITaskItem>(), | ||
| TargetFramework = ".NETCoreApp,Version=v8.0", | ||
| TaskEnvironment = TaskEnvironmentHelper.CreateForTest(projectDir), | ||
| }; |
There was a problem hiding this comment.
AssetsFilePath is set to a Windows-specific relative string ("obj\project.assets.json"). On non-Windows this becomes a filename containing a backslash, so TaskEnvironment.GetAbsolutePath will resolve to a path that doesn’t exist and the test will fail. Use Path.Combine("obj", "project.assets.json") (or "obj/project.assets.json") so the test is OS-agnostic.
| TaskEnvironment = TaskEnvironmentHelper.CreateForTest(projectDir), | ||
| }; | ||
| barrier.SignalAndWait(); | ||
| task.Execute(); |
There was a problem hiding this comment.
The concurrent execution test ignores the return value from task.Execute(). If the task starts logging errors (and returns false) without throwing, this test would still pass. Capture/assert the result (and optionally assert no logged errors) so the test actually validates successful concurrent execution.
| task.Execute(); | |
| var result = task.Execute(); | |
| if (!result) | |
| { | |
| errors.Add($"Thread {i}: task.Execute returned false"); | |
| } |
| TargetRuntimeIdentifier = "linux-x64", | ||
| Items = new ITaskItem[] | ||
| { | ||
| CreateItemWithRid($"Item{i}", "linux-x64") | ||
| }, | ||
| RuntimeIdentifierGraphPath = "" | ||
| }; | ||
| barrier.SignalAndWait(); | ||
| task.Execute(); | ||
| } |
There was a problem hiding this comment.
RuntimeIdentifierGraphPath is set to an empty string in this test. TaskEnvironmentHelper ultimately constructs an AbsolutePath and will throw for null/empty paths, so this concurrent execution test will consistently fail. Create/write a real runtime graph file (like the earlier parity test) and pass its path here.
| public void SetGeneratedAppConfigMetadata_ConcurrentExecution(int parallelism) | ||
| { | ||
| var errors = new ConcurrentBag<string>(); | ||
| var barrier = new Barrier(parallelism); | ||
| Parallel.For(0, parallelism, new ParallelOptions { MaxDegreeOfParallelism = parallelism }, i => | ||
| { | ||
| try | ||
| { | ||
| var task = new SetGeneratedAppConfigMetadata | ||
| { | ||
| BuildEngine = new MockBuildEngine(), | ||
| GeneratedAppConfigFile = $"obj/app{i}.exe.config", | ||
| TargetName = $"app{i}.exe.config" | ||
| }; | ||
| barrier.SignalAndWait(); | ||
| task.Execute(); | ||
| } | ||
| catch (Exception ex) { errors.Add($"Thread {i}: {ex.Message}"); } | ||
| }); | ||
| errors.Should().BeEmpty(); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(4)] | ||
| [InlineData(16)] | ||
| public void ValidateExecutableReferences_ConcurrentExecution(int parallelism) | ||
| { | ||
| var errors = new ConcurrentBag<string>(); | ||
| var barrier = new Barrier(parallelism); | ||
| Parallel.For(0, parallelism, new ParallelOptions { MaxDegreeOfParallelism = parallelism }, i => | ||
| { | ||
| try | ||
| { | ||
| var task = new ValidateExecutableReferences | ||
| { | ||
| BuildEngine = new MockBuildEngine(), | ||
| IsExecutable = false, | ||
| SelfContained = false, | ||
| ReferencedProjects = Array.Empty<ITaskItem>() | ||
| }; | ||
| barrier.SignalAndWait(); | ||
| task.Execute(); | ||
| } | ||
| catch (Exception ex) { errors.Add($"Thread {i}: {ex.Message}"); } | ||
| }); | ||
| errors.Should().BeEmpty(); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(4)] | ||
| [InlineData(16)] | ||
| public void RemoveDuplicatePackageReferences_ConcurrentExecution(int parallelism) | ||
| { | ||
| var errors = new ConcurrentBag<string>(); | ||
| var barrier = new Barrier(parallelism); | ||
| Parallel.For(0, parallelism, new ParallelOptions { MaxDegreeOfParallelism = parallelism }, i => | ||
| { | ||
| try | ||
| { | ||
| var task = new RemoveDuplicatePackageReferences | ||
| { | ||
| BuildEngine = new MockBuildEngine(), | ||
| InputPackageReferences = new ITaskItem[] | ||
| { | ||
| new MockTaskItem($"Package{i}", new Dictionary<string, string> { { "Version", "1.0.0" } }) | ||
| } | ||
| }; | ||
| barrier.SignalAndWait(); | ||
| task.Execute(); | ||
| } | ||
| catch (Exception ex) { errors.Add($"Thread {i}: {ex.Message}"); } | ||
| }); | ||
| errors.Should().BeEmpty(); | ||
| } |
There was a problem hiding this comment.
These concurrent execution tests call task.Execute() but don’t assert the returned value. If a task logs errors and returns false (without throwing), the test would still pass and mask failures. Assert Execute() returns true (and/or validate the mock build engine has no errors) inside the loop.
| protected override void ExecuteCore() | ||
| { | ||
| var lockFileCache = new LockFileCache(this); | ||
| LockFile lockFile = lockFileCache.GetLockFile(AssetsFilePath); | ||
| LockFile lockFile = lockFileCache.GetLockFile(TaskEnvironment.GetAbsolutePath(AssetsFilePath)); | ||
|
|
There was a problem hiding this comment.
ExecuteCore unconditionally dereferences TaskEnvironment and calls GetAbsolutePath. If TaskEnvironment isn’t injected (e.g., task instantiated outside MSBuild, or an older host that doesn’t set it), this becomes a NullReferenceException that rethrows from TaskBase.Execute() rather than a logged build error. Consider guarding against null (and falling back to the original AssetsFilePath / logging a clear error) to avoid hard crashes.
… fix - Replace Barrier+Parallel.For with ManualResetEventSlim+Task.Run - Use async test methods with Task.WhenAll - Replace Windows path separator with Path.Combine - Fix empty RuntimeIdentifierGraphPath in concurrent test - Assert Execute() return value in concurrent tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add TaskEnvironmentDefaults.cs for NETFRAMEWORK lazy-init fallback - Apply lazy-init pattern to FilterResolvedFiles 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>
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>
Use assembly-relative path instead of bare filename to avoid FileNotFound when parallel tests change the process CWD via TaskTestEnvironment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…y-init and set TaskEnvironment in tests - Replace '= null!' with #if NETFRAMEWORK lazy-init pattern for TaskEnvironment property, matching all other Pattern B tasks - Set TaskEnvironment in all 5 test instantiations to prevent NRE on .NET Core where there is no lazy-init fallback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Migrate 5 tasks to support multithreaded MSBuild execution. Three tasks are attribute-only (Pattern A), two require interface-based migration (Pattern B).
Tasks Migrated
IMultiThreadableTask,TaskEnvironment; absolutizedRuntimeIdentifierGraphPathIMultiThreadableTask,TaskEnvironment; absolutizedAssetsFilePathPattern B Details
SelectRuntimeIdentifierSpecificItems: Calls
RuntimeGraphCache.GetRuntimeGraph(RuntimeIdentifierGraphPath)which enforcesPath.IsPathRooted()and performs file I/O viaJsonRuntimeFormat.ReadRuntimeGraph().RuntimeIdentifierGraphPathis absolutized viaTaskEnvironment.GetAbsolutePath()before the call.FilterResolvedFiles: Calls
LockFileCache.GetLockFile(AssetsFilePath)which requires rooted paths.AssetsFilePathis absolutized before the call.Tests Added
Files Changed
src/Tasks/Microsoft.NET.Build.Tasks/SelectRuntimeIdentifierSpecificItems.cssrc/Tasks/Microsoft.NET.Build.Tasks/SetGeneratedAppConfigMetadata.cssrc/Tasks/Microsoft.NET.Build.Tasks/ValidateExecutableReferences.cssrc/Tasks/Microsoft.NET.Build.Tasks/FilterResolvedFiles.cssrc/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAttributeOnlyTasksGroup7.cs(new)src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAFilterResolvedFilesMultiThreading.cs(new)