Expose disposing capability on IAssemblyLoaderInternal #74845
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -11,6 +11,10 @@ | |||
| using System.Reflection; | |||
| using Roslyn.Utilities; | |||
|
|
|||
| #if NET | |||
| using System.Runtime.Loader; | |||
| #endif | |||
There was a problem hiding this comment.
technically needed since i have a <see cref="AssemblyLoadContext"/> below in teh comment. And that type (and namespace) is only available on NET not below.
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.Core.cs
Show resolved
Hide resolved
| _loadContextByDirectory.Clear(); | ||
| } | ||
|
|
||
| foreach (var context in contexts) | ||
| { | ||
| context.Unload(); |
There was a problem hiding this comment.
Note that this can throw if the AssemblyLoadContext cannot be unloaded. A misbehaving analyzer could cause this to happen by stashing references to itself somewhere you did not anticipate. That is probably somewhat rare but I also think it's going to be > 0.
This code path today doesn't really deal with exceptions because we only aggressively use it inside our unit tests. Unloading is important there because of the limitations of our test environments.
Think we should consider how we want the IDE to behave in this case and harden this code path a bit.
There was a problem hiding this comment.
yeah, worst case, unloading doesn't work, and we have some memory growth in teh case that analyzer references are changing a lot on disk. we can live with that.
| @@ -51,5 +58,8 @@ public Assembly LoadFromPath(string fullPath) | |||
|
|
|||
| return defaultLoader.LoadFromPath(fullPath); | |||
| } | |||
|
|
|||
| public void UnloadAll() | |||
| => defaultLoader.UnloadAll(); | |||
There was a problem hiding this comment.
When UnloadAll is called do we create a new instance of this type? Keeping the existing one around likely has some issues because it's caching items like AssemblyName that you want to be different when you're live reloading.
There was a problem hiding this comment.
When UnloadAll is called
We only call it in the finalizer of the type that owns this AnalyzerLoader. So nothing is holding onto it anymore.
do we create a new instance of this type?
Morally 'yes'. We will have thrown one away because we moved to a new one.
CyrusNajmabadi
changed the title
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.Core.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs
Outdated
Show resolved
Hide resolved
Extracted from #74780 to make that PR simpler and easier to follow.