[release/6.0] 1388 xml serializer assembly load context awareness #61266
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…on't keep any hard refs to types that could prevent unloading.
…ain test assembly.
…ewriting similar code everywhere.
StephenMolloy
added
the
Servicing-consider
|
The test failures are due to a bug in coreclr that we discovered while working on this PR. It has been fixed in 7.0. I don't know if the CLR team wants to backport the fix to 6.0. Without the CLR fix, this PR still enables the scenario of exception-free use of XmlSerializer with types in an unloadable ALC. However, the ALC in some cases will end up becoming not unloadable. |
|
@mconnew Can you also give a risk assessment here? |
|
I agree with what @StephenMolloy said. The only behavior change is for scenarios which were previously failing scenarios. The code maintains the same behavior for all existing working scenarios. The nature of this change is very binary for existing working scenarios. It will either always work, or always won't work. If there was a mistake made, it would always fail as there are no edge cases. All our test scenarios are passing so there is very high confidence we have zero functional regression. The new enabled scenarios will have slightly poorer performance due to use of ConditionalWeakTable, but this isn't on a hot code path, only serializer instantiation. Existing working scenarios avoid the more expensive ConditionalWeakTable and continue to use the same collections so will be unaffected. |
|
Great! Could you also review the backport? |
There was a problem hiding this comment.
Approve for servicing as the risk of the change is low.
That said, the original feature (sorry I wasn't able to review that in main) has some issues which should be fixed as it will not work correctly in some cases. But the incorrect behavior is only going to happen in the cases where it originally didn't work at all. That said, somebody might take a dependency on the incorrect behavior and it would make it harder for us to fix this in future releases.
| s_xmlSerializerTable[type] = typedMappingTable; | ||
| } | ||
| } | ||
| AssemblyLoadContext? alc = AssemblyLoadContext.GetLoadContext(type.Assembly); |
There was a problem hiding this comment.
This doesn't seem to be used anywhere.
There was a problem hiding this comment.
Ahh, a remnant from a previous iteration where we spread logic out instead of encapsulating these repetitive patterns in our new ContextAwareTable. I will remove this line.
src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs
Show resolved
Hide resolved
| var obj = Activator.CreateInstance(type); | ||
| var rtobj = SerializeAndDeserialize(obj, null, () => serializer, true); | ||
| Assert.NotNull(rtobj); | ||
| Assert.True(rtobj.Equals(obj)); |
There was a problem hiding this comment.
Worth a comment that this assumes that the tested type implements equality correctly.
I would still probably add a check that the deserialized type is from the test ALC (same type as we serialized, not just the same name).
There was a problem hiding this comment.
The test I added does the right thing with Equals(). I chose the type to use in part because I could make Equals() work here. But I see how this method is not restricted to the type that I choose to pass in in the one test I added. I can add the extra check.
src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs
Show resolved
Hide resolved
| { | ||
| throw; | ||
| serializer = Assembly.Load(name); |
There was a problem hiding this comment.
Instead of using contextual reflection it would be cleaner to just call AssemblyLoadContext.GetLoadContext(type.Assembly).LoadFromAssemblyName.
We recommend people stop using the Assembly.Load because of its weird behavior in terms of which ALC it loads into (without contextual reflection it relies on caller's ALC which is often times wrong).
| else if (assemblyAttribute.CodeBase != null && assemblyAttribute.CodeBase.Length > 0) | ||
| { | ||
| serializerName = assemblyAttribute.CodeBase; | ||
| serializer = Assembly.LoadFrom(serializerName); |
There was a problem hiding this comment.
Not sure what the intention is here, but this will probably not work correctly with custom ALCs.
Assembly.LoadFrom ALWAYS loads into the default ALC. I think this should call AssemblyLoadContext.GetLoadContext(type.Assembly).LoadFromAssemblyPath instead.
In short - it would be better to not use contextual reflection for this method at all.
|
@StephenMolloy it seems like Xml_TypeInCollectibleALC test is failing consistently, we nee d to fix that in order to be able to merge. |
There is a bug in AppDomain::FindAssembly code path that iterates over the domain assemblies. The iteration loop leaves the refcount of the LoaderAllocator related to the returned DomainAssembly bumped, but nothing ever decrements it. So when a code that needs to be unloaded ends up in that code path, all the managed things like managed LoaderAllocator, LoaderAllocatorScout are destroyed, but the unloading doesn't complete due to the refcount. We have never found it before as this code path is never executed in any of the coreclr tests even with unloadability testing option. (cherry picked from commit 8b38c19)
agocke
added
Servicing-approved
ghost
locked as resolved and limited conversation to collaborators
Backport of #58932 to release/6.0
/cc @StephenMolloy
Customer Impact
Prior to this fix, customers could not use XmlSerializer with types that exist in collectible AssemblyLoadContexts. Customers expect to be able to use XmlSerializer in their projects as they have since the days of the full .Net Framework, but if they do, this limitation can prevent them from using a useful feature like ALC in .Net Core. This has been something that customers have been asking for for quite some time and we had intended to target 6.0 with the fix, but did not want to rush a check-in as we came up to the rtm deadline.
Testing
This PR enables new functionality which is only subtly different from the old. Mostly just a difference in which lookup tables to use. All the old XmlSerializer tests cover the old and most common expectation of working primarily in the default ALC. We have added a new test to verify XmlSerializer can work with an unloadable ALC as well. Lots of ad-hoc scenarios were tested while developing this PR and a quick check of very basic serializer performance was done on the final PR before merging in 7.0 with no obvious regression seen.
Risk
Low. The PR enables a currently-broken scenario. The much more common non-broken scenario is pretty well covered by existing tests. We are confident the PR addresses the broken scenario, but if there is a problem here we haven't identified, then the scenario is still broken... which is the same as before.