Fix race condition in analyzer driver where multiple threads attempti… #25496

mavasani · 2018-03-15T15:53:32Z

…ng to analyze the same declaration do not race on freeing the pooled declaration analysis data instance, causing the other threads to unexpectedly fault.

We now pool only the declaration analysis data builder, and the declaration analysis data instance exposed to the driver is immutable, hence avoiding this race.

Fixes VSO 575529.

Ask Mode template

Customer scenario

This is race condition in the core analyzer driver where:

Multiple threads start attempting to analyze a particular source declaration
Thread 1 computes, caches and holds onto an instance of declaration analysis data needed to trigger analyzer callbacks, but gets context switched before it actually makes the analyzer callbacks.
Thread 2 gets this declaration analysis data instance from the cache and executes analysis callbacks.
Thread 2 executes cleanup, and if all analyzers have finished getting callbacks for the declaration, frees the declaration analysis data instance.
Thread 1 gets context and tries to operate on the freed declaration analysis data instance and faults.

All the watson dumps show that this race is most likely to happen when we are analyzing large method bodies and attempting to compute GetOperation on the Thread 1 at step 2. above, which is likely to cause a context switch.

Bugs this fixes

VSO 575529

Workarounds, if any

None

Risk

Medium to High - this fix touches one of the core parts of the analyzer driver that analyzes each symbol declaration and makes syntax node, code block and operation analyzer callbacks for nodes and executable code within declarations. Fix will address the NRE from the Watson, but given the part of code that is being changed, we can potentially introduce other functional regressions.

Performance impact

Low or Medium - As mentioned above, we are touching core parts of the analyzer driver, so we should keep an eye out for any performance regressions.

Is this a regression from a previous update?

The race has existed since the initial analyzer driver implementation, where we tuned the driver to cache and/or pool more data for IDE scenarios. The dumps indicate that the race is likely if the time for computation of entities on which to make analyzer callbacks is larger, which seems to be the case for operation analyzers executing on large method bodies.

Root cause analysis

Race condition that was never seen before or did not have enough watson hits.

How was the bug found?

Watson hits.

Test documentation updated?

N/A

…ng to analyze the same declaration do not race on freeing the pooled declaration analysis data instance, causing the other threads to unexpectedly fault. We now pool only the declaration analysis data builder, and the declaration analysis data instance exposed the driver is immutable, hence avoiding this race. Fixes VSO 575529.

mavasani · 2018-03-15T18:24:48Z

@dotnet/roslyn-analysis @dotnet/roslyn-compiler please review.

heejaechang · 2018-03-15T18:42:17Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.CompilationData.cs

@@ -21,14 +21,14 @@ internal class CompilationData
            private readonly Dictionary<SyntaxTree, SemanticModel> _semanticModelsMap;

            private readonly Dictionary<SyntaxReference, DeclarationAnalysisData> _declarationAnalysisDataMap;


is _declarationAnalysisDataMap pure cache or state?

looks like a cache
http://source.roslyn.io/#Microsoft.CodeAnalysis/DiagnosticAnalyzer/AnalyzerDriver.CompilationData.cs,f5ebdda4890c1cd7,references

Its just cache, it is fine to recompute the data if it does not exist but computation was showing up in FSA traces, so we added this cache. This was done before OOP, so in future we may want to experiment removing the cache completely and see if it is indeed an issue anymore.

heejaechang · 2018-03-15T18:44:22Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.CompilationData.cs

@@ -102,54 +119,8 @@ internal void ClearDeclarationAnalysisData(SyntaxReference declaration)
                    }

                    _declarationAnalysisDataMap.Remove(declaration);


I think now you can just do
_declarationAnalysisDataMap.Remove(declaration);

without TryGetValue since you don't need data anymore

heejaechang · 2018-03-15T18:45:07Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.CompilationData.cs

@@ -21,14 +21,14 @@ internal class CompilationData
            private readonly Dictionary<SyntaxTree, SemanticModel> _semanticModelsMap;

            private readonly Dictionary<SyntaxReference, DeclarationAnalysisData> _declarationAnalysisDataMap;
-            private readonly ObjectPool<DeclarationAnalysisData> _declarationAnalysisDataPool;
+            private readonly ObjectPool<DeclarationAnalysisDataBuilder> _declarationAnalysisDataBuilderPool;


not sure whether this pool is needed...

