Save thread working directory for fallback in Expander and Modifiers#12875
Conversation
|
@rainersigwald @AR-May this was in the prototype branch and seems very painful to avoid the thread static, so I just took it and made the lifecycle better. Should I try more to figure out the expander and metadata another way? I think it's quite bad UX to ban taskitem.GetMetadata("FullPath") in enlightened tasks :( that seems like something that customers would expect to work. |
There was a problem hiding this comment.
Pull request overview
This PR introduces thread-local working directory management to fix path resolution issues in multithreaded builds (issue #12851). The solution adds a thread-static CurrentThreadWorkingDirectory field in FileUtilities that is managed through the TaskEnvironment lifecycle via the IDisposable pattern. This allows the Expander and Modifiers (specifically %(FullPath) and Path.GetFullPath()) to resolve relative paths correctly in multithreaded mode without accessing the actual process current directory.
Key Changes:
- Added
IDisposabletoITaskEnvironmentDriverfor cleaning up thread-local state - Thread-static
CurrentThreadWorkingDirectoryfield set/cleared byMultiThreadedTaskEnvironmentDriver - Fallback to thread-local CWD in Modifiers and WellKnownFunctions when current directory is unavailable
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Shared/FileUtilities.cs | Adds CurrentThreadWorkingDirectory thread-static field for multithreaded path resolution |
| src/Shared/Modifiers.cs | Uses thread-local CWD as fallback when currentDirectory is null for %(FullPath) modifier |
| src/Build/Evaluation/Expander/WellKnownFunctions.cs | Uses thread-local CWD for Path.GetFullPath() function in property expansion |
| src/Framework/ITaskEnvironmentDriver.cs | Extends interface to implement IDisposable for cleanup |
| src/Framework/TaskEnvironment.cs | Adds internal Dispose() method to trigger driver cleanup |
| src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs | Sets thread-static on property setter and clears it in Dispose() |
| src/Build/BackEnd/TaskExecutionHost/MultiProcessTaskEnvironmentDriver.cs | Implements empty Dispose() for singleton pattern |
| src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEntry.cs | Disposes TaskEnvironment when build request completes |
| src/Build.UnitTests/BackEnd/TaskEnvironment_Tests.cs | Updates tests to properly dispose task environments |
Comments suppressed due to low confidence (1)
src/Build.UnitTests/BackEnd/TaskEnvironment_Tests.cs:360
- This test doesn't validate that
FileUtilities.CurrentThreadWorkingDirectoryis properly set during the test and cleaned up after disposal. Since the PR introduces thread-local state management, consider adding assertions to verify:
- That
FileUtilities.CurrentThreadWorkingDirectoryis set to the correct value whenProjectDirectoryis set - That
FileUtilities.CurrentThreadWorkingDirectoryis null afterDispose()is called
This would ensure the core functionality of the PR (thread-local working directory management) is properly tested.
[Fact]
public void TaskEnvironment_MultithreadedEnvironment_ShouldBeIsolatedFromSystem()
{
string testVarName = $"MSBUILD_MULTITHREADED_ISOLATION_TEST_{Guid.NewGuid():N}";
string testVarValue = "multithreaded_test_value";
using var driver = new MultiThreadedTaskEnvironmentDriver(
GetResolvedTempPath(),
new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase));
var multithreadedEnvironment = new TaskEnvironment(driver);
try
{
// Verify system and environement doesn't have the test variable initially
Environment.GetEnvironmentVariable(testVarName).ShouldBeNull();
multithreadedEnvironment.GetEnvironmentVariable(testVarName).ShouldBeNull();
// Set variable in multithreaded environment
multithreadedEnvironment.SetEnvironmentVariable(testVarName, testVarValue);
// Multithreaded should have the value but system should not
multithreadedEnvironment.GetEnvironmentVariable(testVarName).ShouldBe(testVarValue);
Environment.GetEnvironmentVariable(testVarName).ShouldBeNull();
}
finally
{
Environment.SetEnvironmentVariable(testVarName, null);
}
}
AR-May
left a comment
There was a problem hiding this comment.
LGTM, just one comment to resolve.
Co-authored-by: AR-May <[email protected]>
Fixes #12851 #12800
Context
there are 2 places where it's impractical to pass TaskEnvironment but the code relies on CWD
Changes Made
introduce AsyncLocal CWD variable tied to lifetime of a TaskEnvironment
Testing
updated existing tests to dispose the environment
demo this fixes:
AsyncLocalDemo.zip
Notes
this makes the Aspire starter project build