LoadFrom Resource Fallback search fails to search next to current executing assembly

[sdmaclea]

For .NET Core the primary location to look for satellite resource assemblies should be in the culture directory next to the current executing assembly.

This directory is not currently being searched unless it happens to be already on the search path

@jkotas @jeffschwMSFT @tarekgh @vitek-karas FYI

See dotnet/core#2041

[vitek-karas]

This issue is about Assembly.LoadFrom - which has its own dependency resolution logic. It's true that this logic doesn't include any handling of satellite assemblies today, so the described behavior is not surprising.

Did you try this on the .NET Framework? I assume it works there, but would be nice to double check just to make sure.

I think it makes sense to try to fix this.

[sdmaclea]

This issue is about Assembly.LoadFrom

I think LoadFrom may also be broken. Per the docs it is supposed to load into it own context which allows for dependency resolution of its dependencies.

https://docs.microsoft.com/en-us/dotnet/api/system.reflection.assembly.loadfrom?view=netcore-2.1#System_Reflection_Assembly_LoadFrom_System_String_

The load-from context contains assemblies for which the user provided a path not 
included in the directories searched by probing. LoadFrom, CreateInstanceFrom, 
and ExecuteAssembly are examples of methods that load by path. 

The load-from context allows an assembly to be loaded from a path not included 
in probing, and yet allows dependencies on that path to be found and loaded 
because the path information is maintained by the context.

Looking at the code it doesn't look like it is functionally complete.

However the Satellite assembly fallback logic should look next to the current executing assembly. That is not happening either. It shouldn't really matter that it came from LoadFrom, LoadAssemblyFromPath….

[vitek-karas]

The document is about .NET Framework. On .NET Core LoadFrom is not implemented through its own load context, it's just a handler for the global resolve event. But it tries to provide the same behavior.

Whether satellite loading should proactively probe "next to" to the referencing assembly... I'm not so sure about that. We do this for native libraries, but those have rather complex probing logic. Satellite assemblies today don't have much of a probing logic, they simply search the resource probing paths as specified by the host.

Basically the question is if this should be an inherent feature of the runtime, or just a feature of LoadFrom.

[sdmaclea]

If you look at the docs for the .NET Framework resource fallback algorithm, https://github.com/dotnet/docs/blob/77cd71d17d37696f73ba5279a798f36def3cdb24/docs/framework/resources/packaging-and-deploying-resources-in-desktop-apps.md

Then filter it for what is expected in .NET Core, you get something like my PR dotnet/docs#8920

From draft PR...

The .NET Core resource fallback process involves the following steps:

1. The runtime checks the directory of the currently executing assembly for a directory that 
matches the requested culture. If it finds the directory, it searches that directory for a valid 
satellite assembly for the requested culture. The runtime then searches the satellite assembly 
for the requested resource. If it finds the resource in the assembly, it uses it. If it doesn't find 
the resource, it continues the search.

2. The runtime raises the AssemblyLoadContext.Resolving event to indicate that it is unable 
to find the satellite assembly. If you choose to handle the event, your event handler can 
return a reference to the satellite assembly whose resources will be used for the lookup. 
Otherwise, the event handler returns null and the search continues.

3. The runtime next searches the parent culture assemblies through many potential levels, 
each time repeating steps 1 & 2.

The draft is slightly inaccurate as it is missing the AppDomain.AssemblyResolve event.

Ultimately this is a more stable algorithm for finding satellites. It is independent of how its base asembly was loaded so it should just work.

[sdmaclea]

OK I looked closely at the LoadFromResolveHandler. It looks like it covers the simplest LoadFrom, dependency resolve case reasonably well.

• It doesn't handle satellite assemblies.
• It doesn't load into an isolated LoadFromAssemblyLoadContext

Seems reasonable to fix LoadFrom satellite resolution.

I also looked at revising the general fallback algorithm. It looks much more error prone. Seems fixing LoadFrom is the safer first approach.

[jeffschwMSFT]

Unless we have a strong appcompat reason, I would not necessarily look to adjust LoadFrom. I do think we should ensure this code pattern works well when using AssemblyLoadContext.LoadFrom*.

[sdmaclea]

@jeffschwMSFT I believe we are under .NET Standard 2.0 compatibility obligation to make this statement true

