-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Do not use collectible AssemblyLoadContexts in AnalyzerAssemblyLoader. #79990
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -165,29 +165,10 @@ internal DirectoryLoadContext[] GetDirectoryLoadContextsSnapshot() | |||
|
|
||||
| private partial void DisposeWorker() | ||||
| { | ||||
| var contexts = ArrayBuilder<DirectoryLoadContext>.GetInstance(); | ||||
| lock (_guard) | ||||
| { | ||||
| foreach (var (_, context) in _loadContextByDirectory) | ||||
| contexts.Add(context); | ||||
|
|
||||
| _loadContextByDirectory.Clear(); | ||||
| } | ||||
|
|
||||
| foreach (var context in contexts) | ||||
| { | ||||
| try | ||||
| { | ||||
| context.Unload(); | ||||
| CodeAnalysisEventSource.Log.DisposeAssemblyLoadContext(context.Directory, context.ToString()); | ||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I waffled on whether to remove these EventSource methods. I see that they are numbered and didn't want to throw off any telemetry. Would marking them as obsolete be the way to go?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like a reasonable approach.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll do this in a follow up. |
||||
| } | ||||
| catch (Exception ex) when (FatalError.ReportAndCatch(ex, ErrorSeverity.Critical)) | ||||
| { | ||||
| CodeAnalysisEventSource.Log.DisposeAssemblyLoadContextException(context.Directory, ex.ToString(), context.ToString()); | ||||
| } | ||||
| } | ||||
|
|
||||
| contexts.Free(); | ||||
| } | ||||
|
|
||||
| internal sealed class DirectoryLoadContext : AssemblyLoadContext | ||||
|
|
@@ -196,7 +177,7 @@ internal sealed class DirectoryLoadContext : AssemblyLoadContext | |||
| private readonly AnalyzerAssemblyLoader _loader; | ||||
|
|
||||
| public DirectoryLoadContext(string directory, AnalyzerAssemblyLoader loader) | ||||
| : base(isCollectible: true) | ||||
| : base(isCollectible: false) | ||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @CyrusNajmabadi I saw that Extensions use the assembly loader. Will this break those scenarios?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The extension assembly support is supposed to be able to hot unload extensions. We definitely are calling it here here -
In this, are the assemblies still getting unloaded, but we're not disposing of the ALC? Or are both the assemblies and ALC staying around?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Going to take this as is but will follow up with a PR making
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah. we should make this configurable.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened #79997 |
||||
| { | ||||
| Directory = directory; | ||||
| _loader = loader; | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a blurb that this R2R is critical to our VS / CLI perfromance scenarios. Basically a place for some future dev to look for context if they ever want to change this.