Merge pull request #4916 from rainersigwald/assemblyloadcontext

Load plugins in an AssemblyLoadContext

documentation/specs/task-isolation-and-dependencies.md

@@ -3,12 +3,13 @@
 Tasks in MSBuild are dynamically loaded assemblies with potentially separate and colliding dependency trees. Currently MSBuild on .NET Core has no isolation between tasks and as such only one version of any given assembly can be loaded. Prime example of this is Newtonsoft.Json which has multiple versions, but all the tasks must agree on it to work.
 This problem is also described in #1754.
 
-## Possible solution
+## Solution
 Use [`AssemblyLoadContext`](https://docs.microsoft.com/en-us/dotnet/api/system.runtime.loader.assemblyloadcontext?view=netcore-2.2) (ALC) to provide binding isolation for task assemblies. Each task assembly would be loaded into its own ALC instance.
 * The ALC would resolve all dependencies of the task assemblies (see dependency resolution below)
 * ALC would fallback to the Default for dependencies which the assembly doesn't carry with itself (frameworks and so on)
 * ALC would probably have to forcefully fallback for MSBuild assemblies since it's possible that tasks will carry these, but the system requires for the MSBuild assemblies to be shared.
-We would probably also want to load groups of tasks which belong together into the same ALC (for example based on their location on disk) to improve performance. This will need some care as there's no guarantee that two random tasks have compatible dependency trees.
+
+We also want to load groups of tasks which belong together into the same ALC (for example based on their location on disk) to improve performance. This will need some care as there's no guarantee that two random tasks have compatible dependency trees. As implemented, each task assembly is loaded into its own ALC.
 
 ## Potential risks
 * Has some small probability of causing breaks. Currently all assemblies from all tasks are loaded into the default context and thus are "visible" to everybody. Tasks with following properties might not work:
@@ -20,13 +21,11 @@ We would probably also want to load groups of tasks which belong together into t
 * None of these changes would have any effect on MSBuild on .NET Framework
 * Task isolation alone could be achieved on existing MSBuild
 
-
-
 # Task dependency resolution
 ## Problem definition
 Tasks with complex and specifically platform specific dependencies don't work out of the box. For example if a task uses [`LibGit2Sharp`](https://www.nuget.org/packages/LibGit2Sharp) package it will not work as is. `LibGit2Sharp` has native dependencies which are platform specific. While the package carries all of them, there's no built in support for the task to load the right ones. For example [source link](https://github.com/dotnet/sourcelink/blob/master/src/Microsoft.Build.Tasks.Git/GitLoaderContext.cs) runs into this problem.
 
-## Possible solution
+## Solution
 .NET Core uses `.deps.json` files to describe dependencies of components. It would be natural to treat task assemblies as components and use associated .deps.json file to determine their dependencies. This would make the system work nicely end to end with the .NET Core CLI/SDK and VS integration.
 In .NET Core 3 there's a new type [`AssemblyDependencyResolver`](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyDependencyResolver.cs) which implements parsing and processing of a `.deps.json` for a component (or assembly). The usage is to create an instance of the resolver pointing to the assembly (in MSBuild case the task assembly). The resolver parses the `.deps.json` and stores the information. It exposes two methods to resolve managed and native dependencies.
 It was designed to be used as the underlying piece to implement custom ALC. So it would work nicely with task isolation above.
@@ -36,3 +35,5 @@ It was designed to be used as the underlying piece to implement custom ALC. So i
 
 ## Additional consideration
 * Task dependency resolution requires APIs which are only available in .NET Core 3.0 (no plan to backport), as such MSBuild will have to target netcoreapp3.0 to use these APIs.
+
+We decided not to implement `AssemblyDependencyResolver` in the .NET Core 3.x timeframe because of the uncertain impact of the change. We should reconsider in the .NET 5 timeframe.

src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs

@@ -1157,7 +1157,7 @@ private void InitializeHost(bool throwOnExecute)
             ProjectInstance project = CreateTestProject();
 
             TypeLoader typeLoader = new TypeLoader(IsTaskFactoryClass);
-#if FEATURE_ASSEMBLY_LOADFROM
+#if !FEATURE_ASSEMBLYLOADCONTEXT
             AssemblyLoadInfo loadInfo = AssemblyLoadInfo.Create(Assembly.GetAssembly(typeof(TaskBuilderTestTask.TaskBuilderTestTaskFactory)).FullName, null);
 #else
             AssemblyLoadInfo loadInfo = AssemblyLoadInfo.Create(typeof(TaskBuilderTestTask.TaskBuilderTestTaskFactory).GetTypeInfo().FullName, null);

src/Build.UnitTests/BackEnd/TaskHostConfiguration_Tests.cs

@@ -114,7 +114,7 @@ public void ConstructorWithNullLocation()
            );
         }
 
