-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[release/5.0-rc2] Add test for wasm loader regression, fix loading from bundle #42465
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Compile Include="SharedInterfaceImplementation.cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <ProjectReference Include="..\LoaderLinkTest.Shared\LoaderLinkTest.Shared.csproj" /> | ||
| </ItemGroup> | ||
| </Project> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
|
|
||
| using LoaderLinkTest.Shared; | ||
|
|
||
| namespace LoaderLinkTest.Dynamic | ||
| { | ||
| public class SharedInterfaceImplementation : ISharedInterface | ||
| { | ||
| public string TestString => "Hello"; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
|
|
||
| namespace LoaderLinkTest.Shared | ||
| { | ||
| public interface ISharedInterface | ||
| { | ||
| string TestString { get; } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Compile Include="ISharedInterface.cs" /> | ||
| </ItemGroup> | ||
| </Project> |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,33 @@ | ||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||
|
|
||||||
| using System; | ||||||
| using System.IO; | ||||||
| using System.Linq; | ||||||
| using System.Reflection; | ||||||
| using Xunit; | ||||||
|
|
||||||
| using LoaderLinkTest.Shared; | ||||||
|
|
||||||
| namespace LoaderLinkTest | ||||||
| { | ||||||
| public class LoaderLinkTest | ||||||
| { | ||||||
| [Fact] | ||||||
| public static void EnsureTypesLinked() // https://github.com/dotnet/runtime/issues/42207 | ||||||
| { | ||||||
| string parentDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location); | ||||||
| byte[] bytes = File.ReadAllBytes(Path.Combine(parentDir != null ? parentDir : "/", "LoaderLinkTest.Dynamic.dll")); | ||||||
| Assembly asm = Assembly.Load(bytes); | ||||||
| var dynamicType = asm.GetType("LoaderLinkTest.Dynamic.SharedInterfaceImplementation", true); | ||||||
| var sharedInterface = dynamicType.GetInterfaces().First(e => e.Name == nameof(ISharedInterface)); | ||||||
| Assert.Equal(typeof(ISharedInterface).Assembly, sharedInterface.Assembly); | ||||||
| Assert.Equal(typeof(ISharedInterface), sharedInterface); | ||||||
|
|
||||||
| var instance = Activator.CreateInstance(dynamicType); | ||||||
| Assert.True(instance is ISharedInterface); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
|
||||||
| Assert.NotNull(((ISharedInterface)instance).TestString); // cast should not fail | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1684,23 +1684,27 @@ netcore_load_reference (MonoAssemblyName *aname, MonoAssemblyLoadContext *alc, M | |
| * Try these until one of them succeeds (by returning a non-NULL reference): | ||
| * 1. Check if it's already loaded by the ALC. | ||
| * | ||
| * 2. If we have a bundle registered, search the images for a matching name. | ||
| * 2. If it's a non-default ALC, call the Load() method. | ||
| * | ||
| * 3. If it's a non-default ALC, call the Load() method. | ||
| * | ||
| * 4. If the ALC is not the default and this is not a satellite request, | ||
| * 3. If the ALC is not the default and this is not a satellite request, | ||
| * check if it's already loaded by the default ALC. | ||
| * | ||
| * 5. If the ALC is the default or this is not a satellite request, | ||
| * 4. If we have a bundle registered and this is not a satellite request, search the images for a matching name | ||
| * and load into the default ALC. | ||
| * | ||
| * 5. If we have a bundle registered and this is a satellite request, search the images for a matching name | ||
| * and load into the parent assembly's ALC. | ||
| * | ||
| * 6. If the ALC is the default or this is not a satellite request, | ||
| * check the TPA list, APP_PATHS, and ApplicationBase. | ||
| * | ||
| * 6. If this is a satellite request, call the ALC ResolveSatelliteAssembly method. | ||
| * 7. If this is a satellite request, call the ALC ResolveSatelliteAssembly method. | ||
| * | ||
| * 7. Call the ALC Resolving event. | ||
| * 8. Call the ALC Resolving event. | ||
| * | ||
| * 8. Call the ALC AssemblyResolve event (except for corlib satellite assemblies). | ||
| * 9. Call the ALC AssemblyResolve event (except for corlib satellite assemblies). | ||
| * | ||
| * 9. Return NULL. | ||
| * 10. Return NULL. | ||
| */ | ||
|
|
||
| reference = mono_assembly_loaded_internal (alc, aname, FALSE); | ||
|
|
@@ -1709,14 +1713,6 @@ netcore_load_reference (MonoAssemblyName *aname, MonoAssemblyLoadContext *alc, M | |
| goto leave; | ||
| } | ||
|
|
||
| if (bundles != NULL) { | ||
| reference = search_bundle_for_assembly (alc, aname); | ||
| if (reference) { | ||
| mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_ASSEMBLY, "Assembly found in the bundle: '%s'.", aname->name); | ||
| goto leave; | ||
| } | ||
| } | ||
|
|
||
| if (!is_default) { | ||
| reference = mono_alc_invoke_resolve_using_load_nofail (alc, aname); | ||
| if (reference) { | ||
|
|
@@ -1733,6 +1729,38 @@ netcore_load_reference (MonoAssemblyName *aname, MonoAssemblyLoadContext *alc, M | |
| } | ||
| } | ||
|
|
||
| if (bundles != NULL && !is_satellite) { | ||
| reference = search_bundle_for_assembly (mono_domain_default_alc (mono_alc_domain (alc)), aname); | ||
| if (reference) { | ||
| mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_ASSEMBLY, "Assembly found in the bundle: '%s'.", aname->name); | ||
| goto leave; | ||
| } | ||
| } | ||
|
|
||
| if (bundles != NULL && is_satellite) { | ||
| // Satellite assembly byname requests should be loaded in the same ALC as their parent assembly | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think this warrants a test?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
| size_t name_len = strlen (aname->name); | ||
| char *parent_name = NULL; | ||
| MonoAssemblyLoadContext *parent_alc = NULL; | ||
| if (g_str_has_suffix (aname->name, MONO_ASSEMBLY_RESOURCE_SUFFIX)) | ||
| parent_name = g_strdup_printf ("%s.dll", g_strndup (aname->name, name_len - strlen (MONO_ASSEMBLY_RESOURCE_SUFFIX))); | ||
|
|
||
| if (parent_name) { | ||
| MonoAssemblyOpenRequest req; | ||
| mono_assembly_request_prepare_open (&req, MONO_ASMCTX_DEFAULT, alc); | ||
| MonoAssembly *parent_assembly = mono_assembly_request_open (parent_name, &req, NULL); | ||
| parent_alc = mono_assembly_get_alc (parent_assembly); | ||
| } | ||
|
|
||
| if (parent_alc) | ||
| reference = search_bundle_for_assembly (parent_alc, aname); | ||
|
|
||
| if (reference) { | ||
| mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_ASSEMBLY, "Satellite assembly found in the bundle: '%s'.", aname->name); | ||
| goto leave; | ||
| } | ||
| } | ||
|
|
||
| if (is_default || !is_satellite) { | ||
| reference = invoke_assembly_preload_hook (mono_domain_default_alc (mono_alc_domain (alc)), aname, assemblies_path); | ||
| if (reference) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
EnsureAssembliesAreLoadedFromTheDefaultLoadContextor something along those lines?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.