-
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 all commits
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()); | ||||
| } | ||||
| 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.
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?
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.
Seems like a reasonable approach.
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.
I'll do this in a follow up.