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.