Avoid locking TaskHostFactory-using assemblies (#7387)

Getting type information for a task that was supposed to be loaded in a TaskHost involves loading that assembly today. The point of TaskHost nodes is that they don't lock the assembly, and they disappear quickly, releasing the lock from when we actually need to load the assembly to execute it. This will mean that we don't lock the assembly to read its type information, actually complying with the user's likely intent.

If the user asks for a TaskHost, use MetadataLoadContext to get its information rather than loading the assembly fully.

Fixes #6461

Author: Forgind

documentation/specs/event-source.md

@@ -11,6 +11,7 @@ EventSource is primarily used to profile code. For MSBuild specifically, a major
 | Build | Sets up a BuildManager to receive build requests. |
 | BuildProject | Builds a project file. |
 | CachedSdkResolverServiceResolveSdk | The caching SDK resolver service is resolving an SDK. |
+| CreateLoadedType | Creates a LoadedType object from an assembly loaded via MetadataLoadContext. |
 | CopyUpToDate | Checks whether the Copy task needs to execute. |
 | Evaluate | Evaluates a project, running several other parts of MSBuild in the process. |
 | EvaluateCondition | Checks whether a condition is true and removes false conditionals. |

eng/Packages.props

@@ -21,6 +21,7 @@
     <PackageReference Update="System.Net.Http" Version="4.3.4" />
     <PackageReference Update="System.Memory" Version="4.5.5" />
     <PackageReference Update="System.Reflection.Metadata" Version="6.0.0" />
+    <PackageReference Update="System.Reflection.MetadataLoadContext" Version="6.0.0" />
     <PackageReference Update="System.Resources.Extensions" Version="$(SystemResourcesExtensionsPackageVersion)" />
     <PackageReference Update="System.Runtime.CompilerServices.Unsafe" Version="$(SystemRuntimeCompilerServicesUnsafeVersion)" />
     <PackageReference Update="System.Security.Permissions" Version="6.0.0" />

src/Build.UnitTests/BackEnd/AssemblyTaskFactory_Tests.cs

@@ -220,7 +220,7 @@ public void CreatableByTaskFactoryMismatchedIdentity()
         public void VerifyGetTaskParameters()
         {
             TaskPropertyInfo[] propertyInfos = _taskFactory.GetTaskParameters();
-            LoadedType comparisonType = new LoadedType(typeof(TaskToTestFactories), _loadInfo);
+            LoadedType comparisonType = new LoadedType(typeof(TaskToTestFactories), _loadInfo, typeof(TaskToTestFactories).GetTypeInfo().Assembly);
             PropertyInfo[] comparisonInfo = comparisonType.Type.GetProperties(BindingFlags.Instance | BindingFlags.Public);
             Assert.Equal(comparisonInfo.Length, propertyInfos.Length);
 

src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs

@@ -1161,7 +1161,7 @@ private void InitializeHost(bool throwOnExecute)
 #else
             AssemblyLoadInfo loadInfo = AssemblyLoadInfo.Create(typeof(TaskBuilderTestTask.TaskBuilderTestTaskFactory).GetTypeInfo().FullName, null);
 #endif
-            LoadedType loadedType = new LoadedType(typeof(TaskBuilderTestTask.TaskBuilderTestTaskFactory), loadInfo);
+            LoadedType loadedType = new LoadedType(typeof(TaskBuilderTestTask.TaskBuilderTestTaskFactory), loadInfo, typeof(TaskBuilderTestTask.TaskBuilderTestTaskFactory).GetTypeInfo().Assembly);
 
             TaskBuilderTestTask.TaskBuilderTestTaskFactory taskFactory = new TaskBuilderTestTask.TaskBuilderTestTaskFactory();
             taskFactory.ThrowOnExecute = throwOnExecute;

src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs

@@ -23,6 +23,7 @@
 
 using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem;
 using Task = System.Threading.Tasks.Task;
+using System.Linq;
 
 #nullable disable
 
@@ -256,12 +257,12 @@ void ITaskExecutionHost.InitializeForTask(IBuildEngine2 buildEngine, TargetLoggi
 
             TaskRequirements requirements = TaskRequirements.None;
 
-            if (_taskFactoryWrapper.TaskFactoryLoadedType.HasSTAThreadAttribute())
+            if (_taskFactoryWrapper.TaskFactoryLoadedType.HasSTAThreadAttribute)
             {
                 requirements |= TaskRequirements.RequireSTAThread;
             }
 
-            if (_taskFactoryWrapper.TaskFactoryLoadedType.HasLoadInSeparateAppDomainAttribute())
+            if (_taskFactoryWrapper.TaskFactoryLoadedType.HasLoadInSeparateAppDomainAttribute)
             {
                 requirements |= TaskRequirements.RequireSeparateAppDomain;
 
@@ -399,6 +400,14 @@ bool ITaskExecutionHost.GatherTaskOutputs(string parameterName, ElementLocation
             try
             {
                 TaskPropertyInfo parameter = _taskFactoryWrapper.GetProperty(parameterName);
+                foreach (TaskPropertyInfo prop in _taskFactoryWrapper.TaskFactoryLoadedType.Properties)
+                {
+                    if (prop.Name.Equals(parameterName, StringComparison.OrdinalIgnoreCase))
+                    {
+                        parameter = prop;
+                        break;
+                    }
+                }
 
                 // flag an error if we find a parameter that has no .NET property equivalent
                 ProjectErrorUtilities.VerifyThrowInvalidProject
@@ -420,17 +429,14 @@ bool ITaskExecutionHost.GatherTaskOutputs(string parameterName, ElementLocation
                     _taskName
                 );
 
-                // grab the outputs from the task's designated output parameter (which is a .NET property)
-                Type type = parameter.PropertyType;
-
                 EnsureParameterInitialized(parameter, _batchBucket.Lookup);
 
-                if (TaskParameterTypeVerifier.IsAssignableToITask(type))
+                if (parameter.IsAssignableToITask)
                 {
                     ITaskItem[] outputs = GetItemOutputs(parameter);
                     GatherTaskItemOutputs(outputTargetIsItem, outputTargetName, outputs, parameterLocation, parameter);
                 }
-                else if (TaskParameterTypeVerifier.IsValueTypeOutputParameter(type))
+                else if (parameter.IsValueTypeOutputParameter)
                 {
                     string[] outputs = GetValueOutputs(parameter);
                     GatherArrayStringAndValueOutputs(outputTargetIsItem, outputTargetName, outputs, parameterLocation, parameter);
@@ -897,12 +903,14 @@ private TaskFactoryWrapper FindTaskInRegistry(IDictionary<string, string> taskId
                 // Map to an intrinsic task, if necessary.
                 if (String.Equals(returnClass.TaskFactory.TaskType.FullName, "Microsoft.Build.Tasks.MSBuild", StringComparison.OrdinalIgnoreCase))
                 {
-                    returnClass = new TaskFactoryWrapper(new IntrinsicTaskFactory(typeof(MSBuild)), new LoadedType(typeof(MSBuild), AssemblyLoadInfo.Create(typeof(TaskExecutionHost).GetTypeInfo().Assembly.FullName, null)), _taskName, null);
+                    Assembly taskExecutionHostAssembly = typeof(TaskExecutionHost).GetTypeInfo().Assembly;
+                    returnClass = new TaskFactoryWrapper(new IntrinsicTaskFactory(typeof(MSBuild)), new LoadedType(typeof(MSBuild), AssemblyLoadInfo.Create(taskExecutionHostAssembly.FullName, null), taskExecutionHostAssembly), _taskName, null);
                     _intrinsicTasks[_taskName] = returnClass;
                 }
                 else if (String.Equals(returnClass.TaskFactory.TaskType.FullName, "Microsoft.Build.Tasks.CallTarget", StringComparison.OrdinalIgnoreCase))
                 {
-                    returnClass = new TaskFactoryWrapper(new IntrinsicTaskFactory(typeof(CallTarget)), new LoadedType(typeof(CallTarget), AssemblyLoadInfo.Create(typeof(TaskExecutionHost).GetTypeInfo().Assembly.FullName, null)), _taskName, null);
+                    Assembly taskExecutionHostAssembly = typeof(TaskExecutionHost).GetTypeInfo().Assembly;
+                    returnClass = new TaskFactoryWrapper(new IntrinsicTaskFactory(typeof(CallTarget)), new LoadedType(typeof(CallTarget), AssemblyLoadInfo.Create(taskExecutionHostAssembly.FullName, null), taskExecutionHostAssembly), _taskName, null);
                     _intrinsicTasks[_taskName] = returnClass;
                 }
             }
@@ -1008,12 +1016,43 @@ out bool parameterSet
             try
             {
                 // check if the task has a .NET property corresponding to the parameter
-                TaskPropertyInfo parameter = _taskFactoryWrapper.GetProperty(parameterName);
+                LoadedType loadedType = _taskFactoryWrapper.TaskFactoryLoadedType;
+                int indexOfParameter = -1;
+                for (int i = 0; i < loadedType.Properties.Length; i++)
+                {
+                    if (loadedType.Properties[i].Name.Equals(parameterName))
+                    {
+                        indexOfParameter = i;
+                        break;
+                    }
+                }
 
-                if (parameter != null)
+                // For most tasks, finding the parameter in our list of known properties is equivalent to
+                // saying the task was properly invoked, as far as this parameter is concerned. However,
+                // that is not true for CodeTaskFactories like RoslynCodeTaskFactory. In that case, they
+                // will often have a list of parameters under the UsingTask declaration. Fortunately, if
+                // your TaskFactory is RoslynCodeTaskFactory, it isn't TaskHostFactory, which means the
+                // types are fully loaded at this stage, and we can access them as we had in the past.
+                TaskPropertyInfo parameter = null;
+                Type parameterType = null;
+                if (indexOfParameter != -1)
+                {
+                    parameter = loadedType.Properties[indexOfParameter];
+                    parameterType = Type.GetType(
+                        loadedType.PropertyAssemblyQualifiedNames?[indexOfParameter] ??
+                        parameter.PropertyType.AssemblyQualifiedName);
+                }
+                else
                 {
-                    Type parameterType = parameter.PropertyType;
+                    parameter = _taskFactoryWrapper.GetProperty(parameterName);
+                    if (parameter != null)
+                    {
+                        parameterType = Type.GetType(parameter.PropertyType.AssemblyQualifiedName);
+                    }
+                }
 
+                if (parameter != null)
+                {
                     EnsureParameterInitialized(parameter, _batchBucket.Lookup);
 
                     // try to set the parameter
@@ -1068,30 +1107,15 @@ out parameterSet
                 else
                 {
                     // flag an error if we find a parameter that has no .NET property equivalent
-                    if (_taskFactoryWrapper.TaskFactoryLoadedType.LoadedAssembly is null)
-                    {
-                        _taskLoggingContext.LogError
-                            (
-                            new BuildEventFileInfo( parameterLocation ),
-                            "UnexpectedTaskAttribute",
-                            parameterName,
-                            _taskName,
-                            _taskFactoryWrapper.TaskFactoryLoadedType.Type.Assembly.FullName,
-                            _taskFactoryWrapper.TaskFactoryLoadedType.Type.Assembly.Location
-                            );
-                    }
-                    else
-                    {
-                        _taskLoggingContext.LogError
-                            (
-                            new BuildEventFileInfo( parameterLocation ),
-                            "UnexpectedTaskAttribute",
-                            parameterName,
-                            _taskName,
-                            _taskFactoryWrapper.TaskFactoryLoadedType.LoadedAssembly.FullName,
-                            _taskFactoryWrapper.TaskFactoryLoadedType.LoadedAssembly.Location
-                            );
-                    }
+                    _taskLoggingContext.LogError
+                        (
+                        new BuildEventFileInfo( parameterLocation ),
+                        "UnexpectedTaskAttribute",
+                        parameterName,
+                        _taskName,
+                        _taskFactoryWrapper.TaskFactoryLoadedType.LoadedAssemblyName.FullName,
+                        _taskFactoryWrapper.TaskFactoryLoadedType.Path
+                        );
                 }
             }
             catch (AmbiguousMatchException)

src/Build/Instance/ReflectableTaskPropertyInfo.cs

@@ -52,6 +52,22 @@ internal ReflectableTaskPropertyInfo(PropertyInfo propertyInfo)
             _propertyInfo = propertyInfo;
         }
 
+        /// <summary>
+        /// Initializes a new <see cref="ReflectableTaskPropertyInfo"/> with three precomputed parameters. This is specifically
+        /// used with MetadataLoadContext, as these parameters cannot be computed for the property type passed in directly but
+        /// rather the relevant base type.
+        /// </summary>
+        internal ReflectableTaskPropertyInfo(PropertyInfo propertyInfo, bool output, bool required, bool isAssignableToITaskItemType)
+            : base(
+            propertyInfo.Name,
+            propertyInfo.PropertyType,
+            output,
+            required)
+        {
+            _propertyInfo = propertyInfo;
+            IsAssignableToITask = isAssignableToITaskItemType;
+        }
+
         /// <summary>
         /// Gets or sets the reflection-produced PropertyInfo.
         /// </summary>

src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using System.Reflection;
 #if FEATURE_APPDOMAIN
 using System.Threading.Tasks;
@@ -149,14 +150,7 @@ public bool Initialize(string taskName, IDictionary<string, string> factoryIdent
         /// </summary>
         public TaskPropertyInfo[] GetTaskParameters()
         {
-            PropertyInfo[] infos = _loadedType.Type.GetProperties(BindingFlags.Instance | BindingFlags.Public);
-            var propertyInfos = new TaskPropertyInfo[infos.Length];
-            for (int i = 0; i < infos.Length; i++)
-            {
-                propertyInfos[i] = new ReflectableTaskPropertyInfo(infos[i]);
-            }
-
-            return propertyInfos;
+            return _loadedType.Properties;
         }
 
         /// <summary>
@@ -279,7 +273,7 @@ string taskProjectFile
             {
                 ErrorUtilities.VerifyThrowArgumentLength(taskName, nameof(taskName));
                 _taskName = taskName;
-                _loadedType = _typeLoader.Load(taskName, loadInfo);
+                _loadedType = _typeLoader.Load(taskName, loadInfo, _taskHostFactoryExplicitlyRequested);
                 ProjectErrorUtilities.VerifyThrowInvalidProject(_loadedType != null, elementLocation, "TaskLoadFailure", taskName, loadInfo.AssemblyLocation, String.Empty);
             }
             catch (TargetInvocationException e)

src/Build/Instance/TaskRegistry.cs

@@ -436,7 +436,7 @@ ElementLocation elementLocation
                         targetLoggingContext.LogComment(MessageImportance.Low, "TaskFoundFromFactory", taskName, taskFactory.Name);
                     }
 
-                    if (taskFactory.TaskFactoryLoadedType.HasSTAThreadAttribute())
+                    if (taskFactory.TaskFactoryLoadedType.HasSTAThreadAttribute)
                     {
                         targetLoggingContext.LogComment(MessageImportance.Low, "TaskNeedsSTA", taskName);
                     }

src/Build/Microsoft.Build.csproj

@@ -35,6 +35,7 @@
     <PackageReference Include="System.Text.Json" />
 
     <PackageReference Include="System.Reflection.Metadata" Condition="'$(MonoBuild)' == 'true'" />
+    <PackageReference Include="System.Reflection.MetadataLoadContext" />
 
     <PackageReference Include="Microsoft.IO.Redist" Condition="'$(FeatureMSIORedist)' == 'true'" />
   </ItemGroup>

src/Framework/MSBuildEventSource.cs

@@ -623,6 +623,30 @@ public void SdkResolverServiceLoadResolversStop(string manifestName, int resolve
             WriteEvent(84, manifestName, resolverCount);
         }
 
+        [Event(85, Keywords = Keywords.All)]
+        public void CreateLoadedTypeStart(string assemblyName)
+        {
+            WriteEvent(85, assemblyName);
+        }
+
+        [Event(86, Keywords = Keywords.All)]
+        public void CreateLoadedTypeStop(string assemblyName)
+        {
+            WriteEvent(86, assemblyName);
+        }
+
+        [Event(87, Keywords = Keywords.All)]
+        public void LoadAssemblyAndFindTypeStart()
+        {
+            WriteEvent(87);
+        }
+
+        [Event(88, Keywords = Keywords.All)]
+        public void LoadAssemblyAndFindTypeStop(string assemblyPath, int numberOfPublicTypesSearched)
+        {
+            WriteEvent(88, assemblyPath, numberOfPublicTypesSearched);
+        }
+
         #endregion
     }
 }

src/Framework/TaskPropertyInfo.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System;
+using System.Reflection;
 
 #nullable disable
 
@@ -26,6 +27,9 @@ public TaskPropertyInfo(string name, Type typeOfParameter, bool output, bool req
             PropertyType = typeOfParameter;
             Output = output;
             Required = required;
+            Type elementType = typeOfParameter.IsArray ? typeOfParameter.GetElementType() : typeOfParameter;
+            IsValueTypeOutputParameter = elementType.GetTypeInfo().IsValueType || elementType.FullName.Equals("System.String");
+            IsAssignableToITask = typeof(ITaskItem).IsAssignableFrom(elementType);
         }
 
         /// <summary>
@@ -62,5 +66,8 @@ public TaskPropertyInfo(string name, Type typeOfParameter, bool output, bool req
         /// Whether the Log and LogItemMetadata properties have been assigned already.
         /// </summary>
         internal bool Initialized = false;
+
+        internal bool IsValueTypeOutputParameter { get; private set; }
+        internal bool IsAssignableToITask { get; set; }
     }
 }

src/MSBuild/MSBuild.csproj

@@ -76,6 +76,7 @@
       <Link>FileUtilities.cs</Link>
       <ExcludeFromStyleCop>true</ExcludeFromStyleCop>
     </Compile>
+    <Compile Include="..\Build\Instance\ReflectableTaskPropertyInfo.cs" />
     <Compile Include="..\Shared\FileUtilitiesRegex.cs">
       <Link>FileUtilitiesRegex.cs</Link>
       <ExcludeFromStyleCop>true</ExcludeFromStyleCop>

src/MSBuild/OutOfProcTaskAppDomainWrapperBase.cs

@@ -115,7 +115,7 @@ IDictionary<string, TaskParameter> taskParams
             try
             {
                 TypeLoader typeLoader = new TypeLoader(TaskLoader.IsTaskClass);
-                taskType = typeLoader.Load(taskName, AssemblyLoadInfo.Create(null, taskLocation));
+                taskType = typeLoader.Load(taskName, AssemblyLoadInfo.Create(null, taskLocation), false);
             }
             catch (Exception e) when (!ExceptionHandling.IsCriticalException(e))
             {
@@ -133,7 +133,7 @@ IDictionary<string, TaskParameter> taskParams
             }
 
             OutOfProcTaskHostTaskResult taskResult;
-            if (taskType.HasSTAThreadAttribute())
+            if (taskType.HasSTAThreadAttribute)
             {
 #if FEATURE_APARTMENT_STATE
                 taskResult = InstantiateAndExecuteTaskInSTAThread(oopTaskHostNode, taskType, taskName, taskLocation, taskFile, taskLine, taskColumn,

src/MSBuild/app.amd64.config

@@ -91,6 +91,16 @@
           <bindingRedirect oldVersion="0.0.0.0-4.1.4.0" newVersion="4.1.4.0" />
           <codeBase version="4.1.4.0" href="..\System.Numerics.Vectors.dll"/>
         </dependentAssembly>
+        <dependentAssembly>
+          <assemblyIdentity name="System.Reflection.Metadata" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
+          <bindingRedirect oldVersion="0.0.0.0-6.0.0.0" newVersion="6.0.0.0" />
+          <codeBase version="6.0.0.0" href="..\System.Reflection.Metadata.dll" />
+        </dependentAssembly>
+        <dependentAssembly>
+          <assemblyIdentity name="System.Reflection.MetadataLoadContext" publicKeyToken="cc7b13ffcd2ddd51" culture="neutral" />
+          <bindingRedirect oldVersion="0.0.0.0-6.0.0.0" newVersion="6.0.0.0" />
+          <codeBase version="6.0.0.0" href="..\System.Reflection.MetadataLoadContext.dll" />
+        </dependentAssembly>
         <dependentAssembly>
           <assemblyIdentity name="System.Resources.Extensions" publicKeyToken="cc7b13ffcd2ddd51" culture="neutral" />
           <bindingRedirect oldVersion="0.0.0.0-6.0.0.0" newVersion="6.0.0.0" />