The LoadFrom allows an assembly to be loaded from a path not included 
in probing, and yet allows dependencies on that path to be found and loaded. 

Since it is not true for culture dependent assemblies, I think we should fix it.

[jeffschwMSFT]

The LoadFrom allows an assembly to be loaded from a path not included
in probing

I believe this is is true, just not for resource assemblies.

What is the corresponding behavior on .NET Framework?

[sdmaclea]

I'll run the code in the morning. I suspect it is intend to just work.

[jeffschwMSFT]

I have no doubt that with all the probing we do no desktop it may. In my mind, it is fine to fix this code path, but for a concrete reason in mind. The more important case is that we ensure that the recommended path (AssemblyLoadContext) has an even better experience.

[sdmaclea]

It is possible Assembly.LoadFrom should be reimplemented eventually to be AssemblyLoadContext.Default.LoadFromAssemblyPath() so the experience is virtually identical.

[jeffschwMSFT]

It is possible Assembly.LoadFrom should be reimplemented eventually to be AssemblyLoadContext.Default.LoadFromAssemblyPath() so the experience is virtually identical.

I fear that may be out of reach, as Assembly.LoadFrom has a lot of legacy behaviors that have been honed and crafted with many changes to match expectations of existing apps. This is part of my rationale to go after AssemblyLoadContext behavior and leave this one alone until it is necessary to match given an apps expectation.

[vitek-karas]

I think that for AssemblyLoadContext.LoadFromAssemblyPath there's little we can (and should) do. This is the low-level API to load assembly from a file which is supposed to be used by the various ALC implementations. As such this should be an "absolutely no magic" API, needs to be very predictable.

That leaves the AssemblyLoadContext.LoadFromAssemlyName which loads only by name not path. This is basically the recommended API if I understand Jeff correctly (since plugins will need to use some other way in order to create new ALC and so on). This is the one we might consider improving. Note that this API is basically equivalent to Assembly.Load with the exception of explicitly specifying the ALC to load from. The underlying mechanism is the same for both APIs. Also this is more or less the same entry point as the default assembly ref resolution in the runtime (which also loads by name). So if we made changes to how the runtime does probing for satellite assemblies when binding on name, all of these cases would benefit.

I think that would be the best option, assuming we should do something here at all. Indirectly this would also fix Assembly.LoadFrom since the bind would succeed before it gets to the event handler which LoadFrom relies on.

Two concerns:

• Security implications - extending our probing logic inherently means potential security issues. I don't think there's anything too serious here, we should just be aware of this.
• Such change is still potentially a small breaking change - if the bind succeeds because of the new functionality, it means that potential event handlers would not get called. Some apps may rely on them being called (either to provide a different assembly or just the fact that they execute). For a major release I would be perfectly fine taking such change.

[sdmaclea]

I have spent more time looking at this.

When loading satellites the algorithm currently is:

1. Look at assemblies already loaded in the ALC
2. Call ALC.Load(AssemblyName)
3. Try to load in the default context
4. Fire ALC.Resolving
5. Fire AppDomain.AssemblyResolve

The algorithm should be:

1. Look at assemblies already loaded in the ALC
2. Call ALC.Load(AssemblyName)
3. Look for satellite next to parent assembly
4. (maybe) Try to load in the default context
5. Fire ALC.Resolving
6. Fire AppDomain.AssemblyResolve

The 4 item is questionable. The satellites seem like they should always be loaded in to the context of their parent. However, I am willing to leave it in to make this a "less" breaking change.

[sdmaclea]

The native code which is implementing this algorithm seems overly complicated.

A snapshot of the back trace on Linux is here

