AssemblyScanner duplicated assembly loading with possible System.IO.FileLoadException: Assembly with same name is already loaded

[alexeypukhov]

Summary

I am facing an issue when NServiceBus AssemblyScanner is trying to load an assembly for a name for which an assembly was already loaded. In my environment this leads to an application crash (see error and callstack below) however even if it won't crash the application, I may still spot some undesired behavior (also see them below). So I am suggesting to improve the logic of AssemblyValidator to skip an assembly if there is already another assembly loaded for the same name. It may not be an ideal-ideal solution but in my opinion it will eliminate edge cases that the customers do not need anyways.

Crash Situation

Our product is an APM dependency that loads to a customer application and we provide assemblies for different architectures (x64, x86) and frameworks (netframework, net) and for some deployments those assemblies may all appear in the NServiceBus AssemblyScanner paths; and that will lead to a crash when assemblies for different architectures are tried to be loaded together. An easy example is when all our assemblies are dropped flat into the customer application working directory, however with AssemblyScanner NestedDirectory setting on or an Additional Assembly Scanning path the possibility of this issue increases. Reproed it on NServiceBus versions 7.8.4 and 8.2.0

Unhandled exception. System.Exception: Could not load '....some_assembly_name.dll'. Consider excluding that assembly from the scanning.
 ---> System.IO.FileLoadException: Assembly with same name is already loaded
   at System.Runtime.Loader.AssemblyLoadContext.LoadFromPath(IntPtr ptrNativeAssemblyLoadContext, String ilPath, String niPath, ObjectHandleOnStack retAssembly)
   at System.Runtime.Loader.AssemblyLoadContext.LoadFromAssemblyPath(String assemblyPath)
   at NServiceBus.Hosting.Helpers.AssemblyScanner.TryLoadScannableAssembly(String assemblyPath, AssemblyScannerResults results, Assembly& assembly) in /_/src/NServiceBus.Core/Hosting/Helpers/AssemblyScanner.cs:line 190
   --- End of inner exception stack trace ---
   at NServiceBus.Hosting.Helpers.AssemblyScanner.TryLoadScannableAssembly(String assemblyPath, AssemblyScannerResults results, Assembly& assembly) in /_/src/NServiceBus.Core/Hosting/Helpers/AssemblyScanner.cs:line 203
   at NServiceBus.Hosting.Helpers.AssemblyScanner.ScanAssembliesInDirectory(String directoryToScan, List`1 assemblies, AssemblyScannerResults results) in /_/src/NServiceBus.Core/Hosting/Helpers/AssemblyScanner.cs:line 155
   at NServiceBus.Hosting.Helpers.AssemblyScanner.GetScannableAssemblies() in /_/src/NServiceBus.Core/Hosting/Helpers/AssemblyScanner.cs:line 118
   at NServiceBus.AssemblyScanningComponent.Initialize(Configuration configuration, SettingsHolder settings) in /_/src/NServiceBus.Core/Hosting/Helpers/AssemblyScanningComponent.cs:line 44
   at NServiceBus.EndpointCreator.Create(EndpointConfiguration endpointConfiguration, IServiceCollection serviceCollection) in /_/src/NServiceBus.Core/EndpointCreator.cs:line 27
   at NServiceBus.Endpoint.Create(EndpointConfiguration configuration, CancellationToken cancellationToken) in /_/src/NServiceBus.Core/Endpoint.cs:line 23
   at NServiceBus.Endpoint.Start(EndpointConfiguration configuration, CancellationToken cancellationToken) in /_/src/NServiceBus.Core/Endpoint.cs:line 42

Undesired Behavior: loading same assembly twice

Even if we can avoid a crash there is a chance the same assembly is loaded twice which may lead to unexpected and undesired behavior when twice loaded assembly triggers double initialization. This one is easy to repro by loading an assembly as a stream prior to calling NserviceBus Endpoint.Start() assuming of course that this assembly appears in the NServiceBus AssemblyScanner path.

byte[] bytes = File.ReadAllBytes("<path_from_the_assembly_scanner_path>\some_assembly_name.dll");
Assembly.Load(bytes);
...
IEndpointInstance endpointInstance = await Endpoint.Start(endpointConfiguration);

This is another use case which is very easy to hit by our customers because our APM dependency is loaded as a stream and if it appears in NServiceBus AssemblyScanner path and there is no our other architecture assembly there, the application won't crash but our assembly will be picked up by the NServiceBus AssemblyScanner and end up loaded twice

Suggested Solution

As a solution I would add a check after this line https://github.com/Particular/NServiceBus/blob/5bd9dba6677a839bb880c6c769d1b0a67a9a06fb/src/NServiceBus.Core/Hosting/Helpers/AssemblyValidator.cs#L32 if there is already an assembly with the same name loaded in AppDomain we just early out saying the assembly is already loaded. I can work on a PR for that improvement if you approve

I understand that it might not be a perfect solution and a better approach would be to fine tune the checks for assembly architecture, bitness etc however it might be a good enough option because by default you won't be willing to load an assembly if it was already loaded via stream or there is an assembly for a different architecture/bitness that is already loaded somehow

I also understand that NServiceBus AssemblyScanner supports various options for exluding assemblies from scanning or the issue can be eliminated by moving assemblies around however from our persective we cannot always guarantee how our customers will deploy our dependencies on their environment

Hope that makes sense.

Additional Context

No response

[kentdr]

@alexeypukhov thank you for your suggestion. Given the broad use of the AssemblyScanner, we can't guarantee such a change wouldn't introduce errors or added corner cases for other customers. The exclude functionality is an extensibility point that was added to cover situations like yours and is the recommended path forward. To solve your problem you'll need to have your customers use the exclusion APIs to exclude whatever assemblies are causing the issues.

[alexeypukhov]

@kentdr Thank you for your reply. I understand that I am suggesting a kind of fundamental change in the AssemblyScanner which require thorough analysis and I totally respect your concerns however why would one want to load an assembly for a name that was already loaded especially if it may come with an application crash or potential duplicated loading? In my case the AsemblyScanner loads my net framework assembly into a customer application running in net core environment and then fail to load the net core assembly. In this case I would prefer the AssemblyScanner to skip the net framework assembly and try to load only net core assembly. I wouldn't have a problem then. Would you be willing to improve the AssemblyScanner in this direction?

Also, even if we forget about my APM dependency, if a customer loads an assembly from bytes and then AssemblyScanner will pick this assembly up and load it again, the assembly will end up being loaded twice and who knows what will happen on duplicated initialization and how much it may cost the customer to realize this.

That being said can we make this behavior opt-in versus current opt-out? Meaning by default AssemblyScanner skips the assemblies for the names that were already loaded unless a configuration is set to proceed with it. Similar to what you currently have with AssemblyScanner.ScanAssembliesInNestedDirectories for nested directories, it is turned off by default

[bording]

in my case the AsemblyScanner loads my net framework assembly into a customer application running in net core environment and then fail to load the net core assembly.

This really sounds like a deployment concern. If you're running a .NET Core application, then the .NET Framework version of the assembly shouldn't be in the bin folder in the first place.

Also, even if we forget about my APM dependency, if a customer loads an assembly from bytes and then AssemblyScanner will pick this assembly up and load it again, the assembly will end up being loaded twice and who knows what will happen on duplicated initialization and how much it may cost the customer to realize this.

This again sounds like a deployment concern. Loading an assembly from bytes is already a rather advanced/unusual thing to be doing, but if that is the case, it should be easy enough to either move the assembly out of the assembly scanner path, or add an exclusion for it.

That being said can we make this behavior opt-in versus current opt-out?

We have had that sort of API in the past, and we ended up removing it because it led to too many problems.

[alexeypukhov]

@bording thank you for chiming in. I respect your risk strategy, you are right, we are talking about edgy-edge cases here and as I said I'm totally aware of the workarounds. I am just trying to see if we can avoid back-and-forth for this tricky situation. Because if our customers suffer from this issue they are also your customers who are suffering from this issue. Unfortunately I cannot predict all possible deployments that the customers may choose and we are not always coming as a nuget dependency (where the nuget would decide the right dll to extract) but I can see situations where they may easily hit this problem unintentionally. We will definitely put a note to our documentation but if they come to you with this problem, how fast will it take your team to figure out what to do with an assembly that failed to load because the assembly with the same name is already loaded. Same with us.

My problem with the excludes is that it requires changes to the application code and for some of the large corporate customers it is usually costly to redeploy production environments. Is there a way to configure excludes through environment variables or config files?

[iskiselev]

@bording, the problem with current exclude functionality that it requires code change. In many companies APM monitor is installed and configured by some IT ream responsible for application monitoring after development team provided an application artifacts to them. Such teams usually can change application config or environment variables, but can not change application code. In our case, APM monitor installation requires extracting APM files and setting environment variables to make it be loaded by runtime. In some cases, customers may select the application folder as a place to extract our files, which will be resulted in attempt of NServiceBus to load it - with no way to suppress that behavior through any config.

[alexeypukhov]

@bording @kentdr I understand your hesitation about changing the design of AssemblyScanner global behavior so I have an alternative suggestion. I see you already filter out Microsoft assemblies by their public key tokens here: https://github.com/Particular/NServiceBus/blob/5bd9dba6677a839bb880c6c769d1b0a67a9a06fb/src/NServiceBus.Core/Hosting/Helpers/AssemblyValidator.cs#L70 Can you add our publick key token to the filter-out logic as well?

// Cisco AppDynamics .Net Agent token
"3F604D9E4F8E4985" => true,

[kentdr]

Unfortunately, as we said above, the risk of changing the AssemblyScanner and the potential introduction of edge cases to our customers is too high for us to proceed. That is true for both the original suggestion and the public token change. The public key exclusions are intended to be Microsoft's runtime assemblies only.

As stated previously, we do have exclusion APIs that cover the scenario in question. If there is no other way to avoid this scenario, you could consider building an extension that allows your customers to use the exclusion APIs without code changes. For example, you could write something that reads a file containing a list of assemblies and then calls the assembly scanner exclusion API for each one of them.