Skip to content

Enablement of parallel project evaluation #12757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MichalPavlik wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enablement of parallel project evaluation #12757

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 39 additions & 45 deletions src/Build/Definition/ProjectCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1221,63 +1221,57 @@ public Project LoadProject(string fileName, IDictionary<string, string> globalPr
ErrorUtilities.VerifyThrowArgumentLength(fileName);
fileName = FileUtilities.NormalizePath(fileName);

using (_locker.EnterDisposableWriteLock())
if (globalProperties == null)
{
if (globalProperties == null)
{
globalProperties = GlobalProperties;
}
else
globalProperties = GlobalProperties;
}
else
{
// We need to update the set of global properties to merge in the ProjectCollection global properties --
// otherwise we might end up declaring "not matching" a project that actually does ... and then throw
// an exception when we go to actually add the newly created project to the ProjectCollection.
// BUT remember that project global properties win -- don't override a property that already exists.
foreach (KeyValuePair<string, string> globalProperty in GlobalProperties)
{
// We need to update the set of global properties to merge in the ProjectCollection global properties --
// otherwise we might end up declaring "not matching" a project that actually does ... and then throw
// an exception when we go to actually add the newly created project to the ProjectCollection.
// BUT remember that project global properties win -- don't override a property that already exists.
foreach (KeyValuePair<string, string> globalProperty in GlobalProperties)
if (!globalProperties.ContainsKey(globalProperty.Key))
{
if (!globalProperties.ContainsKey(globalProperty.Key))
{
globalProperties.Add(globalProperty);
}
globalProperties.Add(globalProperty);
}
}
Comment on lines +1234 to 1240
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

Copilot uses AI. Check for mistakes.
}

// We do not control the current directory at this point, but assume that if we were
// passed a relative path, the caller assumes we will prepend the current directory.
string toolsVersionFromProject = null;
// We do not control the current directory at this point, but assume that if we were
// passed a relative path, the caller assumes we will prepend the current directory.
string toolsVersionFromProject = null;

if (toolsVersion == null)
if (toolsVersion == null)
{
// Load the project XML to get any ToolsVersion attribute.
// If there isn't already an equivalent project loaded, the real load we'll do will be satisfied from the cache.
// If there is already an equivalent project loaded, we'll never need this XML -- but it'll already
// have been loaded by that project so it will have been satisfied from the ProjectRootElementCache.
// Either way, no time wasted.
try
{
// Load the project XML to get any ToolsVersion attribute.
// If there isn't already an equivalent project loaded, the real load we'll do will be satisfied from the cache.
// If there is already an equivalent project loaded, we'll never need this XML -- but it'll already
// have been loaded by that project so it will have been satisfied from the ProjectRootElementCache.
// Either way, no time wasted.
try
{
ProjectRootElement xml = ProjectRootElement.OpenProjectOrSolution(fileName, globalProperties, toolsVersion, ProjectRootElementCache, true /*explicitlyloaded*/);
toolsVersionFromProject = (xml.ToolsVersion.Length > 0) ? xml.ToolsVersion : DefaultToolsVersion;
}
catch (InvalidProjectFileException ex)
{
var buildEventContext = new BuildEventContext(0 /* node ID */, BuildEventContext.InvalidTargetId, BuildEventContext.InvalidProjectContextId, BuildEventContext.InvalidTaskId);
LoggingService.LogInvalidProjectFileError(buildEventContext, ex);
throw;
}
ProjectRootElement xml = ProjectRootElement.OpenProjectOrSolution(fileName, globalProperties, toolsVersion, ProjectRootElementCache, isExplicitlyLoaded: true);
toolsVersionFromProject = (xml.ToolsVersion.Length > 0) ? xml.ToolsVersion : DefaultToolsVersion;
}

string effectiveToolsVersion = Utilities.GenerateToolsVersionToUse(toolsVersion, toolsVersionFromProject, GetToolset, DefaultToolsVersion, out _);
Project project = _loadedProjects.GetMatchingProjectIfAny(fileName, globalProperties, effectiveToolsVersion);

if (project == null)
catch (InvalidProjectFileException ex)
{
// The Project constructor adds itself to our collection,
// it is not done by us
project = new Project(fileName, globalProperties, effectiveToolsVersion, this);
var buildEventContext = new BuildEventContext(nodeId: 0, BuildEventContext.InvalidTargetId, BuildEventContext.InvalidProjectContextId, BuildEventContext.InvalidTaskId);
LoggingService.LogInvalidProjectFileError(buildEventContext, ex);
throw;
}

return project;
}

string effectiveToolsVersion = Utilities.GenerateToolsVersionToUse(toolsVersion, toolsVersionFromProject, GetToolset, DefaultToolsVersion, out _);
Project project = _loadedProjects.GetMatchingProjectIfAny(fileName, globalProperties, effectiveToolsVersion);

// The Project constructor adds itself to our collection,
// it is not done by us
project ??= new Project(fileName, globalProperties, effectiveToolsVersion, this);

Comment on lines +1268 to +1273
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

Critical Race Condition: Removing the lock creates a check-then-act race condition

Removing the write lock from this method introduces a race condition that can cause InvalidOperationException when multiple threads attempt to load the same project concurrently.

The Problem:

  1. Thread A calls GetMatchingProjectIfAny() at line 1268 (which has internal locking)
  2. Thread B calls GetMatchingProjectIfAny() at line 1268 (after Thread A releases its internal lock)
  3. Both threads find no matching project
  4. Both threads create a new Project instance at line 1272
  5. Thread A's Project constructor eventually calls AddProject() (via Initialize() → _renameHandler() → OnAfterRenameLoadedProject())
  6. Thread B's Project constructor attempts to call AddProject() but now Thread A's project exists
  7. Thread B's AddProject() throws InvalidOperationException with "OM_MatchingProjectAlreadyInCollection" (see LoadedProjectCollection.AddProject() at lines 2007-2011)

Why Internal Locking Isn't Sufficient:
While LoadedProjectCollection has internal locking on individual operations (GetMatchingProjectIfAny() and AddProject() both lock _loadedProjects), these are separate lock acquisitions. The original outer write lock ensured the entire check-and-add sequence was atomic.

Solution:
The write lock needs to be retained to ensure atomicity of the check-and-create operation. If parallel project loading is desired, a different approach is needed, such as:

  • Using a concurrent check-and-add pattern (try to add, if it fails because it exists, retrieve the existing one)
  • Using a lock-free algorithm with compare-and-swap semantics
  • Implementing a "get-or-create" method with internal locking in LoadedProjectCollection
Suggested change
Project project = _loadedProjects.GetMatchingProjectIfAny(fileName, globalProperties, effectiveToolsVersion);
// The Project constructor adds itself to our collection,
// it is not done by us
project ??= new Project(fileName, globalProperties, effectiveToolsVersion, this);
Project project;
_lock.EnterWriteLock();
try
{
project = _loadedProjects.GetMatchingProjectIfAny(fileName, globalProperties, effectiveToolsVersion);
// The Project constructor adds itself to our collection,
// it is not done by us
project ??= new Project(fileName, globalProperties, effectiveToolsVersion, this);
}
finally
{
_lock.ExitWriteLock();
}

Copilot uses AI. Check for mistakes.
return project;
}

/// <summary>
Expand Down
Loading