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.

Sign up for GitHub

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

Improve support for loading MSBuild in a non-default ALC. #11111

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Improve support for loading MSBuild in a non-default ALC. #11111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8b83068
Replace mentions to `AssemblyLoadContext.Default` with whatever ALC t…
teo-tsirpanis Dec 8, 2024
888a0a5
Support unloading plugin ALCs.
teo-tsirpanis Dec 8, 2024
2b52221
Simplify and inline `TryGetWellKnownAssembly`.
teo-tsirpanis Dec 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 6 additions & 25 deletions src/Shared/CoreCLRAssemblyLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ internal sealed class CoreClrAssemblyLoader

private bool _resolvingHandlerHookedUp = false;

private static readonly Version _currentAssemblyVersion = new Version(Microsoft.Build.Shared.MSBuildConstants.CurrentAssemblyVersion);

public void AddDependencyLocation(string fullPath)
{
if (fullPath == null)
Expand Down Expand Up @@ -71,7 +69,7 @@ private Assembly LoadUsingLegacyDefaultContext(string fullPath)
{
if (!_resolvingHandlerHookedUp)
{
AssemblyLoadContext.Default.Resolving += TryResolveAssembly;
MSBuildLoadContext.ThisAssemblyLoadContext.Resolving += TryResolveAssembly;
_resolvingHandlerHookedUp = true;
}

Expand All @@ -81,7 +79,7 @@ private Assembly LoadUsingLegacyDefaultContext(string fullPath)
return assembly;
}

return LoadAndCache(AssemblyLoadContext.Default, fullPath);
return LoadAndCache(MSBuildLoadContext.ThisAssemblyLoadContext, fullPath);
}
}

Expand All @@ -108,33 +106,16 @@ private Assembly LoadUsingPluginContext(string fullPath)
}
}

private Assembly TryGetWellKnownAssembly(AssemblyLoadContext context, AssemblyName assemblyName)
{
if (!MSBuildLoadContext.WellKnownAssemblyNames.Contains(assemblyName.Name))
{
return null;
}

// Ensure we are attempting to load a matching version
// of the Microsoft.Build.* assembly.
assemblyName.Version = _currentAssemblyVersion;

string[] searchPaths = [Assembly.GetExecutingAssembly().Location];
return TryResolveAssemblyFromPaths(context, assemblyName, searchPaths);
}

