Fixed analyzer timeout issue. #1566
Conversation
|
I have built this from source, and pointed my VS Code at the .exe. This appears to have solved the FxCop timeout issues I was facing for what it's worth. |
|
This being the constructor, this makes me feel like we might need to configure some of those false values. public CompilationWithAnalyzersOptions (Microsoft.CodeAnalysis.Diagnostics.AnalyzerOptions options, Action<Exception,Microsoft.CodeAnalysis.Diagnostics.DiagnosticAnalyzer,Microsoft.CodeAnalysis.Diagnostic> onAnalyzerException, bool concurrentAnalysis, bool logAnalyzerExecutionTime, bool reportSuppressedDiagnostics);@savpek Do you have any thoughts here? @filipw also? |
| @@ -235,12 +235,12 @@ private async Task AnalyzeDocument(Project project, ImmutableArray<DiagnosticAna | |||
| } | |||
| else if (allAnalyzers.Any()) // Analyzers cannot be called with empty analyzer list. | |||
| { | |||
| var semanticDiagnosticsWithAnalyzers = await compiled | |||
| .WithAnalyzers(allAnalyzers, workspaceAnalyzerOptions) | |||
| var compilationWithAnalyzers = compiled.WithAnalyzers(allAnalyzers, new CompilationWithAnalyzersOptions(workspaceAnalyzerOptions, null, false, false, false)); | |||
There was a problem hiding this comment.
Sorry for delay
It could be good to add named parameters since its much easier to understand how analyzers are configured with them, like reportSuppressedDiagnostics: false instead of "false" 🤔
About error logging, personally i feel that it could be good idea to pass logger function instead of null. Situations what i am thinking are mainly cases like missing services from workspaces etc because there truly are differences between VS and omnisharp in certain cases + configurations may differ which can trigger error situations. That way instead of issue "missing diagnostic X" becomes "missing diagnostic X and heres these errors on logs". Trace could be good place to start as severity since its common to ask trace level logs on issue and then real error is catch up.
Add named parameters. Add analyzer error logging.
|
this is a great fix thanks a lot. I also pulled it locally and it indeed fixes #1552
|
| @@ -261,6 +265,14 @@ private async Task AnalyzeDocument(Project project, ImmutableArray<DiagnosticAna | |||
| } | |||
| } | |||
|
|
|||
| private void OnAnalyzerException(Exception ex, DiagnosticAnalyzer analyzer, Diagnostic diagnostic) | |||
| { | |||
| _logger.LogError($"Exception in diagnostic analyzer." + | |||
There was a problem hiding this comment.
as suggested by @savpek let's change it to lower level i.e. Debug.
Change log level.
…ature/AnalyzerTimeout
|
|
|
/azp run OmniSharp.omnisharp-roslyn |
|
No pipelines are associated with this pull request. |
|
it was a transient build issue |
| .WithAnalyzers(allAnalyzers, workspaceAnalyzerOptions) | ||
| var compilationWithAnalyzers = compiled.WithAnalyzers(allAnalyzers, new CompilationWithAnalyzersOptions(workspaceAnalyzerOptions, | ||
| onAnalyzerException: OnAnalyzerException, | ||
| concurrentAnalysis: false, |
There was a problem hiding this comment.
actually, this is the reason why it works now.
when we use the old code CompilationWithAnalyzersOptions gets initialized to the default values and by default it sets concurrentAnalysis: true. Looks like then something deadlocks internally or suffers from thread starvation and it never completes because of that. In fact I tested this locally and indeed flipping the flag back to concurrentAnalysis: true makes it not work again.
Ultimately I'd like us to be able to recognize why this is happening and how we can avoid that problem but short term this is OK to do it this way as with the current set up it doesn't work at all.
There was a problem hiding this comment.
actually I found an answer myself http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/Diagnostics/AnalyzerHelper.cs,365
looks like VS also switches off concurrency
// in IDE, we always set concurrentAnalysis == false otherwise, we can get into thread starvation due to
// async being used with syncronous blocking concurrency.
|
this should be good to go - I will merge it, thanks @kurobon-jp! 👏 |
Hello.
I think that the cause of this timeout issue is in GetAnalyzerSemanticDiagnosticsAsync.
This function may not return a response.
I checked the difference with AnalyzerRunner and made a correction.
And it seems like it worked.
I'm glad if this information helps a little.