Skip to content

Fix possible race condition in ProjectRootElementCache.Get#7135

Merged
Forgind merged 5 commits intodotnet:mainfrom
AR-May:fix-race-condition-ProjectRootElementCache-Get
Dec 23, 2021
Merged

Fix possible race condition in ProjectRootElementCache.Get#7135
Forgind merged 5 commits intodotnet:mainfrom
AR-May:fix-race-condition-ProjectRootElementCache-Get

Conversation

@AR-May
Copy link
Copy Markdown
Member

@AR-May AR-May commented Dec 8, 2021 •
edited by rainersigwald
Loading

Fixes possible race condition in ProjectRootElementCache.Get supposedly introduced in #6680.

Context

Passing a load function that uses ProjectRootElementCache.Get to ProjectRootElementCache.Get function could lead to failure (to verify a path in the loaded ProjectRootElement), due to a race condition. We eliminate calling ProjectRootElementCache.Get inside the load function, eliminating an unnecessary double entry to ProjectRootElementCache.Get function.

Changes Made

  • Remove double entry to ProjectRootElementCache.Get function that could cause race condition.
  • Adjust path verification in ProjectRootElementCache and SimpleProjectRootElementCache to be more safe and to have more information.

Testing

Unit tests + manual testing

Notes

The stack trace of failure:

> ##[error]C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): Error : MSB0001: Internal MSBuild Error: Got project back with incorrect path 
> at Microsoft.Build.Shared.ErrorUtilities.ThrowInternalError(String message, Exception innerException, Object[] args) 
> at Microsoft.Build.Evaluation.ProjectRootElementCache.Get(String projectFile, OpenProjectRootElement openProjectRootElement, Boolean isExplicitlyLoaded, Nullable1 preserveFormatting) 
> at Microsoft.Build.Evaluation.Evaluator4.ExpandAndLoadImportsFromUnescapedImportExpression(String directoryOfImportingFile, ProjectImportElement importElement, String unescapedExpression, Boolean throwOnFileNotExistsError, List1& imports) 
> at Microsoft.Build.Evaluation.Evaluator4.ExpandAndLoadImportsFromUnescapedImportExpressionConditioned(String directoryOfImportingFile, ProjectImportElement importElement, List1& projects, SdkResult& sdkResult, Boolean throwOnFileNotExistsError) 
> at Microsoft.Build.Evaluation.Evaluator4.ExpandAndLoadImports(String directoryOfImportingFile, ProjectImportElement importElement, SdkResult& sdkResult) 
> at Microsoft.Build.Evaluation.Evaluator4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement) 
> at Microsoft.Build.Evaluation.Evaluator4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport) 
> at Microsoft.Build.Evaluation.Evaluator4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement) 
> at Microsoft.Build.Evaluation.Evaluator4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport) 
> at Microsoft.Build.Evaluation.Evaluator4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement) 
> at Microsoft.Build.Evaluation.Evaluator4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport) 
> at Microsoft.Build.Evaluation.Evaluator4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement) 
> at Microsoft.Build.Evaluation.Evaluator4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport) 
> at Microsoft.Build.Evaluation.Evaluator4.Evaluate()

AR-May
AR-May commented Dec 8, 2021
View reviewed changes
@AR-May AR-May marked this pull request as ready for review December 8, 2021 17:39
Forgind
Forgind reviewed Dec 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

I imagine this would hurt perf? Do you have numbers on by how much?

@AR-May AR-May force-pushed the fix-race-condition-ProjectRootElementCache-Get branch from 542ac14 to 6634a66 Compare December 8, 2021 18:25
@rainersigwald
Copy link
Copy Markdown
Member

rainersigwald commented Dec 8, 2021

Would you mind pasting a stack that shows the double call into the PR description? I remember this looked good when we talked about it but I've forgotten the details and they'd be good to have here for posterity.

@AR-May
Copy link
Copy Markdown
Member Author

AR-May commented Dec 8, 2021

No, it should not affect perf at all. Posted a stack trace of the error in the description.

The problem here is that we had double entry into Get() function of a cache, which is completely unnecessary and under some conditions it may lead to modifying FullPath right after line 276 in ProjectRootElementCache.cs and before the check on line 278. Logic is following: If load function openProjectRootElement has a Get() call inside, we do not load the xml and instead take element from a cache. 'projectRootElement ' is no more only locally accessible variable and some other thread could change it. Therefore, the check fails.

@AR-May
Copy link
Copy Markdown
Member Author

AR-May commented Dec 8, 2021 •
edited
Loading

The stack trace does not show the double call, cause the error happens in the time when we are already in the first call and just came out of the second call. (presumably)

@rokonec
Copy link
Copy Markdown
Member

rokonec commented Dec 9, 2021 •
edited
Loading

@rainersigwald

Would you mind pasting a stack that shows the double call into the PR description? I remember this looked good when we talked about it but I've forgotten the details and they'd be good to have here for posterity.

It was OK, but recently we have made changes that Get was not in one atomic lock but split into multiple lock se we can run loading of XML outside of lock, and this is sensitive against Get same thread reentrancy/recursion.

KirillOsenkov
KirillOsenkov reviewed Dec 13, 2021
View reviewed changes
KirillOsenkov
KirillOsenkov reviewed Dec 13, 2021
View reviewed changes
@AR-May AR-May marked this pull request as draft December 14, 2021 09:42
@AR-May AR-May force-pushed the fix-race-condition-ProjectRootElementCache-Get branch from e550dd1 to b43968f Compare December 14, 2021 15:33
@AR-May AR-May marked this pull request as ready for review December 15, 2021 08:08
@AR-May AR-May requested a review from rokonec December 15, 2021 08:08
rainersigwald
rainersigwald approved these changes Dec 15, 2021
View reviewed changes
@AR-May AR-May added merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. and removed Area: Performance labels Dec 17, 2021
@Forgind Forgind merged commit df510e2 into dotnet:main Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@KirillOsenkov KirillOsenkov KirillOsenkov left review comments

@rainersigwald rainersigwald rainersigwald approved these changes

@rokonec rokonec rokonec approved these changes

+2 more reviewers

@ladipro ladipro ladipro approved these changes

@Forgind Forgind Forgind approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@AR-May @rainersigwald @rokonec @KirillOsenkov @ladipro @Forgind