Taskhost issue #7132
Taskhost issue #7132
Conversation
Replaces LoadedType for cases when we don't actually have a LoadedType. Loading the type requires loading (and locking) the assembly.
| { | ||
| TypeInformation typeInformation = new(); | ||
| string path = _assemblyLoadInfo.AssemblyFile; | ||
| if (path is null) |
There was a problem hiding this comment.
I'm not sure this find-path-if-null part is right. It needs a second look.
There was a problem hiding this comment.
It wasn't, but I think it works now.
src/Shared/TypeLoader.cs
Outdated
| { | ||
| TypeInformationPropertyInfo toAdd = new(); | ||
| toAdd.Name = metadataReader.GetString(propertyDefinition.Name); | ||
| byte[] bytes = metadataReader.GetBlobReader(propertyDefinition.Signature).ReadBytes(metadataReader.GetBlobReader(propertyDefinition.Signature).Length); |
There was a problem hiding this comment.
bytes isn't currently used. I need to get type information for each property, and that's unusually complicated in the general case because of generics and custom types. We may be able to get by without them because there are only so many things that can be passed as inputs/outputs from a task—thoughts @rainersigwald? Do I need a custom SignatureDecoder? (I was also considering using propertyDefinition.DecodeSignature, but I wasn't sure what TType or TGenericContext would be.)
There was a problem hiding this comment.
Specifically, acceptable types include value types, strings, ITaskItems, and arrays of any of those.
There was a problem hiding this comment.
Should we really be accepting structs? Maybe we should require value types to be { integer, float, char, bool, nullable of one of those, or enum }?
| } | ||
|
|
||
| private TypeInformation FindTypeInformationUsingSystemReflectionMetadata(string typeName) | ||
| { |
There was a problem hiding this comment.
@sharwell told me that Assembly.LoadFrom(bytes) doesn't lock the assembly. He suggested we could potentially load the relevant assembly into a byte array, then use Assembly.LoadFrom to get its full information without changing almost any other code. I'm guessing that would be a little less efficient, but if it can simply load the assembly, that may be worth it.
| /// any) is unambiguous; otherwise, if there are multiple types with the same name in different namespaces, the first type | ||
| /// found will be returned. | ||
| /// </summary> | ||
| internal LoadedType Load |
There was a problem hiding this comment.
We does this exist? I think it's fine to leave it essentially unchanged, since we don't need to worry about locking an assembly we need unlocked when the task host comes to load it in that task host, but should this whole thing be deleted in favor of the shared version?
|
I think this is ready for review. Outside my previous comments, the path calculation in TypeLoader may be problematic. rainersigwald told me the ALC part actually loads the assembly and doesn't let it go just because it's been unloaded, and all references to it have ceased to exist. It's possible running GC.Collect() right afterwards would be sufficient; otherwise, we may need a more complicated "find assembly path" method. The version in MSBuildLoadContext does not take into account .config files, codebases, or the GAC, and it assumes we already know the application base, though that should just be the current project, so maybe we do. |
When they request a task host.
1 participant
Fixes #6461
Context
Changes Made
No longer load an assembly if the user explicitly requests a TaskHostFactory. Use System.Reflection.Metadata to look at its properties instead and "fake" having loaded it.
Testing
Notes