Avoid retaining memory while waiting for changes #67982
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs
Outdated
Show resolved
Hide resolved
sharwell
dismissed
CyrusNajmabadi’s stale review
This PR was completely rewritten to address a failure to clear all copies of data in a value type
| /// <summary> | ||
| /// The workspace this request is for, if applicable. This will be present if <see cref="Document"/> is | ||
| /// present. It will be <see langword="null"/> if <c>requiresLSPSolution</c> is false. | ||
| /// </summary> | ||
| public readonly Workspace? Workspace; | ||
| public Workspace? Workspace => _lspSolution?.Value.Workspace; |
There was a problem hiding this comment.
Can we make the exception a bit clearer if you use this after ClearSolutionContext is called? I assume you're doing the StrongBox to differentiate "this never had it" and "this was cleared", but that's not really confirmed anywhere.
(and frankly, I'm not sure why these wouldn't just throw if requiresLSPSolution is set, since I'd imagine that's a developer oops, but maybe I'm missing some context here)
There was a problem hiding this comment.
I assume you're doing the StrongBox to differentiate "this never had it" and "this was cleared"
No, the StrongBox<> is required because every time RequestContext is passed as an argument, a new copy is made. We need a common storage location for the references to Workspace and Solution that applies to all copies equally.
There was a problem hiding this comment.
➡️ Added code to throw for access to these properties if the solution context was initially available and has since been cleared.
ghost
added this to the 
Fixes AB#1809058