This was added before OOP for perf tuning FSA scenarios, when FSA was on by default. We can either retain this pool for now and remove in a separate PR OR do it in this PR itself. I thought it is better to separate perf work in separate PR.

heejaechang · 2018-03-15T18:53:02Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.CompilationData.cs


-                data = computeDeclarationAnalysisData(_declarationAnalysisDataPool.Allocate);
+                    dataBuilder = computeDeclarationAnalysisData(_declarationAnalysisDataBuilderPool.Allocate);


so, originally pool was there to reduce DeclarationAnalysisData type (and Lists in it). but now we always create those (including immutable arrays)

this now introduces new type builder which is pooled (so its builders as well) but all these are new types. so, pooling this doesnt include new allocations but also doesnt reduce any.

so, at the end, it is same as not having pool at all since we always create DeclarationAnalysisData (and its array).

if we are going this approach, I think we should just remove the pool.

by the way, the pool is not static. and I think we know doesnt share same analyzer driver between compilation. so do we actually need this clear declaration analysis data anymore?

I would rather make sure AnalyzerDriver state is not shared between compilation with analyzer in host and deprecate ClearCacheXXX methods.

Sure, let me get rid of this pool as well and make it cleaner.

CyrusNajmabadi · 2018-03-15T19:12:33Z

src/Compilers/CSharp/CSharpAnalyzerDriver/CSharpDeclarationComputer.cs

