Prevent duplicate invocations of per-compilation analyzer actions for…

[mavasani]

… a given symbol/syntax node in IDE.

User scenario: An analyzer author writes a compilation end analyzer, which registers symbol and/or syntax node actions to maintain some state about them, and a compilation end action to report diagnostics based on the final sate. When running in the IDE, the analyzer will get duplicate callbacks to the symbol/syntax node actions for the same symbol/node in the compilation: once while computing active document diagnostics and once while computing project diagnostics. This can cause one of the following issues:

1. Crash the analyzer if it is using some map keyed on the callback symbol/node, and doesn't handle duplicate callbacks.
2. Corrupt the analyzer state by recording the state twice for a single symbol/node.
3. Possibly cause the analyzer to enter a race or deadlock.

Fix Description: Fix is to ensure that when computing the compilation end diagnostics for such stateful analyzers, we use a new instance of the analyzer. All the compilation wide actions are then invoked on this new instance of the analyzer, avoiding duplicate callbacks into the same instance.

Fixes #248

Testing: Added a regression test + existing tests.

/cc @ManishJayaswal @srivatsn This is for 1.0 (stable).

[mavasani]

@JohnHamby @heejaechang @tmeschter @jmarolf can you please review?

[msJohnHamby]

You might want to move this assignment to be immediately after the assignment to "analyzer" within the try above, to avoid risk of something that might throw an exception creeping in between that assignment and this one. (If that would not be correct, because ClearAnalyzerState requires the two statements above to execute first, then I suggest a comment to the effect that this is the earliest correct point for this assignment.)

[mavasani]

You are right, I think it might be safer to just move the assignment ahead.

[heejaechang]

this might leak. since analyzer will hand out to outer system. assumption was analyzer either leave forever or all handed out analyzer will be cleaned up when reference is removed. which was okay since analyzer were shared.

now, newly created analyzer is handed out. for host analyzer, newly created analyzer might live very long time.

what is the reason we need to clone? analyzer itself is stateless right? the initialize method is the starting point of the state, why not just re-initialize those?

[heejaechang]

is this for the case where there might be a person who put host wide state? what happen if a user put things in static field, cloning own fix those?

[mavasani]

@heejaechang

1. Regarding the leak: Cloned analyzer instance is only handed to the ReanalyzeDiagnosticGetter, and the instance is only used for core analyzer callbacks into the AnalyzerExecutor, so the host analyzer manager never has access to the cloned instance. One place where the current fix can cause leak is for analyzer exception diagnostics, as they will be reported for the cloned instance. I will tweak the current fix so that the exception handler for this case reports exception diagnostics against the original analyzer instance, and behaves as if everything happened on the original instance, including cleaning up the diagnostics. I don't see any other leak possibility, do you know of other caches where this code path can leak?

2. Regarding the reason for fix: We cannot assume anything about analyzer being stateless, i.e.

   1. We cannot make duplicate callbacks to Initialize on same analyzer instance and
   2. We cannot make duplicate callbacks to RegisterCompilationStartAction for same compilation and
   3. We cannot make duplicate callbacks for same symbol or node or code block from a given compilation to an analyzer.

   All of these would violate our analyzer execution semantics, and this change fixes the case where IDE driver violates this. A simple compilation analyzer that say has a map from declared symbol to some value (not on analyzer field, but on the scope of compilation start action), and adds a kvp on this map on every symbol action callback will cause it to crash due to this issue. Such an analyzer is completely correct and abides by our guaranteed execution semantics, so this change aims at avoiding such a scenario.

[mavasani]

@JohnHamby @srivatsn can confirm if the analyzer semantics noted in #(2) above are indeed true, and if so, whether or not they should be altered at this point. I don't know if an analyzer would expect/handle being called repeatedly to Initialize.
Probably this doc comment and this document's first paragraph for Ordering of actions section needs more clarity.

If we feel its ok to invoke Initialize multiple times on the same analyzer instance, then the fix for this issue would be a simple one line fix here: Just clear all existing analyzer state on AnalyzerManager at this point and rest should just work by re-invoking Initialize/CompilationStartActions when asked for analyzer actions.

[msJohnHamby]

We have specified that Initialize will be called no more than once per analyzer instance. It's hard to predict what might break as a consequence of violating that. One might imagine an analyzer that acquires some resource in Initialize and frees it in a finalizer. Calling Initialize more than once for an instance of such an analyzer could leak the resource.

[heejaechang]

Such resource should be acquired in constructor.

•      heejae
  

From: John Hamby [mailto:[email protected]]
Sent: Thursday, May 28, 2015 12:38 PM
To: dotnet/roslyn
Cc: HeeJae Chang
Subject: Re: [roslyn] Prevent duplicate invocations of per-compilation analyzer actions for… (#3105)

In src/Features/Core/Diagnostics/EngineV1/DiagnosticAnalyzerDriver.cshttps://github.com//pull/3105#discussion_r31269579:

•                    // Doing so on the original analyzer instance might cause duplicate callbacks into the analyzer.
  

•                    // So we create a new instance of this analyzer and compute compilation diagnostics on that instance.
  

•                    try
  

•                    {
  

•                        analyzer = Activator.CreateInstance(analyzer.GetType()) as DiagnosticAnalyzer;
  

•                    }
  

•                    catch
  

•                    {
  

•                        // Unable to created a new analyzer instance, bail out on reporting diagnostics.
  

•                        return;
  

•                    }
  

•                    analyzerExecutor = GetAnalyzerExecutor(analyzer, compilation, localDiagnostics.Add);
  

•                    analyzerActions = await this.GetAnalyzerActionsAsync(analyzer, analyzerExecutor).ConfigureAwait(false);
  

•                    analyzerToDispose = analyzer;
  

We have specified that Initialize will be called no more than once per analyzer instance. It's hard to predict what might break as a consequence of violating that. One might imagine an analyzer that acquires some resource in Initialize and frees it in a finalizer. Calling Initialize more than once for an instance of such an analyzer could leak the resource.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3105/files#r31269579.

[msJohnHamby]

For analyzers in particular, acquiring an expensive resource might best be done in Initialize rather than in a constructor, because the instance might be constructed only to determine that the analyzer will not be used (due to all of its diagnostics being suppressed).

The point I was trying to make is that things might go wrong for some analyzers if we don't obey the specified contract. Debating whether a particular way things might go wrong could be avoided by changing the design of an analyzer is not going to resolve the issue.

[mavasani]

I agree with @JohnHamby on this. Changing the semantics at this stage might break existing analyzers, who rely on our current semantics.

Btw, I am seeing this issue very often for a new statful code block analyzer that I am trying to write for #2949. I think we ought to fix this issue for 1.0, regardless of whether we clone analyzer or change Initialize semantics.

[msJohnHamby]

I sign off. This looks to me as if it solves the issue, and I do not see correctness risks. The fix does execute some actions twice (with separate state), and so there are arbitrary space and time implications depending on the behavior of an analyzer. The issue of more-than-strictly-necessary action execution is by itself sufficient to warrant another serious look at overhauling the incremental analyzer for V2, but this is a good change, and probably the best available, for V1.

[mavasani]

Thanks John. I agree with your analysis that this can be fixed in a better and correct way with v2, but this is probably the best, low risk fix for current engine.

[mavasani]

Moving this back to shiproom as John/Heejae are fine with the proposed change.

[MattGertz]

Approved 5/29 by ML Shiproom.

[msJohnHamby]

Dozens of thanks, Manish!!