#0  RuntimeInvokeHostAssemblyResolver (pManagedAssemblyLoadContextToBindWithin=140737353880488, pIAssemblyName=0x74b610, pTPABinder=0x6c5c10, pAssemblyName=0x747b60, ppLoadedAssembly=0x7fffffff9320) at /home/stmaclea/git/coreclr/src/vm/appdomain.cpp:8152
dotnet/coreclr#1  0x00007ffff656ed2e in BINDER_SPACE::AssemblyBinder::BindUsingHostAssemblyResolver (pManagedAssemblyLoadContextToBindWithin=140737353880488, pAssemblyName=0x747b60, pIAssemblyName=0x74b610, pTPABinder=0x6c5c10, ppAssembly=0x7fffffff9458) at /home/stmaclea/git/coreclr/src/binder/assemblybinder.cpp:1835
dotnet/coreclr#2  0x00007ffff655dd5a in CLRPrivBinderAssemblyLoadContext::BindAssemblyByName (this=0x73ad10, pIAssemblyName=0x74b610, ppAssembly=0x7fffffff96b8) at /home/stmaclea/git/coreclr/src/binder/clrprivbinderassemblyloadcontext.cpp:91
dotnet/coreclr#3  0x00007ffff654a817 in BINDER_SPACE::Assembly::BindAssemblyByName (this=0x73be90, pIAssemblyName=0x74b610, ppAssembly=0x7fffffff96b8) at /home/stmaclea/git/coreclr/src/binder/assembly.cpp:299
dotnet/coreclr#4  0x00007ffff61f5282 in AssemblySpec::Bind (this=0x7fffffffaed8, pAppDomain=0x680530, fThrowOnFileNotFound=0, pResult=0x7fffffffa360, fNgenExplicitBind=0, fExplicitBindToNativeImage=0, pCallerStackMark=0x7fffffffb740) at /home/stmaclea/git/coreclr/src/vm/coreassemblyspec.cpp:171
dotnet/coreclr#5  0x00007ffff6155dfd in AppDomain::BindAssemblySpec (this=0x680530, pSpec=0x7fffffffaed8, fThrowOnFileNotFound=0, pCallerStackMark=0x7fffffffb740, fUseHostBinderIfAvailable=1) at /home/stmaclea/git/coreclr/src/vm/appdomain.cpp:6198
dotnet/coreclr#6  0x00007ffff5f12a33 in AssemblySpec::LoadDomainAssembly (this=0x7fffffffaed8, targetLevel=FILE_LOADED, fThrowOnFileNotFound=0, pCallerStackMark=0x7fffffffb740) at /home/stmaclea/git/coreclr/src/vm/assemblyspec.cpp:947
dotnet/coreclr#7  0x00007ffff5f12699 in AssemblySpec::LoadAssembly (this=0x7fffffffaed8, targetLevel=FILE_LOADED, fThrowOnFileNotFound=0, pCallerStackMark=0x7fffffffb740) at /home/stmaclea/git/coreclr/src/vm/assemblyspec.cpp:766
dotnet/coreclr#8  0x00007ffff6267318 in AssemblyNative::Load (assemblyNameUNSAFE=0x7fff60025748, codeBaseUNSAFE=0x0, requestingAssemblyUNSAFE=0x7fff60013ae0, stackMark=0x7fffffffb740, pPrivHostBinder=0x0, fThrowOnFileNotFound=false, ptrLoadContextBinder=0) at /home/stmaclea/git/coreclr/src/vm/assemblynative.cpp:138
dotnet/coreclr#9  0x00007fff7ce45a6c in ?? () ...  Managed InternalGetSatelliteAssembly & Managed nLoad

To implement 3. Look for satellite next to parent assembly the new code naturally wants to be inserted at the tip of the above stack trace. This is problematic here as the parent assembly is not available at this point of the call stack. The last place it is available is #4 0x00007ffff61f5282 in AssemblySpec::Bind where it calls ICLRPrivBinder->BindAssemblyByName(...).

To load the satellite for mscorlib, AssemblySpec::Bind calls CCoreCLRBinderHelper::BindToSystemSatellite(...) only. This function is identical to the 3. Look for satellite next to parent assembly we need to add.

To solve this it seems we will need to modify/extend ICLRPrivBinder. This type is specified by IDL with a guid. @jkotas Do we need this abstract interface anymore.

[jkotas]

Do we need this abstract interface anymore.

We do not need these internal interfaces defined in IDL. Feel free to delete/simplify them.

We never really needed them. It is just that folks working on the assembly loader in the past liked to use them.

[vitek-karas]

I've already looked at the possibility to pass the "parent" assembly throughout the binder, and the outcome was what you state above (need to modify the interface, or get rid of it).
Doing this will not only benefit this work, but also the future improvements to exception messages and tracing as those will also need to know the parent assembly deep in the binder.

[vitek-karas]

Question about step # 4 above. Do you know if calling the default context will also trigger extension points on the default context (namely the Resolving event)?

[sdmaclea]