@@ -14,14 +14,14 @@ namespace Microsoft.CodeAnalysis.CSharp
 {
    internal class CSharpDeclarationComputer : DeclarationComputer
    {
-        public static void ComputeDeclarationsInSpan(SemanticModel model, TextSpan span, bool getSymbol, List<DeclarationInfo> builder, CancellationToken cancellationToken)
+        public static void ComputeDeclarationsInSpan(SemanticModel model, TextSpan span, bool getSymbol, ImmutableArray<DeclarationInfo>.Builder builder, CancellationToken cancellationToken)


please wrap long lines. On github this was not at all clear that this was a Builder and not an ImmutableArray.

use split view. it will show long line.

i would prefer our code be usable on github in either view :)

well. not sure when there is a way to see it on GitHub without too much trouble. that feels like something github should fix not code to change to fit into github.

without too much trouble.

it is a lot of trouble. because now you have to use split view instead of the unified view.

it also affects anyone who wants to ever look at teh code on github outside of diff-view as it will scroll off the right.

that feels like something github should fix not code to change to fit into github.

I disagree. We're an open source project that uses github extensively as the medium for everyone to collaborate. We should be cognizant of that and should try to be good citizens here. If the tooling ever changes, we can recalibrate. But we should not expect the rest of the ecosystem to bend to us. We should be considerate and be good to the rest of hte ecosystem. That's just good project stewardship.

I believe people who use bigger monitor is larger than number of people who use smaller monitor.

It's not the size of the monitor. It's the resolution. And even recent telemetry data (like 6-9 months old) showed the vast majority of devs were 1080p or less**. I'm currently running 1080p, and i cannot see these lines.

--

Other hardware surveys (like steam's) also show the vast majority of users are around this as well. But MS has this info already and knows that above 1080p is actually the large minority here.

@dotnet/roslyn-ide @dotnet/roslyn-analysis any opinion? if all think same, I can create simple analyzer that will issue warning if any line is longer than 130 characters. so that people can find that one as they use code.

fixer probably not easy since how to wrap is different per case by case. but we can identify all those cases and make the change once for all. and don't think about it again.

@CyrusNajmabadi point taken , @heejaechang can you pls file a separate issue for this and we can priorities it.

if all think same, I can create simple analyzer that will issue warning if any line is longer than 130 characters

I would have a passionate objection to this. I even blocked attempts to add it as a configurable rule to StyleCop Analyzers. I have the Editor Guidelines extension installed with a guideline at 120 characters, but I only use it for wrapping comments.

@sharwell not even as a suggestion? I agree warning is too aggressive but having a subtle suggestion at the character after the column limit doesn't seem like a bad idea. what is your objection to that?

CyrusNajmabadi · 2018-03-15T19:13:51Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.CompilationData.cs

                bool cacheAnalysisData)
            {
-                if (!cacheAnalysisData)
+                DeclarationAnalysisDataBuilder dataBuilder = null;


this is a super strange pattern for pooling and retrieving objects. it seems very error prone.

CyrusNajmabadi · 2018-03-15T19:14:26Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

@@ -684,7 +684,7 @@ private ImmutableHashSet<ISymbol> ComputeGeneratedCodeSymbolsInTree(SyntaxTree t
            var model = compilation.GetSemanticModel(tree);
            var root = tree.GetRoot(cancellationToken);
            var span = root.FullSpan;
-            var builder = new List<DeclarationInfo>();
+            var builder = ImmutableArray.CreateBuilder<DeclarationInfo>();


why not ArrayBuilder?

CyrusNajmabadi · 2018-03-15T19:29:50Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.CompilationData.cs


-                    DeclarationAnalysisData data;
-                    lock (_declarationAnalysisDataMap)
+                DeclarationAnalysisData data;


why have this local at all. just have something like "out var cachedData... return cachedData". then, lower, do "var data = ...". It seems super weird to want to share this same slot.

CyrusNajmabadi · 2018-03-15T19:29:57Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.CompilationData.cs

-                    lock (_declarationAnalysisDataMap)
+                lock (_declarationAnalysisDataMap)
+                {
+                    if (!_declarationAnalysisDataMap.TryGetValue(declaration, out DeclarationAnalysisData existingData))


You do a tryGet just so you can know to add. That's two lookups. Why not just put the value in the map unilaterally?

CyrusNajmabadi · 2018-03-15T19:31:26Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

-            DeclarationAnalysisDataBuilder declarationAnalysisData = allocateData();
-            var builder = declarationAnalysisData.DeclarationsInNode;
-
+            var builder = ImmutableArray.CreateBuilder<DeclarationInfo>();


ahy not ArrayBuilder?

CyrusNajmabadi · 2018-03-15T19:32:42Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.DeclarationAnalysisData.cs

-                IsPartialAnalysis = builder.IsPartialAnalysis;
+                DeclaringReferenceSyntax = declaringReferenceSyntax;
+                TopmostNodeForAnalysis = topmostNodeForAnalysis;
+                DeclarationsInNode = declarationsInNodeBuilder.ToImmutable();


this causes GC pressure as Builder becomes garbage that the GC has to reclaim.

CyrusNajmabadi · 2018-03-15T19:33:18Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.DeclarationAnalysisData.cs

+                SyntaxNode declaringReferenceSyntax,
+                SyntaxNode topmostNodeForAnalysis,
+                ImmutableArray<DeclarationInfo>.Builder declarationsInNodeBuilder,
+                IEnumerable<SyntaxNode> descendantNodesToAnalyze,


can we make descendantNodesToAnalyze an ArrayBuilder as well to reduce the allocations here.

we can use ArrayBuilder for all Builder usage and use ToImmutableAndFree to pool builder. or you can move back to List and use IReadOnlyList

mavasani · 2018-03-15T20:33:41Z

@heejaechang @CyrusNajmabadi Addressed feedback.

CyrusNajmabadi · 2018-03-15T21:02:30Z

src/Compilers/CSharp/CSharpAnalyzerDriver/CSharpDeclarationComputer.cs

+            TextSpan span,
+            bool getSymbol,
+            ArrayBuilder<DeclarationInfo> builder,
+            CancellationToken cancellationToken)


sharwell · 2018-03-16T19:26:21Z

@mavasani My comment in AnalyzerDriver.CompilationData.cs is the blocking comment

mavasani · 2018-03-16T19:38:07Z

@sharwell addressed.

jinujoseph · 2018-03-16T23:28:30Z

@MattGertz for ask mode approval (cc @Pilchie )

jcouv

I'll look at the compiler changes before Monday.

AlekseyTs · 2018-03-17T02:20:19Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalysisState.cs

            model.ComputeDeclarationsInSpan(fullSpan, getSymbol: true, builder: declarationInfos, cancellationToken: cancellationToken);
-            return declarationInfos.Select(declInfo => declInfo.DeclaredSymbol).Distinct().WhereNotNull();
+            return declarationInfos.ToArrayAndFree().Select(declInfo => declInfo.DeclaredSymbol).Distinct().WhereNotNull();


Select(declInfo => declInfo.DeclaredSymbol).Distinct().WhereNotNull() [](start = 53, length = 69)

Is this an intent to keep all these operations (select, distinct, where) lazy and repeat them for each individual enumeration through the result? If not, then consider saving the result ToArray rather than saving the source into an array, and query of off the builder directly. #Closed

AlekseyTs · 2018-03-17T02:24:42Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.CompilationData.cs

                lock (_declarationAnalysisDataMap)
                {
-                    if (_declarationAnalysisDataMap.TryGetValue(declaration, out data))
+                    if (_declarationAnalysisDataMap.TryGetValue(declaration, out var cachedData))


var [](start = 81, length = 3)

Please spell out the type. #Closed

@AlekseyTs If you are planning to review this item in pull requests, please modify this line (to set the severity to warning or error):

roslyn/src/Compilers/.editorconfig

Line 6 in 6ac2c27

csharp_style_var_elsewhere = false:none

The current statement is this is at the discretion of the implementer, which does not align with the code reviews from the compiler team.

Changing editorconfig is not a pre-req for commenting on pull requests. The compiler team prefers that var be avoided in cases where the type is not obvious. This policy has been in place since the beginning of Roslyn. Shouldn't have to re-litigate this on every PR.

I think sam's point is that this problem is avoidable if you change that. Namely, that people will see the problem ahead of time pre-pr, and thus can address it. It will mean that during the PR this conversation doesn't need to be repeated over and over again.

in a sense, this is precisely not re-litigating. It's allowing the compiler to emphatically say once and for all "that var be avoided in cases where the type is not obvious", and have that be respected so that time isn't wasted on this for compiler PRs.

I will repeat my statement:

Changing editorconfig is not a pre-req for commenting on pull requests.

I have not contradicted that. Nor do i think that sam was saying that. :) The point was that if you do change this, it will actually make things smoother (both for the compiler and people making PRs) by eliminating the need for these comments entirely.

There was no claim that this was a pre-req. Just that this sort of change would help free up time and remove this sort of noise from PRs, freeing up people to focus on more critical things.

Sam specifically said that.

If you are planning to review this item in pull requests, please modify this line

Yes. That sounds like a very pleasant suggestion on a way to alleviate the cost of having to review for this in future PRs. It's not at all a pre-req. Just something to consider as a way so that this becomes a non-issue in the future. You are, of course, free to not do that (and i don't think anyone would say otherwise). But, that just leaves us in the current position where compiler devs have to exercise mental cycles in reviews repeatedly looking for this and asking for it to be addressed, instead of having it be taken care of prior to the code ever even making it to the PR stage.

Again, if that's what you would prefer that's totally fine. No one is saying you have to take this. But, to me, it seems like a great idea simply because it both aids those making the PR (since they will know and follow the compiler rules), and it assists those reviewing things (since they don't have to spend cycles on this topic).

Changing editorconfig is not a pre-req for commenting on pull requests.

I'm not saying it is a prerequisite. I'm saying it will reduce noise in the pull request and will be less of a shock for users who:

Work elsewhere in dotnet/roslyn

Become used to the UI alerting them to use var everywhere

Write code in the compiler layer using var, and find no UI indication that anything is wrong

Submit review, blows up

AlekseyTs · 2018-03-17T02:26:21Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.CompilationData.cs

                    }
                }

-                data = computeDeclarationAnalysisData(_declarationAnalysisDataPool.Allocate);
+                var data = computeDeclarationAnalysisData();


var [](start = 16, length = 3)

Please spell out the type. #Closed

AlekseyTs · 2018-03-17T02:30:20Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

            model.ComputeDeclarationsInSpan(span, getSymbol: true, builder: builder, cancellationToken: cancellationToken);
+            var declarationInfos = builder.ToImmutableAndFree();


var declarationInfos = builder.ToImmutableAndFree(); [](start = 12, length = 52)

Why do this allocation, can we foreach over the builder? #Closed

AlekseyTs · 2018-03-17T02:32:45Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

-            var declarationAnalysisData = allocateData();
-            var builder = declarationAnalysisData.DeclarationsInNode;
-
+            var builder = ArrayBuilder<DeclarationInfo>.GetInstance();
            var declaringReferenceSyntax = declaration.GetSyntax(cancellationToken);


var [](start = 12, length = 3)

Please spell out the type. #Closed

AlekseyTs · 2018-03-17T02:32:56Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

-            var declarationAnalysisData = allocateData();
-            var builder = declarationAnalysisData.DeclarationsInNode;
-
+            var builder = ArrayBuilder<DeclarationInfo>.GetInstance();
            var declaringReferenceSyntax = declaration.GetSyntax(cancellationToken);
            var topmostNodeForAnalysis = semanticModel.GetTopmostNodeForDiagnosticAnalysis(symbol, declaringReferenceSyntax);


var [](start = 12, length = 3)

Please spell out the type. #Closed

AlekseyTs · 2018-03-17T02:33:06Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

            var declaringReferenceSyntax = declaration.GetSyntax(cancellationToken);
            var topmostNodeForAnalysis = semanticModel.GetTopmostNodeForDiagnosticAnalysis(symbol, declaringReferenceSyntax);
            ComputeDeclarationsInNode(semanticModel, symbol, declaringReferenceSyntax, topmostNodeForAnalysis, builder, cancellationToken);
+            var declarationInfos = builder.ToImmutableAndFree();


var [](start = 12, length = 3)

Please spell out the type. #Closed

AlekseyTs · 2018-03-17T02:33:19Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

            var declaringReferenceSyntax = declaration.GetSyntax(cancellationToken);
            var topmostNodeForAnalysis = semanticModel.GetTopmostNodeForDiagnosticAnalysis(symbol, declaringReferenceSyntax);
            ComputeDeclarationsInNode(semanticModel, symbol, declaringReferenceSyntax, topmostNodeForAnalysis, builder, cancellationToken);
+            var declarationInfos = builder.ToImmutableAndFree();

            var isPartialDeclAnalysis = analysisScope.FilterSpanOpt.HasValue && !analysisScope.ContainsSpan(topmostNodeForAnalysis.FullSpan);


var [](start = 12, length = 3)

Please spell out the type. #Closed

AlekseyTs · 2018-03-17T02:33:31Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

-            declarationAnalysisData.IsPartialAnalysis = isPartialDeclAnalysis;
-
-            return declarationAnalysisData;
+            var nodesToAnalyze = GetSyntaxNodesToAnalyze(topmostNodeForAnalysis, symbol, declarationInfos, analysisScope, isPartialDeclAnalysis, semanticModel, analyzerExecutor);


var [](start = 12, length = 3)

Please spell out the type. #Closed

AlekseyTs · 2018-03-17T02:37:29Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

-                GetSyntaxNodesToAnalyze(declaredNode, descendantDeclsToSkip);
-
-            if (isPartialDeclAnalysis)
+            bool ShouldAddNode(SyntaxNode node) => descendantDeclsToSkipOpt == null || !descendantDeclsToSkipOpt.Contains(node);


ShouldAddNode [](start = 17, length = 13)

Names of local functions should start with small letter. #Closed

AlekseyTs · 2018-03-17T02:47:06Z

Done review pass of Compilers code (iteration 6) #Closed

mavasani · 2018-03-19T15:20:13Z

@dotnet-bot retest windows_release_unit32_prtest please

mavasani · 2018-03-19T15:49:20Z

@AlekseyTs Addressed feedback, please review.

AlekseyTs · 2018-03-19T17:32:00Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalysisState.cs

-            return declarationInfos.Select(declInfo => declInfo.DeclaredSymbol).Distinct().WhereNotNull();
+            var declarationInfoBuilder = ArrayBuilder<DeclarationInfo>.GetInstance();
+            model.ComputeDeclarationsInSpan(fullSpan, getSymbol: true, builder: declarationInfoBuilder, cancellationToken: cancellationToken);
+            ImmutableArray<ISymbol> result = declarationInfoBuilder.Select(declInfo => declInfo.DeclaredSymbol).Distinct().WhereNotNull().ToImmutableArray();


WhereNotNull() [](start = 123, length = 14)

Not blocking: It might make sense to move filtering before the Distinct.

Sure, I will address it next time this is being touched.

AlekseyTs

LGTM (iteration 8)

mavasani added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 15, 2018

mavasani requested a review from jinujoseph March 15, 2018 15:53

mavasani requested review from a team as code owners March 15, 2018 15:53

mavasani force-pushed the RaceConditionFix branch from 8aeeaaf to 49c4382 Compare March 15, 2018 16:10

mavasani changed the base branch from dev15.6.x to dev15.7.x March 15, 2018 18:02

mavasani force-pushed the RaceConditionFix branch from 49c4382 to 0b6152b Compare March 15, 2018 18:04

mavasani added Area-Analyzers Area-Compilers and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Mar 15, 2018

heejaechang reviewed Mar 15, 2018

View reviewed changes

CyrusNajmabadi reviewed Mar 15, 2018

View reviewed changes

Address PR feedback and remove pooling

4755064

CyrusNajmabadi reviewed Mar 15, 2018

View reviewed changes

Review feedback

c77f475

333fred approved these changes Mar 15, 2018

View reviewed changes

CyrusNajmabadi reviewed Mar 15, 2018

View reviewed changes

Some more refactoring

f7a7565

sharwell approved these changes Mar 16, 2018

View reviewed changes

jcouv self-requested a review March 16, 2018 23:46

jcouv reviewed Mar 16, 2018

View reviewed changes

MattGertz added the Approved to merge label Mar 17, 2018

AlekseyTs reviewed Mar 17, 2018

View reviewed changes

mavasani added 2 commits March 19, 2018 06:47

Address review feedback from Aleksey

8779fa9

Formatting

c4fda78

AlekseyTs reviewed Mar 19, 2018

View reviewed changes

AlekseyTs approved these changes Mar 19, 2018

View reviewed changes

mavasani merged commit d8e676b into dotnet:dev15.7.x Mar 19, 2018

mavasani deleted the RaceConditionFix branch March 19, 2018 18:15

This was referenced Jul 5, 2018

Crash due to uncaught exception inside Roslyn's AnalyzerDriver mono/monodevelop#4673

Closed

Crash in AnalyzerDriver #26411

Closed

		@@ -21,14 +21,14 @@ internal class CompilationData
		private readonly Dictionary<SyntaxTree, SemanticModel> _semanticModelsMap;

		private readonly Dictionary<SyntaxReference, DeclarationAnalysisData> _declarationAnalysisDataMap;

		@@ -102,54 +119,8 @@ internal void ClearDeclarationAnalysisData(SyntaxReference declaration)
		}

		_declarationAnalysisDataMap.Remove(declaration);


		data = computeDeclarationAnalysisData(_declarationAnalysisDataPool.Allocate);
		dataBuilder = computeDeclarationAnalysisData(_declarationAnalysisDataBuilderPool.Allocate);

		model.ComputeDeclarationsInSpan(span, getSymbol: true, builder: builder, cancellationToken: cancellationToken);
		var declarationInfos = builder.ToImmutableAndFree();

