Skip to content

Save thread working directory for fallback in Expander and Modifiers#12875

Merged
JanProvaznik merged 7 commits intodotnet:mainfrom
JanProvaznik:CurrentThreadWorkingDirectory
Dec 15, 2025
Merged

Save thread working directory for fallback in Expander and Modifiers#12875
JanProvaznik merged 7 commits intodotnet:mainfrom
JanProvaznik:CurrentThreadWorkingDirectory

Conversation

@JanProvaznik
Copy link
Member

@JanProvaznik JanProvaznik commented Dec 4, 2025

Fixes #12851 #12800

Context

there are 2 places where it's impractical to pass TaskEnvironment but the code relies on CWD

  1. Expander
  2. Modifier on TaskItems -> FullPath

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

@JanProvaznik JanProvaznik changed the title Save current thread working directory to fallback in Expander and Modifiers Save thread working directory for fallback in Expander and Modifiers Dec 4, 2025
@JanProvaznik
Copy link
Member Author

@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.

@JanProvaznik JanProvaznik marked this pull request as ready for review December 5, 2025 10:41
Copilot AI review requested due to automatic review settings December 5, 2025 10:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 IDisposable to ITaskEnvironmentDriver for cleaning up thread-local state
  • Thread-static CurrentThreadWorkingDirectory field set/cleared by MultiThreadedTaskEnvironmentDriver
  • 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.CurrentThreadWorkingDirectory is properly set during the test and cleaned up after disposal. Since the PR introduces thread-local state management, consider adding assertions to verify:
  1. That FileUtilities.CurrentThreadWorkingDirectory is set to the correct value when ProjectDirectory is set
  2. That FileUtilities.CurrentThreadWorkingDirectory is null after Dispose() 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);
            }
        }

@JanProvaznik JanProvaznik self-assigned this Dec 8, 2025
Copy link
Member

@AR-May AR-May left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one comment to resolve.

@JanProvaznik JanProvaznik enabled auto-merge (squash) December 15, 2025 09:46
@JanProvaznik JanProvaznik merged commit 8ed6aba into dotnet:main Dec 15, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix thread-safety issues in Expander.

4 participants