SimpleProjectRootElementCache throws on SDK resolution

[dfederm]

Issue Description

In QuickBuild we opt-into MsBuildUseSimpleProjectRootElementCacheConcurrency, and recently this started causing exceptions:

Stack:

System.NotImplementedException: The method or operation is not implemented.
   at Microsoft.Build.Evaluation.SimpleProjectRootElementCache.OnProjectRootElementDirtied(ProjectRootElement sender, ProjectXmlChangedEventArgs e)
   at Microsoft.Build.Evaluation.Evaluator`4.<>c__DisplayClass71_0.<CreateProjectForSdkResult>g__InnerCreate|0(String _, ProjectRootElementCacheBase __)
   at Microsoft.Build.Evaluation.SimpleProjectRootElementCache.<>c__DisplayClass4_0.<GetFromOrAddToCache>b__0(String key)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Microsoft.Build.Evaluation.Evaluator`4.CreateProjectForSdkResult(SdkResult sdkResult)
   at Microsoft.Build.Evaluation.Evaluator`4.ExpandAndLoadImportsFromUnescapedImportExpressionConditioned(String directoryOfImportingFile, ProjectImportElement importElement, List`1& projects, SdkResult& sdkResult)
   at Microsoft.Build.Evaluation.Evaluator`4.ExpandAndLoadImports(String directoryOfImportingFile, ProjectImportElement importElement, SdkResult& sdkResult)
   at Microsoft.Build.Evaluation.Evaluator`4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement)
   at Microsoft.Build.Evaluation.Evaluator`4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport)
   at Microsoft.Build.Evaluation.Evaluator`4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement)
   at Microsoft.Build.Evaluation.Evaluator`4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport)
   at Microsoft.Build.Evaluation.Evaluator`4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement)
   at Microsoft.Build.Evaluation.Evaluator`4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport)
   at Microsoft.Build.Evaluation.Evaluator`4.Evaluate()
   at Microsoft.Build.Evaluation.Evaluator`4.Evaluate(IEvaluatorData`4 data, Project project, ProjectRootElement root, ProjectLoadSettings loadSettings, Int32 maxNodeCount, PropertyDictionary`1 environmentProperties, ILoggingService loggingService, IItemFactory`2 itemFactory, IToolsetProvider toolsetProvider, IDirectoryCacheFactory directoryCacheFactory, ProjectRootElementCacheBase projectRootElementCache, BuildEventContext buildEventContext, ISdkResolverService sdkResolverService, Int32 submissionId, EvaluationContext evaluationContext, Boolean interactive)
   at Microsoft.Build.Execution.ProjectInstance.Initialize(ProjectRootElement xml, IDictionary`2 globalProperties, String explicitToolsVersion, String explicitSubToolsetVersion, Int32 visualStudioVersionFromSolution, BuildParameters buildParameters, ILoggingService loggingService, BuildEventContext buildEventContext, ISdkResolverService sdkResolverService, Int32 submissionId, Nullable`1 projectLoadSettings, EvaluationContext evaluationContext, IDirectoryCacheFactory directoryCacheFactory)
   at Microsoft.Build.Execution.ProjectInstance..ctor(String projectFile, IDictionary`2 globalProperties, String toolsVersion, String subToolsetVersion, ProjectCollection projectCollection, Nullable`1 projectLoadSettings, EvaluationContext evaluationContext, IDirectoryCacheFactory directoryCacheFactory, Boolean interactive)
   at Microsoft.Build.Execution.ProjectInstance.FromFile(String file, ProjectOptions options)
   at Enlistment.DependencyParserV2.<>c__DisplayClass31_0.<ParseDirectory>b__1(String projectPath, Dictionary`2 globalProperties, ProjectCollection projCollection) in F:\dbs\el\cb_m\private\BuildEngine\Enlistment.Library\src\DependencyParserV2.cs:line 242
ERROR Error ENL21900:
Offending file: D:\Code\CloudBuild\private\BuildEngine\dirs.proj
Reason: Unexpected exception was thrown while parsing D:\Code\CloudBuild\private\BuildEngine\dirs.proj.
NotImplementedException message: The method or operation is not implemented..
Stack trace:    at Microsoft.Build.Evaluation.SimpleProjectRootElementCache.OnProjectRootElementDirtied(ProjectRootElement sender, ProjectXmlChangedEventArgs e)
   at Microsoft.Build.Evaluation.Evaluator`4.<>c__DisplayClass71_0.<CreateProjectForSdkResult>g__InnerCreate|0(String _, ProjectRootElementCacheBase __)
   at Microsoft.Build.Evaluation.SimpleProjectRootElementCache.<>c__DisplayClass4_0.<GetFromOrAddToCache>b__0(String key)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Microsoft.Build.Evaluation.Evaluator`4.CreateProjectForSdkResult(SdkResult sdkResult)
   at Microsoft.Build.Evaluation.Evaluator`4.ExpandAndLoadImportsFromUnescapedImportExpressionConditioned(String directoryOfImportingFile, ProjectImportElement importElement, List`1& projects, SdkResult& sdkResult)
   at Microsoft.Build.Evaluation.Evaluator`4.ExpandAndLoadImports(String directoryOfImportingFile, ProjectImportElement importElement, SdkResult& sdkResult)
   at Microsoft.Build.Evaluation.Evaluator`4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement)
   at Microsoft.Build.Evaluation.Evaluator`4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport)
   at Microsoft.Build.Evaluation.Evaluator`4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement)
   at Microsoft.Build.Evaluation.Evaluator`4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport)
   at Microsoft.Build.Evaluation.Evaluator`4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement)
   at Microsoft.Build.Evaluation.Evaluator`4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport)
   at Microsoft.Build.Evaluation.Evaluator`4.Evaluate()
   at Microsoft.Build.Evaluation.Evaluator`4.Evaluate(IEvaluatorData`4 data, Project project, ProjectRootElement root, ProjectLoadSettings loadSettings, Int32 maxNodeCount, PropertyDictionary`1 environmentProperties, ILoggingService loggingService, IItemFactory`2 itemFactory, IToolsetProvider toolsetProvider, IDirectoryCacheFactory directoryCacheFactory, ProjectRootElementCacheBase projectRootElementCache, BuildEventContext buildEventContext, ISdkResolverService sdkResolverService, Int32 submissionId, EvaluationContext evaluationContext, Boolean interactive)
   at Microsoft.Build.Execution.ProjectInstance.Initialize(ProjectRootElement xml, IDictionary`2 globalProperties, String explicitToolsVersion, String explicitSubToolsetVersion, Int32 visualStudioVersionFromSolution, BuildParameters buildParameters, ILoggingService loggingService, BuildEventContext buildEventContext, ISdkResolverService sdkResolverService, Int32 submissionId, Nullable`1 projectLoadSettings, EvaluationContext evaluationContext, IDirectoryCacheFactory directoryCacheFactory)
   at Microsoft.Build.Execution.ProjectInstance..ctor(String projectFile, IDictionary`2 globalProperties, String toolsVersion, String subToolsetVersion, ProjectCollection projectCollection, Nullable`1 projectLoadSettings, EvaluationContext evaluationContext, IDirectoryCacheFactory directoryCacheFactory, Boolean interactive)
   at Microsoft.Build.Execution.ProjectInstance.FromFile(String file, ProjectOptions options)

I don't have an exact version that this started and haven't isolated the issue to a specific change at this point.

The throw appears to happen here when setting the FullPath of the ProjectRootElement, which calls MarkDirty which calls OnProjectRootElementDirtied which is not implemented in SimpleProjectRootElementCache:

[dfederm]

Version where it repros:

>msbuild /version
MSBuild version 17.14.0-preview-25063-09+3b7f99137 for .NET Framework
17.14.0.6309

Version where it does not:

>msbuild /version
MSBuild version 17.13.9+e0f243f1e for .NET Framework
17.13.9.7704

Sorry, I don't have a more narrow range, just public preview vs int preview.

[dfederm]

I believe this change triggered the issue: dotnet/sdk#45364 (backported via dotnet/sdk#45459)

It appears to be adding PropertiesToAdd to the sdk result. Before when there were none, the CreateProjectForSdkResult code path would be skipped, but now that it's populated, CreateProjectForSdkResult is being called which appears to create a ProjectRootElement on the fly and causes SimpleProjectRootElementCache to throw.

[dfederm]

This implies that the CreateProjectForSdkResult method is using APIs which are causing a lot of dirtiness events to be raised unnecessarily. When loading a project file from disk, the XML document is parsed and then put into the ProjectRootElement all at once. CreateProjectForSdkResult is adding to the XML one element at a time.

A possible solution which both fixes this bug and may (very slightly) improve performance would be to build the XmlDocument first and use the overload of ProjectRootElement which takes a XmlDocumentWithLocation. That would avoid all the unnecessary events from being raised.

[YuliiaKovalova]

the sdk result. Before when there were none, the CreateProjectForSdkResult code path would be skipped, but now that it's populated, CreateProjectForSdkResult is being called which appears to create a ProjectRootElement on the fly and causes SimpleProjectRootElementCache to throw.

cc: @surayya-MS @baronfel

[rainersigwald]

@jeffkl @AndyGerlicher fyi it looks like this affects slngen too.

[NickCraver]

@rainersigwald @surayya-MS I have engineers reporting it their flows are broken (inability to use slngen, our entry point) - what's the current workaround/advice we're giving here? Is my understanding correct from the issue that it's the MSBuild version that broke binary compat here? (are we adding the old overload back?)

[rainersigwald]

It's not exactly a binary compat thing, it's the interaction of the SDK and the not-exactly-supported MsBuildUseSimpleProjectRootElementCacheConcurrency. I wonder if while investigating a long-term fix we should see if we can produce an slngen that doesn't use that.

Or I guess we could ignore the env var in MSBuild . . .

[NickCraver]

If it helps, I've just advised our team to not upgrade to 17.13.x if they haven't, and rollback if they have - this one's a pretty nasty blocker to doing much of anything given it's how we launch VS. If I can help unblock something here, happy to lend a hand.

[rainersigwald]

Wait, you're seeing this in 17.13? I only saw it in 17.14 builds, that definitely bumps the priority.

[PiDiBi]

Wait, you're seeing this in 17.13? I only saw it in 17.14 builds, that definitely bumps the priority.

yes, after update from 17.12.4 to 17.3 our team (200+ developers) is not able to generate sln files, spent few hours repairing and later reinstalling VS, until I run into this open bug, so now I know it's not my wrong VS installation

[dfederm]

not-exactly-supported MsBuildUseSimpleProjectRootElementCacheConcurrency

One discussion we had on our end is if this is still even useful. At the time it had significant performance gains for large repos, but perhaps it's worth re-evaluating to see if that's still the case.