Multithreading migration: Group 8 — 3 COM & Shims tasks (Pattern B)#53119
Multithreading migration: Group 8 — 3 COM & Shims tasks (Pattern B)#53119SimaTian wants to merge 12 commits intodotnet:mainfrom
Conversation
Tasks: CreateComHost, GenerateRegFreeComManifest, GenerateShims. Each task receives [MSBuildMultiThreadableTask] attribute, IMultiThreadableTask interface, TaskEnvironment property, and path absolutization. Includes multithreading tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace reflection-based TaskEnvironment assignment with direct property access in CreateComHost and GenerateRegFreeComManifest path-resolution tests - Add Paths_AreResolvedRelativeToProjectDirectory test for GenerateShims, matching the pattern from CreateComHost and GenerateRegFreeComManifest Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Change from 'private set' to 'set' to match the [Required] MSBuild property contract and allow test assignment. Consistent with all neighboring task input properties (ComHostSourcePath, IntermediateAssembly, etc.). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Migrates three COM/shim-related MSBuild tasks to Pattern B multithreaded execution by implementing IMultiThreadableTask and resolving file-system paths via TaskEnvironment instead of relying on process current directory, with new unit tests intended to validate CWD-independence and concurrency behavior.
Changes:
- Add
IMultiThreadableTask+TaskEnvironmenttoCreateComHost,GenerateRegFreeComManifest, andGenerateShims. - Absolutize key input paths (and some type library metadata paths) before invoking external libraries that perform file I/O.
- Add multithreading-focused unit tests for path resolution parity and concurrent execution.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tasks/Microsoft.NET.Build.Tasks/CreateComHost.cs | Implements multithreaded task support and absolutizes COM host paths. |
| src/Tasks/Microsoft.NET.Build.Tasks/GenerateRegFreeComManifest.cs | Implements multithreaded task support and absolutizes manifest-related paths. |
| src/Tasks/Microsoft.NET.Build.Tasks/GenerateShims.cs | Implements multithreaded task support and absolutizes apphost/output/assembly paths. |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenACreateComHostMultiThreading.cs | Adds tests for CWD independence and concurrency (currently has cross-platform + deadlock risks). |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateRegFreeComManifestMultiThreading.cs | Adds tests for path resolution parity and concurrency (currently has cross-platform + deadlock risks). |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateShimsMultiThreading.cs | Adds tests for shims path resolution and concurrency (currently has cross-platform + deadlock risks). |
| var apphostItem = new TaskItem("tools\\apphost.exe"); | ||
| apphostItem.SetMetadata(MetadataKeys.RuntimeIdentifier, "linux-x64"); | ||
|
|
||
| var task = new GenerateShims | ||
| { | ||
| BuildEngine = new MockBuildEngine(), | ||
| ApphostsForShimRuntimeIdentifiers = new ITaskItem[] { apphostItem }, | ||
| IntermediateAssembly = "bin\\test.dll", | ||
| PackageId = "TestPackage", | ||
| PackageVersion = "1.0.0", | ||
| TargetFrameworkMoniker = ".NETCoreApp,Version=v8.0", | ||
| ToolCommandName = "test-tool", | ||
| ToolEntryPoint = "test-tool.dll", | ||
| PackagedShimOutputDirectory = "shims", | ||
| ShimRuntimeIdentifiers = new ITaskItem[] { new TaskItem("linux-x64") }, | ||
| TaskEnvironment = TaskEnvironmentHelper.CreateForTest(projectDir), |
There was a problem hiding this comment.
This test hard-codes Windows path separators in TaskItem ItemSpecs (e.g., tools\\apphost.exe, bin\\test.dll). On Unix these become different file names than the ones created with Path.Combine, so the test can fail. Construct these ItemSpecs with Path.Combine (or use /) to keep the test cross-platform.
| TaskEnvironment = TaskEnvironmentHelper.CreateForTest(projectDir), | ||
| }; | ||
|
|
||
| barrier.SignalAndWait(); |
There was a problem hiding this comment.
This concurrency test uses a Barrier without a timeout. If a worker throws before reaching SignalAndWait, the remaining workers can deadlock and hang CI. Add a timeout to SignalAndWait (and record a failure) or avoid barrier-based coordination.
| barrier.SignalAndWait(); | |
| var signaled = barrier.SignalAndWait(System.TimeSpan.FromSeconds(30)); | |
| if (!signaled) | |
| { | |
| results.Add((false, "BarrierTimeout")); | |
| return; | |
| } |
| // Absolutize type library paths | ||
| if (typeLibIdMap != null) | ||
| { | ||
| foreach (var key in typeLibIdMap.Keys.ToList()) | ||
| { | ||
| typeLibIdMap[key] = TaskEnvironment.GetAbsolutePath(typeLibIdMap[key]); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new loop absolutizes typeLibIdMap values, but typeLibIdMap is not used anywhere after TryCreateTypeLibraryIdDictionary succeeds. This makes the change effectively a no-op and adds confusing/unused work; either remove this absolutization (and potentially the dictionary creation if it’s only for validation), or wire the absolutized paths into the manifest generation logic if that was intended.
| var resolvedApphostAssetPath = (string)TaskEnvironment.GetAbsolutePath(GetApphostAsset(ApphostsForShimRuntimeIdentifiers, runtimeIdentifier)); | ||
|
|
||
| var packagedShimOutputDirectoryAndRid = Path.Combine( | ||
| PackagedShimOutputDirectory, | ||
| (string)TaskEnvironment.GetAbsolutePath(PackagedShimOutputDirectory), | ||
| runtimeIdentifier); |
There was a problem hiding this comment.
PackagedShimOutputDirectory is now absolutized before computing appHostDestinationFilePath, which means EmbeddedApphostPaths item specs become absolute when the input directory is relative. This may break consistency with GetEmbeddedApphostPaths (which still returns paths based on the raw PackagedShimOutputDirectory) and can cause incremental-build/publish item mismatches; consider keeping output item specs in the same form as before (while still using absolute paths for file I/O) or updating the expected-path computation accordingly.
| ComHostSourcePath = "source\\comhost.dll", | ||
| ComHostDestinationPath = "output\\comhost.dll", | ||
| ClsidMapPath = "source\\clsidmap.bin", |
There was a problem hiding this comment.
This test hard-codes Windows path separators (e.g., source\\comhost.dll). On non-Windows agents this will be treated as a literal backslash in the filename, so the created files won’t be found and the test can fail/flap. Prefer Path.Combine(...) (or Path.Join) for ItemSpec strings in tests to keep them cross-platform.
| ComHostSourcePath = "source\\comhost.dll", | |
| ComHostDestinationPath = "output\\comhost.dll", | |
| ClsidMapPath = "source\\clsidmap.bin", | |
| ComHostSourcePath = Path.Combine("source", "comhost.dll"), | |
| ComHostDestinationPath = Path.Combine("output", "comhost.dll"), | |
| ClsidMapPath = Path.Combine("source", "clsidmap.bin"), |
| var barrier = new Barrier(parallelism); | ||
|
|
||
| Parallel.For(0, parallelism, new ParallelOptions { MaxDegreeOfParallelism = parallelism }, i => | ||
| { | ||
| var projectDir = Path.Combine(Path.GetTempPath(), $"comhost-conc-{i}-{Guid.NewGuid():N}"); | ||
| Directory.CreateDirectory(projectDir); | ||
| try | ||
| { | ||
| var sourceDir = Path.Combine(projectDir, "source"); | ||
| var outputDir = Path.Combine(projectDir, "output"); | ||
| Directory.CreateDirectory(sourceDir); | ||
| Directory.CreateDirectory(outputDir); | ||
| File.WriteAllText(Path.Combine(sourceDir, "comhost.dll"), "fake-comhost-pe"); | ||
| File.WriteAllText(Path.Combine(sourceDir, "clsidmap.bin"), "fake-clsid"); | ||
|
|
||
| var task = new CreateComHost | ||
| { | ||
| BuildEngine = new MockBuildEngine(), | ||
| ComHostSourcePath = Path.Combine("source", "comhost.dll"), | ||
| ComHostDestinationPath = Path.Combine("output", "comhost.dll"), | ||
| ClsidMapPath = Path.Combine("source", "clsidmap.bin"), | ||
| TaskEnvironment = TaskEnvironmentHelper.CreateForTest(projectDir), | ||
| }; | ||
|
|
||
| barrier.SignalAndWait(); | ||
| var result = task.Execute(); | ||
| results.Add((result, "none")); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| results.Add((false, ex.GetType().Name)); | ||
| } |
There was a problem hiding this comment.
Barrier.SignalAndWait() is used without a timeout; if any worker throws before reaching the barrier (e.g., due to I/O issues), the remaining workers can deadlock and hang the test run. Add a timeout (and fail when it expires) or switch to a non-blocking coordination primitive that can’t deadlock on early failure.
| IntermediateAssembly = $"bin\\{assemblyFileName}", | ||
| ComHostName = "test.comhost.dll", | ||
| ClsidMapPath = "bin\\clsidmap.bin", | ||
| ComManifestPath = "bin\\test.manifest", |
There was a problem hiding this comment.
This test uses bin\\... / clsidmap paths with Windows separators. On non-Windows this can resolve to a non-existent file because backslash is a valid filename character. Use Path.Combine("bin", ...) (or forward slashes) when constructing relative paths for cross-platform tests.
| IntermediateAssembly = $"bin\\{assemblyFileName}", | |
| ComHostName = "test.comhost.dll", | |
| ClsidMapPath = "bin\\clsidmap.bin", | |
| ComManifestPath = "bin\\test.manifest", | |
| IntermediateAssembly = $"bin/{assemblyFileName}", | |
| ComHostName = "test.comhost.dll", | |
| ClsidMapPath = "bin/clsidmap.bin", | |
| ComManifestPath = "bin/test.manifest", |
| TaskEnvironment = TaskEnvironmentHelper.CreateForTest(projectDir), | ||
| }; | ||
|
|
||
| barrier.SignalAndWait(); |
There was a problem hiding this comment.
Barrier.SignalAndWait() is called without a timeout in this concurrency test. If any iteration fails before signaling the barrier, other iterations can block indefinitely and hang the test suite. Add a timeout and fail fast, or replace the barrier-based coordination with a safer approach.
| barrier.SignalAndWait(); | |
| if (!barrier.SignalAndWait(System.TimeSpan.FromSeconds(30))) | |
| { | |
| results.Add((false, "BarrierTimeout")); | |
| return; | |
| } |
…code - Replace Barrier+Parallel.For with ManualResetEventSlim+Task.Run - Replace Windows path separators with Path.Combine - Remove dead typeLibIdMap absolutization in GenerateRegFreeComManifest - Remove unnecessary (string) cast on GetAbsolutePath Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add TaskEnvironment to EmptyShimRuntimeIdentifiers test - Use non-empty PackagedShimOutputDirectory (GetAbsolutePath throws on empty) - Catch FileNotFoundException for HostModel (ExcludeAssets=Runtime in .csproj) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… for merge-group-8 - Add TaskEnvironmentDefaults.cs for NETFRAMEWORK lazy-init fallback - Apply lazy-init pattern to GenerateShims, CreateComHost, GenerateRegFreeComManifest - Add empty ShimRuntimeIdentifiers early-return guard in GenerateShims 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>
|
Temporarily closing due to review-queue bottleneck — one big grouped PR has turned out harder to review than 5 individual task PRs (see #52936 thread). Strategy update: tasks in this PR have been recorded in our internal postponed-tasks backlog and will be re-opened as individual per-task PRs once the current merge-group-2 and merge-group-5 splits are cleared. Note that some tasks in this group (ValidateExecutableReferences, ProcessFrameworkReferences, ResolveTargetingPackAssets) have already been delivered via individual PRs based on OrchardCore profiling priority — see #53942, #53943, #53944, #53945, #53946. |
Summary
Migrate 3 COM and shim-related tasks to support multithreaded MSBuild execution. All 3 tasks perform file I/O and require full Pattern B migration (
IMultiThreadableTask+TaskEnvironment).Tasks Migrated
ComHostSourcePath,ComHostDestinationPath,ClsidMapPath, type library map valuesIntermediateAssembly,ClsidMapPath,ComManifestPath, type library map valuesPackagedShimOutputDirectory,IntermediateAssembly; fixedApphostsForShimRuntimeIdentifierssetter accessibilityMigration Details
All three tasks delegate to external library methods (
ComHost.Create(),RegFreeComManifest.CreateManifestFromClsidmap(),HostWriter.CreateAppHost()) that perform file I/O using the paths provided. All paths are absolutized viaTaskEnvironment.GetAbsolutePath()before being passed to these methods.Type library dictionary values (
TypeLibrariesmetadata) are also absolutized in a loop before use — these contain file paths that flow into COM registration file operations.Tests Added
Files Changed
src/Tasks/Microsoft.NET.Build.Tasks/CreateComHost.cssrc/Tasks/Microsoft.NET.Build.Tasks/GenerateRegFreeComManifest.cssrc/Tasks/Microsoft.NET.Build.Tasks/GenerateShims.cssrc/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenACreateComHostMultiThreading.cs(new)src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateRegFreeComManifestMultiThreading.cs(new)src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateShimsMultiThreading.cs(new)