-#if FEATURE_ASSEMBLY_LOADFROM
+#if !FEATURE_ASSEMBLYLOADCONTEXT
         /// <summary>
         /// Test that an exception is thrown when the path to the task assembly is empty
         /// </summary>
@@ -267,7 +267,7 @@ public void TestTranslationWithNullDictionary()
             TaskHostConfiguration deserializedConfig = packet as TaskHostConfiguration;
 
             Assert.Equal(config.TaskName, deserializedConfig.TaskName);
-#if FEATURE_ASSEMBLY_LOADFROM
+#if !FEATURE_ASSEMBLYLOADCONTEXT
             Assert.Equal(config.TaskLocation, deserializedConfig.TaskLocation);
 #endif
             Assert.Null(deserializedConfig.TaskParameters);
@@ -305,7 +305,7 @@ public void TestTranslationWithEmptyDictionary()
             TaskHostConfiguration deserializedConfig = packet as TaskHostConfiguration;
 
             Assert.Equal(config.TaskName, deserializedConfig.TaskName);
-#if FEATURE_ASSEMBLY_LOADFROM
+#if !FEATURE_ASSEMBLYLOADCONTEXT
             Assert.Equal(config.TaskLocation, deserializedConfig.TaskLocation);
 #endif
             Assert.NotNull(deserializedConfig.TaskParameters);
@@ -348,7 +348,7 @@ public void TestTranslationWithValueTypesInDictionary()
             TaskHostConfiguration deserializedConfig = packet as TaskHostConfiguration;
 
             Assert.Equal(config.TaskName, deserializedConfig.TaskName);
-#if FEATURE_ASSEMBLY_LOADFROM
+#if !FEATURE_ASSEMBLYLOADCONTEXT
             Assert.Equal(config.TaskLocation, deserializedConfig.TaskLocation);
 #endif
             Assert.NotNull(deserializedConfig.TaskParameters);
@@ -389,7 +389,7 @@ public void TestTranslationWithITaskItemInDictionary()
             TaskHostConfiguration deserializedConfig = packet as TaskHostConfiguration;
 
             Assert.Equal(config.TaskName, deserializedConfig.TaskName);
-#if FEATURE_ASSEMBLY_LOADFROM
+#if !FEATURE_ASSEMBLYLOADCONTEXT
             Assert.Equal(config.TaskLocation, deserializedConfig.TaskLocation);
 #endif
             Assert.NotNull(deserializedConfig.TaskParameters);
@@ -429,7 +429,7 @@ public void TestTranslationWithITaskItemArrayInDictionary()
             TaskHostConfiguration deserializedConfig = packet as TaskHostConfiguration;
 
             Assert.Equal(config.TaskName, deserializedConfig.TaskName);
-#if FEATURE_ASSEMBLY_LOADFROM
+#if !FEATURE_ASSEMBLYLOADCONTEXT
             Assert.Equal(config.TaskLocation, deserializedConfig.TaskLocation);
 #endif
             Assert.NotNull(deserializedConfig.TaskParameters);

src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs

