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.

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

1388 xml serializer assembly load context awareness #58932

Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
29 changes: 29 additions & 0 deletions src/libraries/Common/tests/System/Runtime/Serialization/Utils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
using System.Threading.Tasks;
using System.Xml.Linq;
using System.Linq;
using System.Reflection;
using System.Runtime.Loader;
using Xunit;

internal static class Utils
Expand Down Expand Up @@ -351,3 +353,30 @@ private static bool IsPrefixedAttributeValue(string atrValue, out string localPr
return false;
}
}

internal class TestAssemblyLoadContext : AssemblyLoadContext
{
private AssemblyDependencyResolver _resolver;

public TestAssemblyLoadContext(string name, bool isCollectible, string mainAssemblyToLoadPath = null) : base(name, isCollectible)
{
if (!PlatformDetection.IsBrowser)
_resolver = new AssemblyDependencyResolver(mainAssemblyToLoadPath ?? Assembly.GetExecutingAssembly().Location);
}

protected override Assembly Load(AssemblyName name)
{
if (PlatformDetection.IsBrowser)
{
return base.Load(name);
}

string assemblyPath = _resolver.ResolveAssemblyToPath(name);
if (assemblyPath != null)
{
return LoadFromAssemblyPath(assemblyPath);
}

return null;
}
}
3 changes: 3 additions & 0 deletions src/libraries/System.Private.Xml/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -2787,6 +2787,9 @@
<data name="XmlNotSerializable" xml:space="preserve">
<value>Type '{0}' is not serializable.</value>
</data>
<data name="XmlTypeInBadLoadContext" xml:space="preserve">
<value>Type '{0}' is from an AssemblyLoadContext which is incompatible with that which contains this XmlSerializer.</value>
</data>
<data name="XmlPregenInvalidXmlSerializerAssemblyAttribute" xml:space="preserve">
<value>Invalid XmlSerializerAssemblyAttribute usage. Please use {0} property or {1} property.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@
<Reference Include="System.Runtime.CompilerServices.Unsafe" />
<Reference Include="System.Runtime.Extensions" />
<Reference Include="System.Runtime.InteropServices" />
<Reference Include="System.Runtime.Loader" />
<Reference Include="System.Security.Cryptography.Algorithms" />
<Reference Include="System.Security.Cryptography.Primitives" />
<Reference Include="System.Text.Encoding.Extensions" />
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -627,40 +627,48 @@ private static void AddObjectsIntoTargetCollection(object targetCollection, List
}
}

private static readonly ConcurrentDictionary<(Type, string), ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate> s_setMemberValueDelegateCache = new ConcurrentDictionary<(Type, string), ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>();
private static readonly ConditionalWeakTable<Type, Hashtable> s_setMemberValueDelegateCache = new ConditionalWeakTable<Type, Hashtable>();

Copy link
Member

Choose a reason for hiding this comment

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

You are still using a ConditionalWeakTable for lookups even when using the default ALC. This will regress a lot as every single property set will hit this method and based on looking at the code for CWT, it's going to be expensive. This is used on a hot code path. You need to do the double table approach for this too.

