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 6 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,15 +627,16 @@ 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, ConcurrentDictionary<string, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>> s_setMemberValueDelegateCache = new ConditionalWeakTable<Type, ConcurrentDictionary<string, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>>();

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.GetOrCreateValue(type);
if (!delegateCacheForType.TryGetValue(memberName, out var result))
Copy link
Member

Choose a reason for hiding this comment

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

GetOrCreateValue uses Activator.CreateInstance to create an instance of TValue (ConcurrentDictionary in this case). There's another method you can use where you provide a callback which would be more efficient.

var delegateCacheForType = s_setMemberValueDelegateCache.GetValue(type, _ => return new ConcurrentDictionary<string, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>());

Also ConcurrentDictionary is very wasteful for space in it's defaults. The default capacity is 31 and default concurrency is the number of CPU's there are. It creates the same number of locks as the concurrency and spreads the data among that number of internal data stores. The likelyhood of there being multiple threads trying to write to this concurrent dictionary is very low, and that will only happen the first time serializing a particular type. I have 2 suggestions.

  1. Use the GetValue overload above and use the ConcurrentDictionary constructor which takes concurrencyLevel and capacity. I would set capacity to 31 (seems reasonable for number of properties on a type) and concurrencyLevel to 2. It does mean if you get 3 threads trying to add to the dictionary at once, one of them would block. But that would require 3 threads trying to use a serializer for the first time each serializing the same type at once, and would only happen once.
  2. Switch to using a Hashtable and use a lock around all writes.

Also this is going to regress performance when using the reflection based serializer. I would suggest having two collections, one for the default ALC, and one for everything else. The extra cost would be on the creation, but lookup for the default case will remain exactly the same.

{
MemberInfo memberInfo = ReflectionXmlSerializationHelper.GetEffectiveSetInfo(o.GetType(), memberName);
Debug.Assert(memberInfo != null, "memberInfo could not be retrieved");
Expand All @@ -657,7 +658,7 @@ private static ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate Get
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);
delegateCacheForType.TryAdd(memberName, result);
}

return result;
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,12 @@
<DefineConstants>$(DefineConstants);ReflectionOnly</DefineConstants>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\..\..\..\Microsoft.XmlSerializer.Generator\tests\SerializableAssembly.csproj" />
</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,13 @@
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\..\..\Microsoft.XmlSerializer.Generator\tests\SerializableAssembly.csproj" />
</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
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
using System.IO;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.Loader;
using System.Text;
using System.Threading;
using System.Xml;
Expand Down Expand Up @@ -1960,6 +1962,50 @@ public static void Xml_TypeWithSpecialCharacterInStringMember()
Assert.Equal(x.Name, y.Name);
}

[Fact]
#if XMLSERIALIZERGENERATORTESTS
// Lack of AssemblyDependencyResolver results in assemblies that are not loaded by path to get
// loaded in the default ALC, which causes problems for this test.
[SkipOnPlatform(TestPlatforms.Browser, "AssemblyDependencyResolver not supported in wasm")]
#endif
public static void Xml_TypeInCollectibleALC()
{
ExecuteAndUnload("SerializableAssembly.dll", "SerializationTypes.SimpleType", out var weakRef);

for (int i = 0; weakRef.IsAlive && i < 10; i++)
{
GC.Collect();
GC.WaitForPendingFinalizers();
}
Assert.True(!weakRef.IsAlive);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static void ExecuteAndUnload(string assemblyfile, string typename, out WeakReference wref)
{
StephenMolloy marked this conversation as resolved.
Show resolved Hide resolved
var fullPath = Path.GetFullPath(assemblyfile);
var alc = new TestAssemblyLoadContext("XmlSerializerTests", true, fullPath);
wref = new WeakReference(alc);

// Load assembly by path. By name, and it gets loaded in the default ALC.
var asm = alc.LoadFromAssemblyPath(fullPath);

// Ensure the type loaded in the intended non-Default ALC
var type = asm.GetType(typename);
Assert.Equal(AssemblyLoadContext.GetLoadContext(type.Assembly), alc);
Assert.NotEqual(alc, AssemblyLoadContext.Default);

// Round-Trip the instance
XmlSerializer serializer = new XmlSerializer(type);
var obj = Activator.CreateInstance(type);
var rtobj = SerializeAndDeserialize(obj, null, () => serializer, true);
Assert.NotNull(rtobj);
Assert.True(rtobj.Equals(obj));

alc.Unload();
}


private static readonly string s_defaultNs = "http://tempuri.org/";
private static T RoundTripWithXmlMembersMapping<T>(object requestBodyValue, string memberName, string baseline, bool skipStringCompare = false, string wrapperName = null)
{
Expand Down Expand Up @@ -2080,11 +2126,7 @@ private static Stream GenerateStreamFromString(string s)
private static T SerializeAndDeserialize<T>(T value, string baseline, Func<XmlSerializer> serializerFactory = null,
bool skipStringCompare = false, XmlSerializerNamespaces xns = null)
{
XmlSerializer serializer = new XmlSerializer(typeof(T));
if (serializerFactory != null)
{
serializer = serializerFactory();
}
XmlSerializer serializer = (serializerFactory != null) ? serializerFactory() : new XmlSerializer(typeof(T));

using (MemoryStream ms = new MemoryStream())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@ public static bool AreEqual(SimpleType x, SimpleType y)
return (x.P1 == y.P1) && (x.P2 == y.P2);
}
}

public override bool Equals(object? obj)
{
if (obj is SimpleType st)
return AreEqual(this, st);

return base.Equals(obj);
}

public override int GetHashCode() => base.GetHashCode();
}

public class TypeWithGetSetArrayMembers
Expand Down