multithreaded-task-migration

# Migrating MSBuild Tasks to Multithreaded API MSBuild's multithreaded execution model requires tasks to avoid global process state (working directory, environment variables). Thread-safe tasks declare this capability via `MSBuildMultiThreadableTask` and use `TaskEnvironment` from `IMultiThreadableTask` for safe alternatives. ## Migration Steps ### Step 1: Update Task Class Declaration a. Ensure the task implementing class is decorated with the `MSBuildMultiThreadableTask` attribute. b. Implement `IMultiThreadableTask` **only if** the task needs `TaskEnvironment` APIs (path absolutization, env vars, process start). If the task has no file/environment operations (e.g., a stub class), the attribute alone is sufficient. ```csharp [MSBuildMultiThreadableTask] public class MyTask : Task, IMultiThreadableTask { public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; ... } ``` **Note**: `[MSBuildMultiThreadableTask]` has `Inherited = false` — it must be on each concrete class, not just the base. The corollary is easy to miss: the base class still executes multithreaded, but is not analyzed. Audit the whole base chain — see [Unsafe Code in an Unannotated Base Class](#unsafe-code-in-an-unannotated-base-class). ### Step 2: Absolutize Paths Before File Operations All path strings must be absolutized with `TaskEnvironment.GetAbsolutePath()` before use in file system APIs. This resolves paths relative to the project directory, not the process working directory. ```csharp AbsolutePath absolutePath = TaskEnvironment.GetAbsolutePath(inputPath); if (File.Exists(absolutePath)) { string content = File.ReadAllText(absolutePath); } ``` The [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) struct: - `Value` — the absolute path string - `OriginalValue` — preserves the input path (use for error messages and `[Output]` properties) - Implicitly convertible to `string` for File/Directory API compatibility - `GetCanonicalForm()` — resolves `..` segments and normalizes separators (see [Sin 5](#sin-5-canonicalization-mismatch)) **CAUTION**: `GetAbsolutePath()` throws `ArgumentException` for null/empty inputs. See [Sin 3](#sin-3-null-coalescing-that-changes-control-flow) and [Sin 6](#sin-6-exception-type-change) for compatibility implications. ### Step 3: Replace Environment Variable APIs | BEFORE (UNSAFE) | AFTER (SAFE) | |--------------------------------------------------|----------------------------------------------------| | `Environment.GetEnvironmentVariable("VAR");` | `TaskEnvironment.GetEnvironmentVariable("VAR");` | | `Environment.SetEnvironmentVariable("VAR", "v");` | `TaskEnvironment.SetEnvironmentVariable("VAR", "v");` | ### Step 4: Replace Process Start APIs | BEFORE (UNSAFE - inherits process state) | AFTER (SAFE - uses task's isolated environment) | |----------------------------------------------------|-----------------------------------------------------| | `var psi = new ProcessStartInfo("tool.exe");` | `var psi = TaskEnvironment.GetProcessStartInfo();` | | | `psi.FileName = GetFullPathToTool(); // must be absolute` | ## Updating Unit Tests Built-in MSBuild tasks now initialize `TaskEnvironment` with a `MultiProcessTaskEnvironmentDriver`-backed default. Tests creating instances of built-in tasks no longer need manual `TaskEnvironment` setup. For custom or third-party tasks that implement `IMultiThreadableTask` without a default initializer, set `TaskEnvironment = TaskEnvironment.Fallback` (or use `TaskEnvironment.CreateWithProjectDirectoryAndEnvironment(path)` to point at a specific project directory). ## APIs to Avoid | Category | APIs | Alternative | |---|---|---| | **Forbidden** | `Environment.Exit`, `FailFast`, `Process.Kill`, `ThreadPool.SetMin/MaxThreads`, `Console.*` | Return false, throw, or use `Log` | | **Use TaskEnvironment** | `Environment.CurrentDirectory`, `Get/SetEnvironmentVariable`, `Path.GetFullPath`, `ProcessStartInfo` | See Steps 2-4 | | **Need absolute paths** | `File.*`, `Directory.*`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader/Writer` | Absolutize first (File System APIs) | | **Need absolute paths (analyzer-invisible)** | `AssemblyName.GetAssemblyName`, `XDocument/XElement/XmlDocument.Load(string)`, `XmlReader/XmlWriter.Create(string)`, `ZipFile.*`, `X509CertificateLoader`, `Image.FromFile`, `Assembly.LoadFrom` | Absolutize first — **the analyzer does not flag these** | | **Review required** | `Assembly.Load*`, `Activator.CreateInstance*` | Check for version conflicts | ### Analyzer-Invisible Path Consumers `MSBuildTask0003` monitors a fixed list of types (`File`, `Directory`, `FileInfo`, `DirectoryInfo`, `FileStream`, `StreamReader`, `StreamWriter`, `FileSystemWatcher`). Any *other* API that accepts a path string and touches disk is equally unsafe but produces **no diagnostic**. In a ~150-task migration across dotnet/arcade, dotnet/source-build-assets and the dotnet/dotnet VMR, `AssemblyName.GetAssemblyName` on a raw input was the single most-repeated defect. Do not treat a clean analyzer run as evidence that path handling is complete — see [Verification](#verification-a-clean-analyzer-run-is-not-a-migration). Note the overload distinction: the *string* overloads are hazards; `XDocument.Load(stream)` and `new StreamReader(stream)` are fine, because the caller already resolved the path to open the stream. ## Practical Notes ### CRITICAL: Trace All Path String Usage Trace every path string through all method calls and assignments to find all places it flows into file system operations — including helper methods that may internally use File System APIs. 1. Find every path string (e.g., `item.ItemSpec`, function parameters) 2. Trace downstream through all method calls 3. Absolutize BEFORE any code path that touches the file system 4. Use `OriginalValue` for user-facing output (logs, errors) — see [Sin 2](#sin-2-errorlog-message-path-inflation) **Trace through abstractions, not just concrete calls.** A path can reach the file system without a single `System.IO` type appearing in the task. In `InstallDotNetTool` (dotnet/arcade), the `DestinationPath`, `DotnetPath` and `WorkingDirectory` inputs flow raw through `IFileSystem` and `ICommandFactory` interfaces — the task body looks completely clean, and the analyzer reports nothing. When a task input is passed to an interface or delegate, resolve it at the task boundary rather than hoping the implementation does. **Resolve once, at the boundary.** The recurring failure is not "forgot to absolutize" but "absolutized at three of four use sites". If a variable is going to be used as a path at all, convert it to `AbsolutePath` where it enters the task and keep it that way — see [Sin 8](#sin-8-swallowed-exceptions-hiding-an-unresolved-path). ### Exception Handling in Batch Operations In batch processing (iterating over files), `GetAbsolutePath()` throwing on one bad path aborts the entire batch. Match the original task's error semantics: ```csharp bool success = true; foreach (ITaskItem item in SourceFiles) { try { AbsolutePath path = TaskEnvironment.GetAbsolutePath(item.ItemSpec); ProcessFile(path); } catch (ArgumentException ex) { Log.LogError("Invalid path '{0}': {1}", item.ItemSpec, ex.Message); success = false; } } return success; ``` ### Prefer AbsolutePath Over String Stay in the `AbsolutePath` world — it's implicitly convertible to `string` where needed. Avoid round-tripping through `string` and back. ### TaskEnvironment is Not Thread-Safe If your task spawns multiple threads internally, synchronize access to `TaskEnvironment`. Each task *instance* gets its own environment, so no synchronization between tasks is needed. ### API Discipline When Plumbing Through Helpers When the migration ripples into shared helpers: - **Helper signatures: take `AbsolutePath`, not `(string path, string pathForMessages)`**. Two-string signatures drift apart; the caller passing one `AbsolutePath` keeps `.Value` and `.OriginalValue` in lockstep. - **Repo-wide helpers belong in an extensions class** (e.g., `TaskEnvironmentExtensions`), not on the task. If a per-task helper looks generic, extract it. - **Don't condition behavior on MT-mode**. `TaskEnvironment.Fallback` already gives single-process semantics; `if (mtMode) { … } else { … }` doubles the maintenance surface and skips test coverage of one branch. - **Don't silently swallow `ArgumentException` from `GetAbsolutePath`** in a helper — log a diagnostic so customers can debug bad inputs. ## References - [Thread-Safe Tasks Spec](https://github.com/dotnet/msbuild/blob/main/documentation/specs/multithreading/thread-safe-tasks.md) - [`AbsolutePath`](https://github.com/dotnet/msbuild/blob/main/src/Framework/PathHelpers/AbsolutePath.cs) - [`TaskEnvironment`](https://github.com/dotnet/msbuild/blob/main/src/Framework/TaskEnvironment.cs) - [`IMultiThreadableTask`](https://github.com/dotnet/msbuild/blob/main/src/Framework/IMultiThreadableTask.cs) --- # Compatibility Red-Team Playbook After migration, review for behavioral compatibility. **Every observable difference is a bug until proven otherwise.** Observable behavior = `Execute()` return value, `[Output]` property values, error/warning message content, exception types, files written, and which code path runs. ## The 8 Deadly Compatibility Sins Real bugs found during MSBuild task migrations. Every one shipped in initial "passing" code with green tests. **Edge-case discipline:** For every migrated code path, verify behavior when inputs are `null`, empty string (`""`), or whitespace-only. `GetAbsolutePath` throws `ArgumentException` on null/empty — if the pre-migration code handled these differently (e.g., returned early, used a default, or threw a different exception type), the migration must preserve that behavior. Even if a scenario seems unlikely, treat it as a relevant finding if it is theoretically possible. ### Sin 1: Output Property Contamination Absolutized values leak into `[Output]` properties that users/other tasks consume. ```csharp // BROKEN: ManifestPath was "bin\Release\app.manifest", now "C:\repo\bin\Release\app.manifest" AbsolutePath abs = TaskEnvironment.GetAbsolutePath(Path.Combine(OutputDirectory, name)); ManifestPath = abs; // implicit string conversion! // CORRECT: separate original form from absolutized path string originalPath = Path.Combine(OutputDirectory, name); AbsolutePath outputPath = TaskEnvironment.GetAbsolutePath(originalPath); ManifestPath = originalPath; // [Output]: original form document.Save((string)outputPath); // file I/O: absolute path ``` **Detect**: For every `[Output]` property, trace backward — is it ever assigned from an `AbsolutePath`? ### Sin 2: Error/Log Message Path Inflation Error messages and exception messages show absolutized paths instead of the user's original input. **Direct leakage** — passing an `AbsolutePath` to logging APIs: ```csharp // BROKEN: "Cannot find 'C:\repo\app.manifest'" instead of "Cannot find 'app.manifest'" AbsolutePath abs = TaskEnvironment.GetAbsolutePath(path); Log.LogError("Cannot find '{0}'", abs); // implicit conversion! // CORRECT: use OriginalValue Log.LogError("Cannot find '{0}'", abs.OriginalValue); ``` **Indirect leakage** — exception messages from helpers that received the absolutized path: ```csharp // BROKEN: ex.FileName / ex.Message embed the absolutized path catch (FileNotFoundException ex) { Log.LogError("Not found: {0}", ex.FileName); } catch (Exception ex) { Log.LogError(ex.Message); } // CORRECT: prefer the original input; if you must use the exception, sanitize catch (FileNotFoundException ex) { Log.LogError("Not found: {0}", abs.OriginalValue); } catch (Exception ex) { Log.LogError(ex.Message.Replace(abs.Value, abs.OriginalValue)); } ``` **Detect**: Search every `Log.LogError`/`LogWarning`/`LogMessage` — is any argument an `AbsolutePath`? Also check every `Log.LogError(ex.Message …)` / `ex.FileName` downstream of a `GetAbsolutePath` — did the exception originate from a helper that received the absolutized path? ### Sin 3: Null Coalescing That Changes Control Flow Adding `?? ""` silently swallows an exception the old code relied on for error handling. ```csharp // BEFORE: Path.GetDirectoryName("C:\") → null → Path.Combine(null, x) → ArgumentNullException // → task fails with an exception / error logged → Execute() returns false // BROKEN: ?? "" added "for safety" string dir = Path.GetDirectoryName(fileName) ?? string.Empty; // Path.Combine("", x) succeeds silently → no error → Execute() returns TRUE! ``` **Detect**: For every `??` you added, ask: "What happened when this was null before?" If it threw and was caught → your `??` is a bug. ### Sin 4: Try-Catch Scope Mismatch `GetAbsolutePath()` inside a try block leaves the absolutized value out of scope in the catch block. Helper methods in the catch (like `LockCheck`) then use the original non-absolute path. ```csharp // CORRECT: hoist above try so catch can use it too AbsolutePath abs = TaskEnvironment.GetAbsolutePath(OutputManifest.ItemSpec); try { WriteFile(abs); } catch (Exception ex) { string lockMsg = LockCheck.GetLockedFileMessage(abs); // absolute → correct file Log.LogError("Failed: {0}", OutputManifest.ItemSpec, ...); // original → user-friendly } ``` **Detect**: For every `GetAbsolutePath` inside a try, check if the catch block needs the absolutized value. ### Sin 5: Canonicalization Mismatch `GetAbsolutePath` does NOT canonicalize. `Path.GetFullPath` does TWO things: absolutize AND canonicalize (`..` resolution, separator normalization). If the old code used `Path.GetFullPath` for dictionary keys, comparisons, or display, you must add `.GetCanonicalForm()`: ```csharp // GetAbsolutePath("foo/../bar") → "C:\repo\foo/../bar" (NOT canonical) // Path.GetFullPath("foo/../bar") → "C:\repo\bar" (canonical) // BROKEN for dictionary keys — "C:\repo\foo\..\bar" ≠ "C:\repo\bar"