Skip to content

Conversation

@MichalPavlik
Copy link
Member

Fixes #12520

Context

There's no need to use locking in the LoadProject method—it’s not required in this context.

Changes Made

Taking the lock out of the equation let things run in parallel.

Testing

Existing tests.

Notes

Copilot AI review requested due to automatic review settings November 13, 2025 14:01
Copilot finished reviewing on behalf of MichalPavlik November 13, 2025 14:05
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 attempts to enable parallel project evaluation by removing the write lock from the LoadProject method. However, this introduces a critical race condition that can cause failures in multi-threaded scenarios.

Key Changes:

  • Removed the outer _locker.EnterDisposableWriteLock() that wrapped the entire method logic
  • De-indented all code that was inside the lock block
  • Minor code style improvements (named parameters like nodeId: 0 and isExplicitlyLoaded: true, and using ??= operator)

Comment on lines +1268 to +1273
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);

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.
Comment on lines +1234 to 1240
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);
}
}
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ProjectCollection.LoadProject doesn't allow projects to be evaluated in parallel

2 participants