Thread-Safe Tasks spec

documentation/specs/MultithreadedMSBuild/thread-safe-tasks-api-reference.md

@@ -0,0 +1,154 @@
+# Thread-Safe Tasks: API Analysis Reference
+
+This document provides a list of .NET APIs that should not be used or should be used with caution in thread-safe tasks. These APIs are problematic because they either rely on or modify process-level state, which can cause race conditions in multithreaded execution.
+
+The APIs listed in this document will be detected by Roslyn analyzers and/or MSBuild BuildCheck to help identify potential threading issues in tasks that implement `IThreadSafeTask`.
+
+**Note**: The analyzers rely on **static code analysis** and may not catch all dynamic scenarios (such as reflection-based API calls).
+
+## API Issues Categories
+
+Categories of threading issues with .NET API usage in thread-safe tasks to be aware of:
+
+1. **Working Directory Modifications and Usage**, such as file system operations with relative paths.
+1. **Environment Variables Modification and Usage**
+1. **Process Culture Modification and Usage**, which can affect data formatting.
+1. **Assembly Loading**
+1. **Static Fields**
+
+**TODO**: Consider other possible causes of issues:
+1. Creating new AppDomain.
+1. App config changes. 
+
+### Best Practices
+
+Instead of the problematic APIs listed below, thread-safe tasks should:
+
+1. **Use `ITaskExecutionContext`** for all file system operations, environment variable changes, and working directory changes.
+1. **Always use absolute paths** when still using some standard .NET file system APIs.
+1. **Explicitly configure external processes** with working directory and environment variables.
+1. **Never modify process culture**: Avoid modifying culture defaults.
+### Additional Considerations
+
+## Detailed API Reference
+
+The following tables list specific .NET APIs and their threading safety classification:
+
+### System.IO.Path Class
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| `Path.GetFullPath(string path)` | ERROR | Uses current working directory | Use MSBuild API |
+
+**TODO**: Check other methods and exclude any indirect dependency on working directory.
+
+### System.IO.File Class
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| All methods | ERROR | Uses current working directory | Use absolute paths |
+
+### System.IO.Directory Class
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| All methods | ERROR | Uses current working directory | Use absolute paths |
+
+### System.Environment Class
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+|  All properties setters | ERROR | Modifies process-level state | Use MSBuild API |
+| `Environment.CurrentDirectory` (getter, setter) | ERROR | Accesses process-level state | Use MSBuild API |
+| `Environment.Exit(int exitCode)` | ERROR | Terminates entire process | Return false from task or throw exception |
+| `Environment.FailFast` all overloads | ERROR | Terminates entire process | Return false from task or throw exception |
+| All other methods | ERROR | Modifies process-level state | Use MSBuild API |
+
+### System.IO.FileInfo Class
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| Constructor `FileInfo(string fileName)` | WARNING | Uses current working directory | Use absolute paths |
+| `CopyTo` all overloads | WARNING | Destination path relative to current directory | Use absolute paths |
+| `MoveTo` all overloads | WARNING | Destination path relative to current directory | Use absolute paths |
+| `Replace` all overloads | WARNING | Paths relative to current directory | Use absolute paths |
+
+### System.IO.DirectoryInfo Class
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| Constructor `DirectoryInfo(string path)` | WARNING | Uses current working directory | Use absolute paths |
+| `MoveTo(string destDirName)` | WARNING | Destination path relative to current directory | Use absolute paths |
+
+### System.IO.FileStream Class
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| Constructor `FileStream` all overloads | WARNING | Uses current working directory | Use absolute paths |
+
+### System.IO Stream Classes
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| Constructor `StreamReader` all overloads | WARNING | Uses current working directory | Use absolute paths |
+
+### System.Diagnostics.Process Class
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| All properties setters | ERROR | Modifies process-level state | Avoid |
+| `Process.GetCurrentProcess().Kill()` | ERROR | Terminates entire process | Avoid |
+| `Process.GetCurrentProcess().Kill(bool entireProcessTree)` | ERROR | Terminates entire process | Avoid |
+| `Process.Start` all overloads | ERROR | May inherit process state | Use MSBuild API |
+
+### System.Diagnostics.ProcessStartInfo Class
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| Constructor `ProcessStartInfo()` all overloads | ERROR | May inherit process state | Use MSBuild API |
+
+### System.Threading.ThreadPool Class
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| `ThreadPool.SetMinThreads(int workerThreads, int completionPortThreads)` | ERROR | Modifies process-wide settings | Avoid |
+| `ThreadPool.SetMaxThreads(int workerThreads, int completionPortThreads)` | ERROR | Modifies process-wide settings | Avoid |
+
+### System.Globalization.CultureInfo Class
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| `CultureInfo.DefaultThreadCurrentCulture` (setter) | ERROR | Affects new threads | Modify the thread culture instead |
+| `CultureInfo.DefaultThreadCurrentUICulture` (setter) | ERROR | Affects new threads | Modify the thread culture instead |
+
+### Static
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| Static fields | WARNING | Shared across threads, can cause race conditions | Avoid |
+
+### Assembly Loading (System.Reflection.Assembly class, System.Activator class)
+Tasks that load assemblies dynamically in the task host may cause version conflicts. Version conflicts in task assemblies will cause build failures (previously these might have been sporadic). Both dynamically loaded dependencies and static dependencies can cause issues.
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| `Assembly.LoadFrom(string assemblyFile)` | WARNING | May cause version conflicts | Be aware of potential conflicts, use absolute paths |
+| `Assembly.LoadFile(string path)` | WARNING | May cause version conflicts | Be aware of potential conflicts |
+| `Assembly.Load` all overloads | WARNING | May cause version conflicts | Be aware of potential conflicts |
+| `Assembly.LoadWithPartialName(string partialName)` | WARNING | May cause version conflicts | Be aware of potential conflicts |
+| `Activator.CreateInstanceFrom(string assemblyFile, string typeName)` | WARNING | May cause version conflicts | Be aware of potential conflicts |
+| `Activator.CreateInstance(string assemblyName, string typeName)` | WARNING | May cause version conflicts | Be aware of potential conflicts |
+| `AppDomain.Load` all overloads | WARNING | May cause version conflicts | Be aware of potential conflicts |
+| `AppDomain.CreateInstanceFrom(string assemblyFile, string typeName)` | WARNING | May cause version conflicts | Be aware of potential conflicts |
+| `AppDomain.CreateInstance(string assemblyName, string typeName)` | WARNING | May cause version conflicts | Be aware of potential conflicts |
+
+### P/Invoke
+
+**Concerns**:
+- P/Invoke calls may use process-level state like current working directory
+- Native code may not be thread-safe
+- Native APIs may modify global process state
+
+| API | Level | Short Reason | Recommendation |
+|-----|-------|--------------|-------|
+| `[DllImport]` attribute | WARNING | Not covered by analyzers | Review for thread safety, use absolute paths |

