Fix memory leak in ProjectRootElement.Reload #6457
Conversation
|
I'll add a targeted regression test. |
|
|
||
| /// <summary> | ||
| /// Returns the number of documents retained by the project string cache. | ||
| /// Peeks at it via reflection since internals are not visible to these tests. |
There was a problem hiding this comment.
Is there any reason internals aren't visible to these tests? That seems like something we'd want for other tests later, too.
There was a problem hiding this comment.
I believe it's by design. These tests should be testing the public surface (or Object Model, as in Build.OM.UnitTests). I'm not confident that this test case belongs here. It is testing a public method but needs an internal hook to verify the expected behavior.
There was a problem hiding this comment.
It sounds a little better to me to move it to Build.UnitTests, but it isn't too important.
|
|
||
| // Test successful reload. | ||
| projectElement.Reload(false); | ||
| GetNumberOfDocumentsInProjectStringCache(projectElement).ShouldBe(originalDocumentCount); |
There was a problem hiding this comment.
I'm wondering if we should verify that the project is the new one the first time and the old one the second time. Not directly related to your change, though.
More related, I'd be in favor of reloading 3-4 times just in case there's something unexpected.
Forgind
added
the
merge-when-branch-open
Fixes #6456
Context
Calling
Xml.Reloadon aMicrosoft.Build.Evaluation.Projectleaks memory because the oldXmlDocumentis not removed from theProjectStringCache.Changes Made
ClearAnyCachedStringsis now called on the document that's not retained byProjectRootElement. This would normally be the old document being replaced byReload.Testing
Existing unit tests and manually verified using the repro snippet in #6456.
Notes
Targeting VS16.11 with this change as we've seen multiple feedback tickets related to this. In pathological scenarios the leak really OOMs the Visual Studio process.
Going forward we should see if we can eliminate the error-prone cache altogether (#5444).