Fix race condition in computing _hostAnalyzerStateMap #57648
Conversation
src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.StateManager.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.StateManager.cs
Outdated
Show resolved
Hide resolved
…ementalAnalyzer.StateManager.cs Co-authored-by: Andrew Hall <ryzngard@live.com>
src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.StateManager.cs
Outdated
Show resolved
Hide resolved
allisonchou
dismissed
sharwell’s stale review
The changes appear to be addressed and this fix needs to get in for servicing before tomorrow's deadline.
| } | ||
|
|
||
| public string Language { get; } | ||
| public HostDiagnosticAnalyzers Analyzers { get; } |
There was a problem hiding this comment.
📝 I requested that instead of using HostDiagnosticAnalyzers as the key directly, we use HostDiagnosticAnalyzers.GetHostAnalyzerReferencesMap(). This has the same net result for the keys but ensures we don't accidentally GC root the DiagnosticAnalyzer instances themselves (from other fields in HostDiagnosticAnalyzers) in the off chance this cache ends up with unforeseen items.
…olution state analyzers.
| @@ -15,7 +15,19 @@ internal partial class DiagnosticIncrementalAnalyzer | |||
| private partial class StateManager | |||
| { | |||
| public IEnumerable<StateSet> GetAllHostStateSets() | |||
| => _hostAnalyzerStateMap.Values.SelectMany(v => v.OrderedStateSets); | |||
There was a problem hiding this comment.
@sharwell @ryzngard need your inputs here.
This was returning values for solution analyzers from all solution snapshots, instead of current solution, which led to duplicates being returned and subsequently an assert was firing at https://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.StateManager.cs,309 in integration tests.
This commit has one way to fix this issue. Another way to fix it would be to restore _hostAnalyzerStateMap back to only have the language name as the key as before, and introduce a separate field in the state manager for current solution state analyzer references. When asked to compute host state sets, we would first check if given project's solution state analyzer references are the same as ones stored onto this new field, otherwise we clear _hostAnalyzerStateMap and recompute.
There was a problem hiding this comment.
I think strongly tying the _hostAnalyzerStateMap to requirements of uniqueness makes sense in the way it's already done. It makes it very clear what the inputs used for the current state map are without having to look at multiple fields. I know it does add overhead of passing more values, but I think overall I still like the current approach.
.../Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.StateManager.HostStates.cs
Outdated
Show resolved
Hide resolved
.../Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.StateManager.HostStates.cs
Show resolved
Hide resolved
…ementalAnalyzer.StateManager.HostStates.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>
Fixes [AB#1449967](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1449967) dotnet#57648 fixed a race condition in computing the host analyzer references, but introduced a leak in _hostAnalyzerStateMap, wherein each new solution opened ends up creating a new entry in this map, even though the underlying set of host analyzer references doesn't change. This is due to the fact that each new SolutionState instance creates a new instance of HostDiagnosticAnalyzers, whose constructor creates a new map from analyzer references ID to the analyzer reference. This map is used as part of the key for _hostAnalyzerStateMap, leading to the leak. This change fixes the leak by using the underlying list of host analyzer references as part of the key for _hostAnalyzerStateMap, which is reference equals across all solution instances opened in the VS session. Verified manually that after this fix, repeatedly opening and closing a solution in VS no longer leads to unbounded growth of _hostAnalyzerStateMap.
We recently moved the HostDiagnosticAnalyzers from workspace into solution state. However, the IDE diagnostic incremental analyzer still stores a
_hostAnalyzerStateMapkeyed only by the language name, which was based on the assumption thatHostDiagnosticAnalyzersis a workspace level singleton. This leads to a race condition where in if the first instance ofHostDiagnosticAnalyzerson the solution state doesn't include all built-in VS analyzer references, these analyzers will never be enabled in VS. With this change, we now include theHostDiagnosticAnalyzersfrom the solution state as part of the key for_hostAnalyzerStateMap, avoiding this race.I noticed this while running some integration tests (compiler diagnostics go missing as compiler diagnostic analyzer is not identified). Additionally, we also seem to have few VS feedback tickets where compiler diagnostics are missing through the editing session, and this is a likely cause: AB#1431611 and AB#1430922.
We are considering taking this fix for servicing releases for Dev17.0 and Dev16.11 (needs approval).