Fix race conditions in AssemblyTaskFactory class.#13422
Merged
AR-May merged 2 commits intodotnet:mainfrom Mar 24, 2026
Merged
Conversation
Member
Author
|
It seems the original issue it is not reproducible for us, most probably because of the small number of enlightened tasks compared to the prototype where it was detected, which had more enlightened tasks. |
Contributor
There was a problem hiding this comment.
Pull request overview
Addresses intermittent multi-threaded build crashes by making AssemblyTaskFactory safe to use concurrently when the same factory instance is shared across nodes/projects.
Changes:
- Removed per-task mutable state (
_taskLoggingContext) and switched telemetry logging to use the per-calltaskLoggingContextparameter. - Replaced the .NET Framework AppDomain tracking map with a
ConcurrentDictionaryand updated cleanup to useTryRemove.
JanProvaznik
approved these changes
Mar 20, 2026
This was referenced Mar 24, 2026
dfederm
pushed a commit
to dfederm/msbuild
that referenced
this pull request
Apr 9, 2026
Fixes dotnet#12867 ### Context Building large repos (e.g., Roslyn) in multi-threaded mode crashes intermittently: error MSB4061: The "CombineTargetFrameworkInfoProperties" task could not be instantiated... Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. Root Cause `AssemblyTaskFactory` instances are shared across thread nodes — they're cached in `TaskRegistry._taskFactoryWrapperInstance`. `TaskRegistry` itself is shared across thread nodes by design. In mt builds, multiple projects call `CreateTaskInstance()` on the same `AssemblyTaskFactory` instance concurrently from different threads. This exposed two thread-safety issues: 1. `_taskLoggingContext` field race (crash-causing) Race condition example: When Thread A (Project A) and Thread B (Project B) concurrently call `CreateTaskInstance()`: 1. Thread A sets `_taskLoggingContext = contextA` 2. Thread B overwrites `_taskLoggingContext = contextB` 3. Thread A reads `_taskLoggingContext` for `TrackTaskSubclassing` → gets `contextB` → writes to Project B's ProjectTelemetry 4. Thread B also writes to Project B's ProjectTelemetry 5. Two threads mutate the same Dictionary<string, int> → crash The `ProjectTelemetry` in turn is not supposed to be accessed from multiple threads - only one thread node should build a project. It does not need to be thread-safe. `_taskLoggingContext` field existed so that `ErrorLoggingDelegate` (a named method passed as a callback to `TaskLoader.CreateTask()`) could access the logging context, but it is not really required. For mt mode support, there should not be mutable fields in `AssemblyTaskFactory` that relate to a specific task. 2. `_tasksAndAppDomains` dictionary (in .NET Framework) `Dictionary<ITask, AppDomain>` is not safe for concurrent structural mutations. While each thread operates on different keys (unique ITask instances), concurrent add/remove on different keys still corrupts a plain Dictionary. ### Changes Made - Removed `_taskLoggingContext` field entirely. - Changed `_tasksAndAppDomains` from `Dictionary<ITask, AppDomain>` to `ConcurrentDictionary<ITask, AppDomain>` Note that lock contention should be negligible, as each thread would add and clean-up its own set of keys. ### Testing Existing unit tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #12867
Context
Building large repos (e.g., Roslyn) in multi-threaded mode crashes intermittently:
error MSB4061: The "CombineTargetFrameworkInfoProperties" task could not be instantiated...
Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state.
Root Cause
AssemblyTaskFactoryinstances are shared across thread nodes — they're cached inTaskRegistry._taskFactoryWrapperInstance.TaskRegistryitself is shared across thread nodes by design. In mt builds, multiple projects callCreateTaskInstance()on the sameAssemblyTaskFactoryinstance concurrently from different threads. This exposed two thread-safety issues:_taskLoggingContextfield race (crash-causing)Race condition example: When Thread A (Project A) and Thread B (Project B) concurrently call
CreateTaskInstance():_taskLoggingContext = contextA_taskLoggingContext = contextB_taskLoggingContextforTrackTaskSubclassing→ getscontextB→ writes to Project B's ProjectTelemetryThe
ProjectTelemetryin turn is not supposed to be accessed from multiple threads - only one thread node should build a project. It does not need to be thread-safe._taskLoggingContextfield existed so thatErrorLoggingDelegate(a named method passed as a callback toTaskLoader.CreateTask()) could access the logging context, but it is not really required. For mt mode support, there should not be mutable fields inAssemblyTaskFactorythat relate to a specific task._tasksAndAppDomainsdictionary (in .NET Framework)Dictionary<ITask, AppDomain>is not safe for concurrent structural mutations. While each thread operates on different keys (unique ITask instances), concurrent add/remove on different keys still corrupts a plain Dictionary.Changes Made
_taskLoggingContextfield entirely._tasksAndAppDomainsfromDictionary<ITask, AppDomain>toConcurrentDictionary<ITask, AppDomain>Note that lock contention should be negligible, as each thread would add and clean-up its own set of keys.
Testing
Existing unit tests