Fix race condition in analyzer driver where multiple threads attempti… #25496

Fix race condition in analyzer driver where multiple threads attempti… #25496

Conversation

mavasani commented Mar 15, 2018 • edited Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

mavasani commented Mar 15, 2018

heejaechang Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Mar 18, 2018 • edited Loading

Choose a reason for hiding this comment

Neme12 Mar 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented Mar 15, 2018

Choose a reason for hiding this comment

sharwell commented Mar 16, 2018

mavasani commented Mar 16, 2018

jinujoseph commented Mar 16, 2018

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

sharwell Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

sharwell Mar 18, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 17, 2018 • edited Loading

mavasani commented Mar 15, 2018 •

edited

Loading

heejaechang Mar 15, 2018 •

edited

Loading

heejaechang Mar 17, 2018 •

edited

Loading

sharwell Mar 18, 2018 •

edited

Loading

Neme12 Mar 18, 2018 •

edited

Loading

AlekseyTs Mar 17, 2018 •

edited

Loading

AlekseyTs Mar 17, 2018 •

edited

Loading

sharwell Mar 17, 2018 •

edited

Loading

CyrusNajmabadi Mar 17, 2018 •

edited

Loading

CyrusNajmabadi Mar 17, 2018 •

edited

Loading

sharwell Mar 18, 2018 •

edited

Loading

AlekseyTs Mar 17, 2018 •

edited

Loading

AlekseyTs Mar 17, 2018 •

edited

Loading

AlekseyTs Mar 17, 2018 •

edited

Loading

AlekseyTs Mar 17, 2018 •

edited

Loading

AlekseyTs Mar 17, 2018 •

edited

Loading

AlekseyTs Mar 17, 2018 •

edited

Loading

AlekseyTs Mar 17, 2018 •

edited

Loading

AlekseyTs Mar 17, 2018 •

edited

Loading

AlekseyTs commented Mar 17, 2018 •

edited

Loading