documentation/specs/MultithreadedMSBuild/thread-safe-tasks.md

@@ -0,0 +1,171 @@
+# Thread-Safe Tasks
+
+## Overview
+
+MSBuild's current execution model assumes that tasks have exclusive control over the entire process during execution. This allows tasks to freely modify global process state such as environment variables, the current working directory, and other process-level resources. This design works well for MSBuild's approach of executing builds in separate processes for parallelization. With the introduction of multithreaded execution within a single MSBuild process, multiple tasks can now run concurrently. This requires a new task design to ensure thread safety when multiple tasks access shared process state simultaneously.
+
+To enable this multithreaded execution model, we introduce the `IThreadSafeTask` interface that tasks can implement to declare their thread-safety capabilities. Tasks implementing this interface must avoid using APIs that modify or depend on global process state, as this could cause conflicts when multiple tasks execute concurrently. See [Thread-Safe Tasks API Reference](thread-safe-tasks-api-reference.md) for detailed guidelines. At the same time, the `IThreadSafeTask` interface provides access to the `ExecutionContext` property that allows safe access to global process state. For example, task authors should use `ExecutionContext.GetAbsolutePath(relativePath)` instead of the standard `Path.GetFullPath(relativePath)` to ensure correct path resolution.
+
+## Design
+
+```csharp
+public interface IThreadSafeTask : ITask
+{
+    ITaskExecutionContext ExecutionContext { get; set; }
+}
+```
+
+**Note:** Consider backporting the `IThreadSafeTask` interface to the 17.14 branch to allow a graceful fail when tasks attempt to use it.
+
+The execution context provides essential methods for thread-safe task execution, replacing direct access to global process state:
+
+```csharp
+public interface ITaskExecutionContext
+{
+    AbsolutePath CurrentDirectory { get; set; }
+
+    AbsolutePath GetAbsolutePath(string path);
+
+    string? GetEnvironmentVariable(string name);
+    IReadOnlyDictionary<string, string> GetEnvironmentVariables();
+    void SetEnvironmentVariable(string name, string? value);
+
+    ProcessStartInfo GetProcessStartInfo();
+    Process StartProcess(ProcessStartInfo startInfo);
+    Process StartProcess(string fileName);
+    Process StartProcess(string fileName, IEnumerable<string> arguments);
+}
+```
+
+**Note:** The `ITaskExecutionContext` will not be thread-safe for performance reasons. Task authors who spawn multiple threads within their task implementation must provide their own synchronization when accessing the execution context from multiple threads. However, each thread node has its own isolated context object provided to the tasks, so task authors do not need to worry about synchronization with other tasks running concurrently in different thread nodes.
+
+To help task authors avoid thread-safety issues related to path handling, we introduce `AbsolutePath` and `RelativePath` classes that are implicitly convertible to string. 
+
+```csharp
+public class AbsolutePath
+{
+    public string Path { get; }
+
+    // Will be banned for use in tasks by analyzers
+    public AbsolutePath(string path) { }
+
+    public AbsolutePath(string path, AbsolutePath basePath) { }
+
+    public static implicit operator string(AbsolutePath path) { }
+}
+
+public class RelativePath
+{
+    public string Path { get; }
+
+    public RelativePath(string path) { }
+
+    public AbsolutePath ToAbsolute(AbsolutePath? basePath = null) { }
+
+    public static implicit operator string(RelativePath path) { }
+}
+```
+
+**Note:** Usage of `AbsolutePath` and `RelativePath` may need adjustment if similar concepts are implemented in the standard .NET API in the future.
+
+**TODO:** Consider having conversions for `ITaskItem`.
+
+### Interface Evolution Strategy
+
+To handle future updates without breaking existing implementations, we will create versioned interfaces:
+
+```csharp
+public interface ITaskExecutionContext2 : ITaskExecutionContext
+{
+    // New methods for version 2
+}
+```
+
+#### Alternative: Abstract Classes
+
+Alternatively, we can use abstract classes:
+
+```csharp
+public interface IThreadSafeTask : ITask
+{
+    TaskExecutionContext ExecutionContext { get; set; }
+}
+
+public abstract class TaskExecutionContext
+{ 
+    // Same methods as the interface, with default implementations
+}
+```
+
+**Note:** 
+- Default implementations allow backward compatibility for customers who extend the class.
+- However, version compatibility checking is not possible with this design.
+
+## Authoring Thread-Safe Tasks
+
+### Supporting Running 'classic' MSBuild Tasks in Thread Node
+
+Task authors implementing `IThreadSafeTask` need to:
+
+- Update package dependencies to support thread-safe APIs
+- Create and maintain both thread-safe and legacy versions if support for older MSBuild versions is necessary
+- Implement logic to choose appropriate task implementations based on MSBuild capabilities
+
+These requirements effectively cut off support for a long tail of MSBuild and IDE versions, creating a barrier to adoption.
+
+To allow easier adoption, MSBuild will enable the thread worker node to either recognize and wrap legacy tasks in thread-based semantics or run the 'classic' tasks themselves. This will allow task authors who are confident their tasks don't perform stateful actions to participate in multithreaded builds without sacrificing legacy support or needing to support multiple versions.
+
+Legacy tasks can run in multithreaded builds if they:
+- Do not change environment variables or current working directory
+- Do not depend on relative path resolution from current working directory
+- Do not use static fields or other shared state unsafely
+- Do not rely on specific environment variable values being set
+
+Task authors and users who would like to utilize this capability will need to add the `ThreadSafe` attribute to the task declaration:
+```xml
+  <UsingTask TaskName="MyTask" AssemblyFile="MyTask.dll" ThreadSafe="true" Condition="'$(MSBuildSupportsThreadSafeTasks)' != 'true'" />
+```
+
+**Note** We will need to service supported branches so that older versions of MSBuild ignore the `ThreadSafe` attribute and do not throw the `MSB4066` error. For MSBuild versions that would not be serviced, task authors will need to conditionally import the file with task declarations based on the running MSBuild version.
+
+**Note** Alternatively, we can introduce new types of assembly factories (`AssemblyThreadCompatibleTaskFactory` and others) to avoid the error. The task declarations still need to be conditional, but this would spare us the need to service older versions. 
+
+This support would be enabled by default for the first release of Multithreaded MSBuild, but the goal should be to deprecate it. We will disable the compatibility bridge by default eventually. We will use build-level telemetry to track 'classic' MSBuild tasks running in multithreaded mode to monitor `IThreadSafeTask` adoption trends.
+
+## Examples
+Basic `IThreadSafeTask` Example:
+```csharp
+public class MyTask : IThreadSafeTask
+{
+    public bool Execute()
+    {
+        // Use APIs provided by ExecutionContext
+        string envVar = ExecutionContext.GetEnvironmentVariable("EnvVar");
+
+        // Convert string properties to strongly-typed paths and use them in standard File/Directory APIs
+        AbsolutePath path = ExecutionContext.GetAbsolutePath("SomePath");
+        string content = File.ReadAllText(path);
+
+        return true;
+    }
+}
+```
+
+Conditional Task Declaration Example:
+```xml
+<Project>
+  <PropertyGroup>
+    <MSBuildSupportsThreadSafeTasks Condition="'$(MSBuildVersion)' >= '17.15'">true</MSBuildSupportsThreadSafeTasks>
+  </PropertyGroup>
+
+  <UsingTask 
+    TaskName="MyTask" 
+    AssemblyFile="$(MSBuildThisFileDirectory)../lib/netstandard2.0/ThreadSafe/MyTask.dll" 
+    Condition="'$(MSBuildSupportsThreadSafeTasks)' == 'true'" />
+
+  <UsingTask 
+    TaskName="MyTask" 
+    AssemblyFile="$(MSBuildThisFileDirectory)../lib/netstandard2.0/Legacy/MyTask.dll" 
+    Condition="'$(MSBuildSupportsThreadSafeTasks)' != 'true'" />
+</Project>
+```