@@ -16,7 +16,7 @@ namespace Microsoft.Build.BackEnd.SdkResolution
 {
     internal class SdkResolverLoader
     {
-#if !FEATURE_ASSEMBLY_LOADFROM
+#if FEATURE_ASSEMBLYLOADCONTEXT
         private readonly CoreClrAssemblyLoader _loader = new CoreClrAssemblyLoader();
 #endif
 
@@ -135,10 +135,9 @@ protected virtual IEnumerable<Type> GetResolverTypes(Assembly assembly)
 
         protected virtual Assembly LoadResolverAssembly(string resolverPath, LoggingContext loggingContext, ElementLocation location)
         {
-#if FEATURE_ASSEMBLY_LOADFROM
+#if !FEATURE_ASSEMBLYLOADCONTEXT
             return Assembly.LoadFrom(resolverPath);
 #else
-            _loader.AddDependencyLocation(Path.GetDirectoryName(resolverPath));
             return _loader.LoadFromPath(resolverPath);
 #endif
         }

src/Build/Microsoft.Build.csproj

@@ -756,6 +756,9 @@
     <Compile Include="..\Shared\TypeLoader.cs">
       <Link>SharedUtilities\TypeLoader.cs</Link>
     </Compile>
+    <Compile Include="..\Shared\MSBuildLoadContext.cs" Condition="'$(TargetFrameworkIdentifier)'!='.NETFramework'">
+      <Link>SharedUtilities\MSBuildLoadContext.cs</Link>
+    </Compile>
     <Compile Include="..\Shared\VisualStudioConstants.cs">
       <Link>VisualStudioConstants.cs</Link>
     </Compile>

src/Directory.BeforeCommon.targets

@@ -24,7 +24,6 @@
     <FeatureAppDomain>true</FeatureAppDomain>
     <DefineConstants>$(DefineConstants);FEATURE_APPDOMAIN_UNHANDLED_EXCEPTION</DefineConstants>
     <DefineConstants>$(DefineConstants);FEATURE_ASPNET_COMPILER</DefineConstants>
-    <DefineConstants>$(DefineConstants);FEATURE_ASSEMBLY_LOADFROM</DefineConstants>
     <DefineConstants>$(DefineConstants);FEATURE_ASSEMBLY_LOCATION</DefineConstants>
     <DefineConstants>$(DefineConstants);FEATURE_ASSEMBLY_GETENTRYASSEMBLY</DefineConstants>
     <DefineConstants>$(DefineConstants);FEATURE_ASSEMBLYNAME_CLONE</DefineConstants>
@@ -120,6 +119,7 @@
 
   <PropertyGroup Condition="'$(NetCoreBuild)'=='true'">
     <CompilerToolsDir>$([System.IO.Path]::Combine($(ToolPackagesDir)Microsoft.Net.Compilers, $(CompilerToolsVersion), "tools"))$([System.IO.Path]::DirectorySeparatorChar)</CompilerToolsDir>
+    <DefineConstants>$(DefineConstants);FEATURE_ASSEMBLYLOADCONTEXT</DefineConstants>
     <DefineConstants>$(DefineConstants);FEATURE_PROCESSSTARTINFO_ENVIRONMENT</DefineConstants>
     <DefineConstants>$(DefineConstants);FEATURE_RUNTIMEINFORMATION</DefineConstants>
     <DefineConstants>$(DefineConstants);USE_MSBUILD_DLL_EXTN</DefineConstants>

src/MSBuild.UnitTests/ValidateAssemblyLoadContext.cs

@@ -0,0 +1,46 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+#if FEATURE_ASSEMBLYLOADCONTEXT
+
+using Microsoft.Build.Framework;
+using Microsoft.Build.Shared;
+using Microsoft.Build.Utilities;
+using System.Runtime.Loader;
+
+namespace Microsoft.Build.UnitTests
+{
+    public class ValidateAssemblyLoadContext : Task
+    {
+        public override bool Execute()
+        {
+            var thisLoadContext = AssemblyLoadContext.GetLoadContext(typeof(ValidateAssemblyLoadContext).Assembly);
+
+            // The straightforward implementation of this check:
+            //   if (thisLoadContext is MSBuildLoadContext context)
+            // fails here because MSBuildLoadContext (in this test assembly) is from MSBuild.exe via
+            // IVT, but the one that actually gets used for task isolation is in Microsoft.Build.dll.
+            // This probably doesn't need to be how it is forever: https://github.com/microsoft/msbuild/issues/5041
+            if (thisLoadContext.GetType().FullName == typeof(MSBuildLoadContext).FullName)
+            {
+#if NETCOREAPP && !NETCOREAPP2_1 // TODO: enable this functionality when targeting .NET Core 3.0+
+                if (!thisLoadContext.Name.EndsWith(typeof(ValidateAssemblyLoadContext).Assembly.GetName().Name + ".dll"))
+                {
+                    Log.LogError($"Unexpected AssemblyLoadContext name: \"{thisLoadContext.Name}\", but the current executing assembly was {typeof(ValidateAssemblyLoadContext).Assembly.GetName().Name}");
+                }
+                else
+                {
+                    Log.LogMessage(MessageImportance.High, $"Task {nameof(ValidateAssemblyLoadContext)} loaded in AssemblyLoadContext named {thisLoadContext.Name}");
+                }
+#endif
+            }
+            else
+            {
+                Log.LogError($"Load context was a {thisLoadContext.GetType().AssemblyQualifiedName} instead of an {typeof(MSBuildLoadContext).AssemblyQualifiedName}");
+            }
+
+            return !Log.HasLoggedErrors;
+        }
+    }
+}
+#endif

src/MSBuild.UnitTests/XMake_Tests.cs

@@ -18,6 +18,7 @@
 using Xunit.Abstractions;
 using Shouldly;
 using System.IO.Compression;
+using System.Reflection;
 
 namespace Microsoft.Build.UnitTests
 {
@@ -2119,6 +2120,34 @@ public void BinaryLogContainsImportedFiles()
             }
         }
 
+#if FEATURE_ASSEMBLYLOADCONTEXT
+        /// <summary>
+        /// Ensure that tasks get loaded into their own <see cref="System.Runtime.Loader.AssemblyLoadContext"/>.
+        /// </summary>
+        /// <remarks>
+        /// When loading a task from a test assembly in a test within that assembly, the assembly is already loaded
+        /// into the default context. So put the test here and isolate the task into an MSBuild that runs in its
+        /// own process, causing it to newly load the task (test) assembly in a new ALC.
+        /// </remarks>
+        [Fact]
+        public void TasksGetAssemblyLoadContexts()
+        {
+            string customTaskPath = Assembly.GetExecutingAssembly().Location;
+
+            string projectContents = $@"<Project ToolsVersion=`msbuilddefaulttoolsversion` xmlns=`msbuildnamespace`>
+  <UsingTask TaskName=`ValidateAssemblyLoadContext` AssemblyFile=`{customTaskPath}` />
+
+  <Target Name=`Build`>
+    <ValidateAssemblyLoadContext />
+  </Target>
+</Project>";
+
+            ExecuteMSBuildExeExpectSuccess(projectContents);
+        }
+
+#endif
+
+
         private string CopyMSBuild()
         {
             string dest = null;

src/MSBuild/MSBuild.csproj

@@ -135,6 +135,7 @@
     <Compile Include="..\Shared\OutOfProcTaskHostTaskResult.cs" />
     <Compile Include="..\Shared\TaskHostTaskCancelled.cs" />
     <Compile Include="..\Shared\TaskLoader.cs" />
+    <Compile Include="..\Shared\MSBuildLoadContext.cs" Condition="'$(TargetFrameworkIdentifier)'!='.NETFramework'" />
     <Compile Include="..\Shared\TypeLoader.cs" />
     <Compile Include="..\Shared\LoadedType.cs">
       <ExcludeFromStyleCop>true</ExcludeFromStyleCop>

src/Shared/AssemblyNameExtension.cs

@@ -11,7 +11,7 @@
 using System.Configuration.Assemblies;
 using System.Runtime.Serialization;
 using System.IO;
-#if !FEATURE_ASSEMBLY_LOADFROM
+#if FEATURE_ASSEMBLYLOADCONTEXT
 using System.Reflection.PortableExecutable;
 using System.Reflection.Metadata;
 #endif
@@ -183,7 +183,7 @@ private AssemblyNameExtension(SerializationInfo info, StreamingContext context)
         internal static AssemblyNameExtension GetAssemblyNameEx(string path)
         {
             AssemblyName assemblyName = null;
-#if FEATURE_ASSEMBLY_LOADFROM
+#if !FEATURE_ASSEMBLYLOADCONTEXT
             try
             {
                 assemblyName = AssemblyName.GetAssemblyName(path);