[release/5.0-rc2] Add test for wasm loader regression, fix loading from bundle #42465
Conversation
CoffeeFlux
added
Servicing-consider
|
Tagging subscribers to this area: @CoffeeFlux |
CoffeeFlux
added
Servicing-approved
|
Approved by tactics via email, but still need a review. cc: @lambdageek |
| public class LoaderLinkTest | ||
| { | ||
| [Fact] | ||
| public static void EnsureTypesLinked() // https://github.com/dotnet/runtime/issues/42207 |
There was a problem hiding this comment.
Nit: I'm not very familiar with the language in the loader so feel free to ignore this. The current name sounds like it's talking about the trimmer \ linker. Should we call this EnsureAssembliesAreLoadedFromTheDefaultLoadContext or something along those lines?
There was a problem hiding this comment.
I think it's mostly fine? The idea is that we want to ensure you're referencing the same type, and 'linked' is a common way to refer to that.
| } | ||
|
|
||
| if (bundles != NULL && is_satellite) { | ||
| // Satellite assembly byname requests should be loaded in the same ALC as their parent assembly |
There was a problem hiding this comment.
Do you think this warrants a test?
There was a problem hiding this comment.
We have a suite of tests for that already, they were just previously passing for the wrong reasons due to the old, bad behavior. I'm planning to update a bunch of our loader tests next week to try and catch this stuff throughout, as well as get the DefaultContext test running on wasm. See my last email to tactics (can forward if you aren't subscribed).
| Assert.Equal(typeof(ISharedInterface), sharedInterface); | ||
|
|
||
| var instance = Activator.CreateInstance(dynamicType); | ||
| Assert.True(instance is ISharedInterface); |
There was a problem hiding this comment.
| Assert.True(instance is ISharedInterface); | |
| Assert.IsAssignableFrom<ISharedInterface>(instance); |
There was a problem hiding this comment.
Oops, good catch. I don't really want to run through CI again just for this, but I'll fix this as part of my updates to the tests next week.
lambdageek
left a comment
lambdageek
left a comment
There was a problem hiding this comment.
netcore_load_reference lgtm
so together with the test, this looks good
ghost
locked as resolved and limited conversation to collaborators
3 participants
Manual backport of #42425 to to release/5.0-rc2
Customer Impact
Without this fix, custom ALCs are are somewhat broken on wasm when using the bundle. Any reference from an assembly in a custom ALC will not check the default ALC to see if that assembly is already loaded there, and will instead load a new copy of the assembly into the custom ALC. This means types won't be linked and you can get multiple copies of common assemblies unintentionally. The added test highlights some of the ways this can break customer code, and of particular note is the fact that
Assembly.Load(byte[])creates a new ALC, so this is actually a regression from 3.2.Testing
A new test was added to catch this specific regression, and the existing ALC tests should still pass.
Risk
Moderate. While this is making a change to the loading algorithm, it's wasm-specific, straightforward, and fixes previously broken behavior. With that said, it is possible that code was unintentionally relying on that broken behavior (which is why some of our tests were passing that shouldn't have, for example).
I believe this to be worth the risk, since the type decoupling makes custom ALCs borderline unusable in a lot of scenarios and regresses Assembly.Load.
cc: @marek-safar @SamMonoRT