private Assembly TryResolveAssembly(AssemblyLoadContext context, AssemblyName assemblyName)
{
lock (_guard)
{
Assembly assembly = TryGetWellKnownAssembly(context, assemblyName);

if (assembly != null)
if (MSBuildLoadContext.WellKnownAssemblyNames.Contains(assemblyName.Name))
{
return assembly;
return MSBuildLoadContext.ThisAssemblyLoadContext.LoadFromAssemblyName(assemblyName);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this achieves the same goal in a much more straightforward way. And since we load a specific name in a specific context we don't need to patch the version.

}

if (_namesToAssemblies.TryGetValue(assemblyName.FullName, out assembly))
if (_namesToAssemblies.TryGetValue(assemblyName.FullName, out Assembly assembly))
{
return assembly;
}
Expand Down Expand Up @@ -180,7 +161,7 @@ private Assembly TryResolveAssemblyFromPaths(AssemblyLoadContext context, Assemb
}

/// <remarks>
/// Assumes we have a lock on _guard
/// Assumes we have a lock on _guard.
/// </remarks>
private Assembly LoadAndCache(AssemblyLoadContext context, string fullPath)
{
Expand Down
19 changes: 12 additions & 7 deletions src/Shared/MSBuildLoadContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,13 @@ internal class MSBuildLoadContext : AssemblyLoadContext
"Microsoft.Build.Utilities.Core",
];

/// <summary>
/// The <see cref="AssemblyLoadContext"/> in which the MSBuild assemblies are loaded.
/// </summary>
internal static AssemblyLoadContext ThisAssemblyLoadContext => GetLoadContext(typeof(MSBuildLoadContext).Assembly)!;

public MSBuildLoadContext(string assemblyPath)
: base($"MSBuild plugin {assemblyPath}")
: base($"MSBuild plugin {assemblyPath}", ThisAssemblyLoadContext.IsCollectible)
{
_directory = Directory.GetParent(assemblyPath)!.FullName;

Expand All @@ -50,12 +55,12 @@ public MSBuildLoadContext(string assemblyPath)
{
if (WellKnownAssemblyNames.Contains(assemblyName.Name!))
{
// Force MSBuild assemblies to be loaded in the default ALC
// Force MSBuild assemblies to be loaded in the same ALC
// and unify to the current version.
return null;
return ThisAssemblyLoadContext.LoadFromAssemblyName(assemblyName);
}

// respect plugin.dll.json with the AssemblyDependencyResolver
// respect plugin.deps.json with the AssemblyDependencyResolver
string? assemblyPath = _resolver?.ResolveAssemblyToPath(assemblyName);
if (assemblyPath != null)
{
Expand Down Expand Up @@ -83,7 +88,7 @@ public MSBuildLoadContext(string assemblyPath)
continue;
}

AssemblyName candidateAssemblyName = AssemblyLoadContext.GetAssemblyName(candidatePath);
AssemblyName candidateAssemblyName = GetAssemblyName(candidatePath);
if (candidateAssemblyName.Version != assemblyName.Version)
{
continue;
Expand All @@ -95,13 +100,13 @@ public MSBuildLoadContext(string assemblyPath)
// If the Assembly is provided via a file path, the following rules are used to load the assembly:
// - the assembly from the user specified path is loaded, if it exists, into the custom ALC, or
// - if the simple name of the assembly exists in the same folder as msbuild.exe, then that assembly gets loaded
// into the default ALC (so it's shared with other uses).
// into MSBuild's ALC (so it's shared with other uses).
var assemblyNameInExecutableDirectory = Path.Combine(BuildEnvironmentHelper.Instance.CurrentMSBuildToolsDirectory,
$"{assemblyName.Name}.dll");

if (FileSystems.Default.FileExists(assemblyNameInExecutableDirectory))
{
return AssemblyLoadContext.Default.LoadFromAssemblyPath(assemblyNameInExecutableDirectory);
return ThisAssemblyLoadContext.LoadFromAssemblyPath(assemblyNameInExecutableDirectory);
}

return null;
Expand Down
6 changes: 3 additions & 3 deletions src/Shared/TaskEngineAssemblyResolver.cs
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class seems to be doing an AppDomain-specific thing, and all but one uses of it are guarded behind FEATURE_APPDOMAIN.

Does the class' functionality make sense for ALCs?

Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ internal void InstallHandler()
#else
_eventHandler = new Func<AssemblyLoadContext, AssemblyName, Assembly>(ResolveAssembly);

AssemblyLoadContext.Default.Resolving += _eventHandler;
AssemblyLoadContext.GetLoadContext(typeof(TaskEngineAssemblyResolver).Assembly).Resolving += _eventHandler;
#endif
}

Expand All @@ -75,7 +75,7 @@ internal void RemoveHandler()
#if FEATURE_APPDOMAIN
AppDomain.CurrentDomain.AssemblyResolve -= _eventHandler;
#else
AssemblyLoadContext.Default.Resolving -= _eventHandler;
AssemblyLoadContext.GetLoadContext(typeof(TaskEngineAssemblyResolver).Assembly).Resolving -= _eventHandler;
#endif
_eventHandler = null;
}
Expand Down Expand Up @@ -125,7 +125,7 @@ private Assembly ResolveAssembly(AssemblyLoadContext assemblyLoadContext, Assemb
AssemblyNameExtension argAssemblyName = new AssemblyNameExtension(assemblyName);
if (taskAssemblyName.Equals(argAssemblyName))
{
return AssemblyLoadContext.Default.LoadFromAssemblyPath(_taskAssemblyFile);
return assemblyLoadContext.LoadFromAssemblyPath(_taskAssemblyFile);
}
#endif
}
Expand Down