Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix leaks in AnalyzerDriver introduced in #673 and refactor code in analyzer driver #767

Merged
merged 7 commits into from
Feb 22, 2015

Conversation

mavasani
Copy link
Contributor

My prior change #673 made to address issues with analyzer exception diagnostics introduced a few leaks in product and test code. The primary reason was that I attempted to use static events to track state as the existing AnalyzerDriverHelper type which did the core analyzer execution was a static type with all static methods. Additionally, I added a static event to the new host diagnostic update source added for reporting analyzer specific diagnostics. These were holding onto projects/workspaces in test runs causing leaks.

I have reverted the approach of static events and instead refactored the code in AnalyzerDriver project to simplify the whole design:

  1. Renamed AnalyzerDriverHelper to AnalyzerExecutor and made it a non-static type, which has instance fields for all the configuration parameters for analyzer callbacks.
  2. Move all the core analyzer callbacks (actions/Initialize method/supported diagnostics) into AnalyzerExecutor. Command line compiler just creates a single instance of the executor, while IDE driver creates instances per analyzer.
  3. Both these changes simplified the API a lot, and I just had to add an additional field "Action addExceptionDiagnostic" to AnalyzerExecutor to configure how to handle exception diagnostics.
  4. MEF import AbstractHostDiagnosticUpdateSource in DiagnosticAnalyzerService and thread it down to IDE analyzer driver.
  5. Changes 3 and 4 above meant that in the IDE driver, delegate "addExceptionDiagnostic" just asks the HostDiagnosticUpdateSource to report the exception diagnostic produced by the AnalyzerExecutor.

This change also re-enables the tests skipped by #761 and fixes #759.

Lessons learnt:

  1. Static events are evil, don't use them.
  2. @heejaechang is invariably right, never overlook his review feedback :)

@mavasani
Copy link
Contributor Author

Merging this in as there are 8 broken tests (couple still failing in builds) which this change will fix. I will address CR feedback subsequently.

mavasani added a commit that referenced this pull request Feb 22, 2015
My prior change #673 made to address issues with analyzer exception diagnostics introduced a few leaks in product and test code. The primary reason was that I attempted to use static events to track state as the existing AnalyzerDriverHelper type which did the core analyzer execution was a static type with all static methods. Additionally, I added a static event to the new host diagnostic update source added for reporting analyzer specific diagnostics. These were holding onto projects/workspaces in test runs causing leaks.

I have reverted the approach of static events and instead refactored the code in AnalyzerDriver project to simplify the whole design:
 1. Renamed AnalyzerDriverHelper to AnalyzerExecutor and made it a non-static type, which has instance fields for all the configuration parameters for analyzer callbacks.
 2. Move all the core analyzer callbacks (actions/Initialize method/supported diagnostics) into AnalyzerExecutor. Command line compiler just creates a single instance of the executor, while IDE driver creates instances per analyzer. 
 3. Both these changes simplified the API a lot, and I just had to add an additional field "Action addExceptionDiagnostic" to AnalyzerExecutor to configure how to handle exception diagnostics.
 4. MEF import AbstractHostDiagnosticUpdateSource in DiagnosticAnalyzerService and thread it down to IDE analyzer driver.
 5. Changes 3 and 4 above meant that in the IDE driver, delegate "addExceptionDiagnostic" just asks the HostDiagnosticUpdateSource to report the exception diagnostic produced by the AnalyzerExecutor.

This change also re-enables the tests skipped by #761 and fixes #759.
@mavasani mavasani merged commit a1e56a1 into dotnet:master Feb 22, 2015
@heejaechang
Copy link
Contributor

👍

by the way, is your previous refactoring could cause what to run for compilation end analyzer to be changed? I have this RPS rebuild test regression and I am fixing (or trying to fix it) at engine but still plan to figure out what caused the behavior change.

basically, I think we are running code only for compilation end actions even when there is none.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AnalyzerDriver leaks workspaces in tests
4 participants