[RequiresUnreferencedCode(XmlSerializer.TrimSerializationWarning)]
private static ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate GetSetMemberValueDelegate(object o, string memberName)
{
Debug.Assert(o != null, "Object o should not be null");
Debug.Assert(!string.IsNullOrEmpty(memberName), "memberName must have a value");
(Type, string) typeMemberNameTuple = (o.GetType(), memberName);
if (!s_setMemberValueDelegateCache.TryGetValue(typeMemberNameTuple, out ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate? result))
Type type = o.GetType();
var delegateCacheForType = s_setMemberValueDelegateCache.GetValue(type, _ => new Hashtable());
var result = delegateCacheForType[memberName];
if (result == null)
{
MemberInfo memberInfo = ReflectionXmlSerializationHelper.GetEffectiveSetInfo(o.GetType(), memberName);
Debug.Assert(memberInfo != null, "memberInfo could not be retrieved");
Type memberType;
if (memberInfo is PropertyInfo propInfo)
{
memberType = propInfo.PropertyType;
}
else if (memberInfo is FieldInfo fieldInfo)
{
memberType = fieldInfo.FieldType;
}
else
lock (delegateCacheForType)
{
throw new InvalidOperationException(SR.XmlInternalError);
}
if ((result = delegateCacheForType[memberName]) == null)
{
MemberInfo memberInfo = ReflectionXmlSerializationHelper.GetEffectiveSetInfo(o.GetType(), memberName);
Debug.Assert(memberInfo != null, "memberInfo could not be retrieved");
Type memberType;
if (memberInfo is PropertyInfo propInfo)
{
memberType = propInfo.PropertyType;
}
else if (memberInfo is FieldInfo fieldInfo)
{
memberType = fieldInfo.FieldType;
}
else
{
throw new InvalidOperationException(SR.XmlInternalError);
}

MethodInfo getSetMemberValueDelegateWithTypeGenericMi = typeof(ReflectionXmlSerializationReaderHelper).GetMethod("GetSetMemberValueDelegateWithType", BindingFlags.Static | BindingFlags.Public)!;
MethodInfo getSetMemberValueDelegateWithTypeMi = getSetMemberValueDelegateWithTypeGenericMi.MakeGenericMethod(o.GetType(), memberType);
var getSetMemberValueDelegateWithType = (Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>)getSetMemberValueDelegateWithTypeMi.CreateDelegate(typeof(Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>));
result = getSetMemberValueDelegateWithType(memberInfo);
s_setMemberValueDelegateCache.TryAdd(typeMemberNameTuple, result);
MethodInfo getSetMemberValueDelegateWithTypeGenericMi = typeof(ReflectionXmlSerializationReaderHelper).GetMethod("GetSetMemberValueDelegateWithType", BindingFlags.Static | BindingFlags.Public)!;
MethodInfo getSetMemberValueDelegateWithTypeMi = getSetMemberValueDelegateWithTypeGenericMi.MakeGenericMethod(o.GetType(), memberType);
var getSetMemberValueDelegateWithType = (Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>)getSetMemberValueDelegateWithTypeMi.CreateDelegate(typeof(Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>));
result = getSetMemberValueDelegateWithType(memberInfo);
delegateCacheForType[memberName] = result;
}
}
}

return result;
return (ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate)result;
}

private object? GetMemberValue(object o, MemberInfo memberInfo)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ namespace System.Xml.Serialization
using System.Xml.Serialization;
using System.Xml;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;

///<internalonly/>
public abstract class XmlSerializationWriter : XmlSerializationGeneratedCode
Expand Down Expand Up @@ -1465,13 +1466,13 @@ internal static class DynamicAssemblies
{
private static readonly Hashtable s_nameToAssemblyMap = new Hashtable();
private static readonly Hashtable s_assemblyToNameMap = new Hashtable();
private static readonly Hashtable s_tableIsTypeDynamic = Hashtable.Synchronized(new Hashtable());
private static readonly ConditionalWeakTable<Type, object> s_tableIsTypeDynamic = new ConditionalWeakTable<Type, object>();

StephenMolloy marked this conversation as resolved.
Show resolved Hide resolved
// SxS: This method does not take any resource name and does not expose any resources to the caller.
// It's OK to suppress the SxS warning.
internal static bool IsTypeDynamic(Type type)
{
object? oIsTypeDynamic = s_tableIsTypeDynamic[type];
s_tableIsTypeDynamic.TryGetValue(type, out object? oIsTypeDynamic);
if (oIsTypeDynamic == null)
Copy link
Member

Choose a reason for hiding this comment

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

I'm confident this isn't on a hot code path for RefEmit serializer, but what about the reflection based serializer? Does this get called for every object that's being serialized or is it an initialization only usage? If the former, then this needs to do the dual table route.

{
Assembly assembly = type.Assembly;
Expand Down Expand Up @@ -1500,7 +1501,7 @@ internal static bool IsTypeDynamic(Type type)
}
}
}
s_tableIsTypeDynamic[type] = oIsTypeDynamic = isTypeDynamic;
s_tableIsTypeDynamic.AddOrUpdate(type, oIsTypeDynamic = isTypeDynamic);
}
return (bool)oIsTypeDynamic;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.IO;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.Loader;
using System.Runtime.Versioning;
using System.Security;
using System.Text;
Expand Down Expand Up @@ -162,6 +163,7 @@ private static XmlSerializerNamespaces DefaultNamespaces
private const string TrimDeserializationWarning = "Members from deserialized types may be trimmed if not referenced directly";

