Update plugin assembly loading for .NET 3/5

[rainersigwald]

Several improvements to #4916 are possible once we're moved to target .NET 3+:

• Name AssemblyLoadContexts
• Use AssemblyDependencyResolver to make native dependencies easier to handle (Load plugins in an AssemblyLoadContext #4916 (comment))
• Audit MSBuild's dependencies and consider handling Resolving rather than Load (https://github.com/microsoft/msbuild/pull/4916/files/6d4c3b1e2adcd6ce97233b85489c514b2b990949#r363710290, Load plugins in an AssemblyLoadContext #4916 (comment))

[rainersigwald]

Note: this was already mostly done in 90ac0c3. We could mostly pull back to that, but we'll need to keep the escape hatches and other behaviors of the current implementation.

[AArnott]

Please to take a look at the unique requirements proposed by #1755 which was recently duped against this one. The goal being that now that msbuild isn't a Windows, .NET Framework only process, we need a more scalable, simpler model for plugins to ship without shipping the full transitive closure of their own runtime dependencies for every single OS+runtime, while still being able to run on each of these.

[rainersigwald]

@AArnott are you saying there are MSBuild-specific requirements that are not covered by AssemblyDependencyResolver? None jumped out at me.

[AArnott]

@rainersigwald I'm not sure. I don't see anything in the AssemblyDependencyResolver doc to suggest that it can load a plugin's dependencies from the nuget package cache.

[mwpowellhtx]

@rainersigwald Are there examples how to plugin with this in view? Thank you...

[rainersigwald]

You might be interested in https://docs.microsoft.com/dotnet/core/tutorials/creating-app-with-plugin-support#plugin-with-library-dependencies, @mwpowellhtx.

[marcin-krystianc]

Hi @rainersigwald, I've recently come across a memory leak caused by the fact that assemblies are loaded into AssemblyLoadContext but they are never unloaded.
The memory footprint of loaded assemblies is small enough that this problem can be ignored most of the time. But for us it is a problem as we want to build a long running service which uses msbuild libraries to evaluate project files. Fortunately there is a workaround (#5098) so we are not blocked on this problem anymore, but it would be great if it was fixed.

According to my tests, it is actually quite an easy fix. It is enough to construct the AssemblyLoadContext with isCollectible = true (available in .NET 3+). When the ALC is collectible, it will automatically unload assemblies when they are not needed (https://github.com/dotnet/runtime/blob/84bad7e24589169975ec8a599743110d703c761b/src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs#L113-L125).

Documentation says that it is pretty safe, as the assembly will be unloaded only if it is no longer referenced/used (https://docs.microsoft.com/en-us/dotnet/standard/assembly/unloadability). I wonder whether you see any problems with making ALC collectible? Would you accept a PR for it?

[rainersigwald]

@marcin-krystianc We considered making the ALCs collectible, but that can harm performance in some cases--specifically, when we're reusing MSBuild nodes for a subsequent build, having the tasks loaded (and JITted) from a previous run can speed up the dev inner loop.

I'm a bit confused by the leak you're describing, though. Some questions:

1. Are you really only evaluating projects? That shouldn't require any tasks and shouldn't use any ALCs.
2. When you observe the leak, what is causing creation of new ALCs? There should be one per task assembly, so assuming you're not constantly changing task assemblies, I would expect extremely slow growth.
3. How does the workaround from Add an escape hatch for ALC isolation #5098 help? Without that, all the tasks should be loaded into the default ALC and not be collectible, so the growth characteristics should be roughly the same.

[marcin-krystianc]

Ad 1
Yes, to reproduce the problem it is enough to evaluate projects. You are right that project evaluation doesn't require creating new MsBuild tasks, but the leak is not related to tasks at all. There are SdkResolvers which need to be loaded for the project file evaluation (E.g.: Microsoft.Build.NuGetSdkResolver.dll, Microsoft.NET.Sdk.WorkloadMSBuildSdkResolver.dll).
Example call stack is:

AssemblyLoadContext.LoadFromAssemblyPath() 
CoreClrAssemblyLoader.LoadUsingPluginContext()
CoreClrAssemblyLoader.LoadFromPath()
SdkResolverLoader.LoadResolverAssembly()
SdkResolverLoader.LoadResolvers()
SdkResolverLoader.LoadResolvers()
SdkResolverService.Initialize()
SdkResolverService.ResolveSdk()
CachingSdkResolverService.<>n__0()
CachingSdkResolverService.<>c__DisplayClass3_0.<ResolveSdk>b__1()
Lazy<SdkResult>.ViaFactory()
Lazy<SdkResult>.ExecutionAndPublication()
Lazy<SdkResult>.CreateValue()
Lazy<__Canon>.get_Value()
CachingSdkResolverService.ResolveSdk()
Evaluator<ProjectProperty, ProjectItem, ProjectMetadata, ProjectItemDefinition>.ExpandAndLoadImportsFromUnescapedImportExpressionConditioned()
Evaluator<ProjectProperty, ProjectItem, ProjectMetadata, ProjectItemDefinition>.ExpandAndLoadImports()
Evaluator<ProjectProperty, ProjectItem, ProjectMetadata, ProjectItemDefinition>.EvaluateImportElement()
Evaluator<ProjectProperty, ProjectItem, ProjectMetadata, ProjectItemDefinition>.PerformDepthFirstPass()
Evaluator<ProjectProperty, ProjectItem, ProjectMetadata, ProjectItemDefinition>.Evaluate()
Evaluator<ProjectProperty, ProjectItem, ProjectMetadata, ProjectItemDefinition>.Evaluate()
Project.ProjectImpl.Reevaluate()
Project.ProjectImpl.ReevaluateIfNecessary()
Project.ProjectImpl.ReevaluateIfNecessary()
Project.ProjectImpl.ReevaluateIfNecessary()
Project.ProjectImpl.Initialize()
new Project()
Project.FromFile()
...

Ad 2
New ALC are created because SdkResolvers need to be loaded for each evaluated project.
There is a cache for them called CachingSdkResolverService (in the EvaluationContext) so they are loaded only once per each instance of that cache. If it was possible to use single EvaluationContext for the whole lifetime of our long running service that would fix the leak from ALCs, unfortunately it is not possible as it creates other memory leaks (e.g. from the EngineFileUtilities).

Here is simplified version of our code in which we create an EvaluationContext for each solution we process:

voiid ProcessSolution(string path)
{
	using (var projectCollection = new ProjectCollection())
	{
		var projectOptions = new ProjectOptions
		{
			ProjectCollection = projectCollection,
			LoadSettings = ProjectLoadSettings.IgnoreEmptyImports | ProjectLoadSettings.IgnoreInvalidImports |
						   ProjectLoadSettings.RecordDuplicateButNotCircularImports | ProjectLoadSettings.IgnoreMissingImports,
			EvaluationContext = EvaluationContext.Create(EvaluationContext.SharingPolicy.Shared),
		};
		
		var solutionFile = SolutionFile.Parse(path);
		var projects = solutionFile.ProjectsInOrder
			.Where(x => x.ProjectType == SolutionProjectType.KnownToBeMSBuildFormat)
			.Select(x => Project.FromFile(x.AbsolutePath, projectOptions))
			.ToList();
		...
	}
}

Ad 3
My understanding is that when the plugin ALCs are disabled then only the default ALC is used. With the single default ALC all subsequent load requests for already loaded assemblies basically do nothing. In case of SkdResolvers, their path is always the same (e.g. c:\Program Files\dotnet\sdk\5.0.302\SdkResolvers\...) so such assembly is loaded only once into memory and the memory leak is not observed anymore.

[rainersigwald]

@marcin-krystianc Ah, thank you for the details! That should be resolvable; I filed #6842.

[MichalPavlik]

@rainersigwald, is this issue still relevant? All linked PRs in original post were merged.

[rainersigwald]

I don't think we rigorously did

• Audit MSBuild's dependencies and consider handling Resolving rather than Load (https://github.com/microsoft/msbuild/pull/4916/files/6d4c3b1e2adcd6ce97233b85489c514b2b990949#r363710290, Load plugins in an AssemblyLoadContext #4916 (comment))

But yes, I think we can close.