Question about step # 4 above. Do you know if calling the default context will also trigger extension points on the default context (namely the Resolving event)?

AFAIK it does not right now. It triggers the tpa binder. I can instrument it if you need 100% certainty.

[vitek-karas]

I think I agree with your new list of operations. dotnet/coreclr#4 (fallback to default) should remain. What that will effectively do is trigger a search in the resource paths (as specified by the host). While I understand that for well behaved apps this should not actually resolve anything, what this will for example do is resolve satellites of app assemblies which are intentionally loaded into separate context (but are otherwise normal parts of the app, for example Assembly.LoadFile).

Regarding where the satellite is loaded - it really doesn't matter. Load context of an assembly really only matters for isolation and when that assembly triggers another bind. Satellites will participate in isolation, but they are effectively inert with regard to triggering binds (as satellite can have no assembly refs and it has no code to trigger it programatically). For isolation, it really only matters for version collisions, in which case it's the same story as normal code assemblies, and thus having the same behavior (fallback to default in dotnet/coreclr#4) is preferrable.

[jkotas]

(fallback to default) should remain

I am not sure about this. Consider the following:

• Plugin contains an assembly with no localized satellites
• The main app has a different version of the same assembly, with localized satellites

We do not want the plugin to fallback to localized satellites in the app in this case.

[sdmaclea]

(fallback to default) should remain

Based on personal bias and @jkotas comments I would prefer to remove (fallback to default)

[vitek-karas]

OK - that's a good point. We should find out if we're tracking breaking changes in .NET Core 3 (I think we should) and add this to the list. It is technically a breaking change, although it should be pretty rare.

[sdmaclea]

There is a request to make the culture directory search case-insensitive. I believe this is reasonable.

[ericjohannsen]

Satellite assembly resolution does not work in a very basic console app built against .NET Core 3.0.0.

I created the console application in VS2019 and added welcome.resx and welcome.de.resx. Both have a string resource called Greeting. When the project builds, it creates a subfolder called de containing MyTest.resources.dll. The main method does this:

ResourceManager r = new ResourceManager("MyTest.welcome",
                             Assembly.GetExecutingAssembly());

string greeting = r.GetString("Greeting", 
    new System.Globalization.CultureInfo("de"));

Console.WriteLine(greeting); // Gives me the default resource, not the localized one.

When I add an AssemblyResolve event handler and manually specify the desired assembly (workaround listed in dotnet/core#2041), it works. However, I believe the work-around should no longer be necessary based on the comments in this issue.

See also https://stackoverflow.com/q/58193794/141172

[tarekgh]

@ericjohannsen thanks for reporting the issue, we'll try to take a look and get back to you.

[tarekgh]

@ericjohannsen by the way, are you running on Windows or Mac? I guess you are running Windows using VS2019 as you mentioned.

[ericjohannsen]

@tarekgh Windows 10. VS2019 is the latest build.

[tarekgh]

@ericjohannsen I have tried the same code and I am seeing everything is working just fine. I have attached here the project I used.
ResourceSatellite.zip

This project created with VS2019 and targetting .NET Core 3.0. The code is doing:

            Console.WriteLine("Hello World!");
            ResourceManager r = new ResourceManager("ResourceSatellite.welcome", Assembly.GetExecutingAssembly());
            string greeting = r.GetString("String1");
            Console.WriteLine(greeting);
            greeting = r.GetString("String1", new System.Globalization.CultureInfo("de"));
            Console.WriteLine(greeting);
            Console.WriteLine("Done!");

And that is producing the following output

Hello World!
English
Dutch
Done!

[ericjohannsen]

@tarekgh Thanks for the prompt reply. Your project works as expected on my system, but mine still does not. I'm combing through for differences.

[ericjohannsen]

@tarekgh OK here's the difference: My project also had a manifest file (the default one you get when using Add/New Item). When I deleted the manifest from the project, it ran as expected. Strangely, though, when I added a manifest file back in, it continued to run as expected. Since it's a small test project, I don't have the old project state in source control, so I can't get back to the broken state. I appreciate your efforts and will post a concrete repro if I come across the situation again.

[tarekgh]

Thanks @ericjohannsen for the update. Let's know if you run into the problem again. And please try to add the needed information to https://stackoverflow.com/q/58193794/141172 to help other users.