private static readonly Dictionary<Type, Dictionary<XmlSerializerMappingKey, XmlSerializer>> s_xmlSerializerTable = new Dictionary<Type, Dictionary<XmlSerializerMappingKey, XmlSerializer>>();
private static readonly ConditionalWeakTable<Type, Dictionary<XmlSerializerMappingKey, XmlSerializer>> s_xmlSerializerWeakTable = new ConditionalWeakTable<Type, Dictionary<XmlSerializerMappingKey, XmlSerializer>>();

protected XmlSerializer()
{
Expand Down Expand Up @@ -235,30 +237,28 @@ public XmlSerializer(Type type, string? defaultNamespace)
_tempAssembly = s_cache[defaultNamespace, type];
if (_tempAssembly == null)
{
XmlSerializerImplementation? contract = null;
Assembly? assembly = TempAssembly.LoadGeneratedAssembly(type, defaultNamespace, out contract);
if (assembly == null)
{
XmlSerializerImplementation? contract = null;
Assembly? assembly = TempAssembly.LoadGeneratedAssembly(type, defaultNamespace, out contract);
if (assembly == null)
if (Mode == SerializationMode.PreGenOnly)
{
if (Mode == SerializationMode.PreGenOnly)
{
AssemblyName name = type.Assembly.GetName();
var serializerName = Compiler.GetTempAssemblyName(name, defaultNamespace);
throw new FileLoadException(SR.Format(SR.FailLoadAssemblyUnderPregenMode, serializerName));
}

// need to reflect and generate new serialization assembly
XmlReflectionImporter importer = new XmlReflectionImporter(defaultNamespace);
_mapping = importer.ImportTypeMapping(type, null, defaultNamespace);
_tempAssembly = GenerateTempAssembly(_mapping, type, defaultNamespace)!;
}
else
{
// we found the pre-generated assembly, now make sure that the assembly has the right serializer
// try to avoid the reflection step, need to get ElementName, namespace and the Key form the type
_mapping = XmlReflectionImporter.GetTopLevelMapping(type, defaultNamespace);
_tempAssembly = new TempAssembly(new XmlMapping[] { _mapping }, assembly, contract);
AssemblyName name = type.Assembly.GetName();
var serializerName = Compiler.GetTempAssemblyName(name, defaultNamespace);
throw new FileLoadException(SR.Format(SR.FailLoadAssemblyUnderPregenMode, serializerName));
}

// need to reflect and generate new serialization assembly
XmlReflectionImporter importer = new XmlReflectionImporter(defaultNamespace);
_mapping = importer.ImportTypeMapping(type, null, defaultNamespace);
_tempAssembly = GenerateTempAssembly(_mapping, type, defaultNamespace)!;
}
else
{
// we found the pre-generated assembly, now make sure that the assembly has the right serializer
// try to avoid the reflection step, need to get ElementName, namespace and the Key form the type
_mapping = XmlReflectionImporter.GetTopLevelMapping(type, defaultNamespace);
_tempAssembly = new TempAssembly(new XmlMapping[] { _mapping }, assembly, contract);
}
}
s_cache.Add(defaultNamespace, type, _tempAssembly);
Expand Down Expand Up @@ -403,7 +403,9 @@ public void Serialize(XmlWriter xmlWriter, object? o, XmlSerializerNamespaces? n
}
}
else
{
_tempAssembly.InvokeWriter(_mapping, xmlWriter, o, namespaces == null || namespaces.Count == 0 ? DefaultNamespaces : namespaces, encodingStyle, id);
}
}
catch (Exception? e)
{
Expand Down Expand Up @@ -629,7 +631,10 @@ public static XmlSerializer[] FromMappings(XmlMapping[]? mappings, Type? type)
{
XmlSerializer[] serializers = new XmlSerializer[mappings.Length];
for (int i = 0; i < serializers.Length; i++)
{
serializers[i] = (XmlSerializer)contract!.TypedSerializers[mappings[i].Key!]!;
TempAssembly.VerifyLoadContext(serializers[i]._rootType, type!.Assembly);
}
return serializers;
}
}
Expand Down Expand Up @@ -696,14 +701,30 @@ internal static bool GenerateSerializer(Type[]? types, XmlMapping[] mappings, St
private static XmlSerializer[] GetSerializersFromCache(XmlMapping[] mappings, Type type)
{
XmlSerializer?[] serializers = new XmlSerializer?[mappings.Length];

Dictionary<XmlSerializerMappingKey, XmlSerializer>? typedMappingTable = null;
lock (s_xmlSerializerTable)
AssemblyLoadContext? alc = AssemblyLoadContext.GetLoadContext(type.Assembly);

// Look up in the fast table if not collectible
if (alc == null || !alc.IsCollectible)
{
if (!s_xmlSerializerTable.TryGetValue(type, out typedMappingTable))
lock (s_xmlSerializerTable)
{
typedMappingTable = new Dictionary<XmlSerializerMappingKey, XmlSerializer>();
s_xmlSerializerTable[type] = typedMappingTable;
if (!s_xmlSerializerTable.TryGetValue(type, out typedMappingTable))
{
typedMappingTable = new Dictionary<XmlSerializerMappingKey, XmlSerializer>();
s_xmlSerializerTable[type] = typedMappingTable;
}
}
}
else // Otherwise, look up in the collectible-friendly table
{
lock (s_xmlSerializerWeakTable)
{
if (!s_xmlSerializerWeakTable.TryGetValue(type, out typedMappingTable))
{
typedMappingTable = new Dictionary<XmlSerializerMappingKey, XmlSerializer>();
s_xmlSerializerWeakTable.Add(type, typedMappingTable);
}
}
StephenMolloy marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@
<DefineConstants>$(DefineConstants);ReflectionOnly</DefineConstants>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\..\..\..\Microsoft.XmlSerializer.Generator\tests\SerializableAssembly.csproj" />
<TrimmerRootAssembly Include="SerializableAssembly" />
</ItemGroup>
<ItemGroup>
<Compile Include="$(CommonTestPath)System\Runtime\Serialization\Utils.cs" />
<Compile Include="$(TestSourceFolder)..\..\..\..\System.Runtime.Serialization.Xml\tests\SerializationTypes.cs" />
<None Include="$(TestSourceFolder)..\..\..\..\System.Runtime.Serialization.Xml\tests\SerializationTypes.cs" />
<Compile Include="$(TestSourceFolder)..\..\..\..\System.Runtime.Serialization.Xml\tests\SerializationTypes.RuntimeOnly.cs" />
<Compile Include="$(TestSourceFolder)..\XmlSerializerTests.cs" />
<Compile Include="$(TestSourceFolder)..\XmlSerializerTests.RuntimeOnly.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\..\..\Microsoft.XmlSerializer.Generator\tests\SerializableAssembly.csproj" />
<TrimmerRootAssembly Include="SerializableAssembly" />
</ItemGroup>
<ItemGroup>
<Compile Include="$(CommonTestPath)System\Runtime\Serialization\Utils.cs" />
<Compile Include="$(TestSourceFolder)..\..\..\System.Runtime.Serialization.Xml\tests\SerializationTypes.RuntimeOnly.cs" />
<Compile Include="$(TestSourceFolder)..\..\..\System.Runtime.Serialization.Xml\tests\SerializationTypes.cs" />
<None Include="$(TestSourceFolder)..\..\..\System.Runtime.Serialization.Xml\tests\SerializationTypes.cs" />
<Compile Include="$(TestSourceFolder)XmlSerializerTests.cs" />
<Compile Include="$(TestSourceFolder)XmlSerializerTests.Internal.cs" />
<Compile Include="$(TestSourceFolder)XmlSerializerTests.RuntimeOnly.cs" />
Expand Down
Loading