dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.98k stars 4.66k forks source link

Inconsistent behavior with Type.GetType compared to .NET Framework #12376

Closed bording closed 4 years ago

bording commented 5 years ago

Given the the following program:

using System;

namespace GetTypeRepro
{
    class Program
    {
        static void Main(string[] args)
        {
            //Project is referencing Library.dll via PackageReference

            //Uncomment this to see it work in netcoreapp3.0 (and netcoreapp2.2 if CopyLocalLockFileAssemblies is uncommented in the csproj)
            //System.Reflection.Assembly.LoadFrom("GetTypeRepro.dll");

            var type1 = Type.GetType("Library.Class1, Library, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null", true); // < package version
            var type2 = Type.GetType("Library.Class1, Library, Version=2.0.0.0, Culture=neutral, PublicKeyToken=null", true); // == package version
            var type3 = Type.GetType("Library.Class1, Library, Version=3.0.0.0, Culture=neutral, PublicKeyToken=null", true); // > package version
        }
    }
}

There appears to be inconsistent behavior between the .NET Framework and .NET Core.

When run on net472, all three Type.GetType calls return the type from the referenced package.

However, when run on netcoreapp2.2 or netcoreapp3.0, the first two return the type as expected, but the third call throws an exception instead:

   System.IO.FileLoadException: Could not load file or assembly 'Library, Version=3.0.0.0, Culture=neutral, PublicKeyToken=null'. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
File name: 'Library, Version=3.0.0.0, Culture=neutral, PublicKeyToken=null'
   at System.RuntimeTypeHandle.GetTypeByName(String name, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMarkHandle stackMark, IntPtr pPrivHostBinder, Boolean loadTypeFromPartialName, ObjectHandleOnStack type, ObjectHandleOnStack keepalive)
   at System.RuntimeTypeHandle.GetTypeByName(String name, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMark& stackMark, IntPtr pPrivHostBinder, Boolean loadTypeFromPartialName)
   at System.RuntimeType.GetType(String typeName, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMark& stackMark)
   at System.Type.GetType(String typeName, Boolean throwOnError)
   at GetTypeRepro.Program.Main(String[] args) in C:\Code\GetTypeRepro\GetTypeRepro\Program.cs:line 18

One discovery I've made while investigating this is that the exception can be avoided if the referenced assembly is located next to the application assembly. This is the default in netcoreapp3.0, and can be achieved on netcoreapp2.2 as well be setting CopyLocalLockFileAssemblies to true in the project file.

Once that is done, if you first call Assembly.LoadFrom on the application assembly, then the third Type.GetType stops throwing the exception and instead returns the type from the referenced assembly just like net472 does. This doesn't prevent the exception if the assembly is still being referenced from the NuGet package cache, though.

I have a runnable repro available at https://github.com/bording/GetTypeRepro

This behavior seems very strange to me. Have I come across a bug, or is this somehow working as intended?

RussKeldorph commented 5 years ago

@jeffschwMSFT

jeffschwMSFT commented 5 years ago

Thanks for reaching out @bording .

However, when run on netcoreapp2.2 or netcoreapp3.0, the first two return the type as expected, but the third call throws an exception instead:

The only sanity check that the binder does is ensuring that the version number is not less than what was requested. In this case 3.0.0 was requested but we only have 2.0.0 to provide. That is expected.

//Uncomment this to see it work in netcoreapp3.0 (and netcoreapp2.2 if CopyLocalLockFileAssemblies is uncommented in the csproj) //System.Reflection.Assembly.LoadFrom("Repro.dll");

What is repro.dll? What is in that assembly?

bording commented 5 years ago

What is repro.dll? What is in that assembly?

Sorry, that's a typo. It should have been GetTypeRepro.dll, which is the executing program assembly.

I've corrected the issue text and updated my repro code.

bording commented 5 years ago

The only sanity check that the binder does is ensuring that the version number is not less than what was requested. In this case 3.0.0 was requested but we only have 2.0.0 to provide. That is expected.

Why is that expected though? That is different than how net472 operates, and as I've demonstrated here, that's not even consistent within .NET Core.

bording commented 5 years ago

@jeffschwMSFT Were you able to take a look at my repro code? Why does the LoadFrom call alter the GetType behavior?

sdmaclea commented 5 years ago

@bording

Why does the LoadFrom call alter the GetType behavior?

Assembly.LoadFrom on .NET Core, is installing an event handler to look for dependent assemblies next to the Loaded assembly to resolve its dependencies. Apparently that is circumventing the Version check.

sdmaclea commented 5 years ago

@jkotas @vitek-karas

We could probably "fix" the LoadFrom resolve handler to not ignore the version in 3.0, but this would be a behavior change. Opinions?

bording commented 5 years ago

We could probably "fix" the LoadFrom resolve handler to not ignore the version in 3.0, but this would be a behavior change. Opinions?

So this would be making .NET Core always have different behavior than the .NET Framework?

I'm curious as to why it's okay for the .NET Framework to work the way it does, but makes that undesirable behavior for .NET Core.

jeffschwMSFT commented 5 years ago

Sorry for the delay @bording.

Thanks @sdmaclea.

We could probably "fix" the LoadFrom resolve handler to not ignore the version in 3.0, but this would be a behavior change. Opinions?

Assembly.LoadFrom has a hand crafted set of expectations coming from desktop, even though this is seemingly inconsistent, I would think we should keep it as it is.

Why is that expected though? That is different than how net472 operates, and as I've demonstrated here, that's not even consistent within .NET Core.

.NET Core has a vastly simplified binding model. The version check seems common sense and good hygiene, so we have kept it.

bording commented 5 years ago

.NET Core has a vastly simplified binding model. The version check seems common sense and good hygiene, so we have kept it.

The thing that I'm still not sure I agree with here is that in this scenario, .NET Core is the one being more restrictive, despite it having the simpler binding model. .NET Framework is perfectly happy to return a type for the third GetType call, but .NET Core throws an exception.

jeffschwMSFT commented 5 years ago

I appreciate having the simplified repro to help demonstrate the underlying difference, but was the original reason to notice that we fail to load assemblies with a higher version than requested?

jkotas commented 5 years ago

.NET Core is the one being more restrictive,

.NET Framework did not respect assembly versions some of the time. It was a bug in .NET Framework that is fixed in .NET Core.

bording commented 5 years ago

I appreciate having the simplified repro to help demonstrate the underlying difference, but was the original reason to notice that we fail to load assemblies with a higher version than requested?

@jeffschwMSFT We had a customer open a support case about why they were seeing a failure in their system when they were moving from .NET Framework to .NET Core, and I tracked it down to this root cause.

It's basically a serialization scenario where two applications were using different versions of a contract assembly to pass messages. The sender app was using a newer version of the assembly than the receiver app, so when they switched to .NET Core, it stopped working (unless they had the contract assembly in the local bin folder).

Our code ends up calling LoadFrom as part of an assembly scanning process, so that's how I discovered that calling it will alter the behavior to match the .NET Framework behavior.

jeffschwMSFT commented 5 years ago

That is a common scenario, particularity having multiple versions of an assembly that only differ based on version. Do you have enough information to help get it back to a working state?

bording commented 5 years ago

That is a common scenario, particularity having multiple versions of an assembly that only differ based on version.

I agree, which is why I surprised by the difference in behavior here. I still feel like the .NET Framework behavior is more desirable, especially when you consider that you can make .NET Core match the .NET Framework behavior with the LoadFrom call.

If .NET Core is going to throw in some cases, it really does seem like it should do it in all cases. The fact that LoadFrom alters the behavior made this much more difficult to debug than it should have been. It took me a while to figure out why my simple repro wasn't matching the behavior I was seeing in our product, until I finally discovered that LoadFrom was the key.

However, if your position is that this is all working as intended and nothing is going to change, then I have some ideas on how we can change our code to work around this behavior on .NET Core and keep things consistent.

jkotas commented 5 years ago

Assembly.LoadFrom is very confusing API, both in .NET Framework and .NET Core. One thing we should do here is to add a note to the docs that .NET Core behavior is best-effort simulation of .NET Framework behavior, and discourage its use.

how we can change our code to work around this behavior on .NET Core and keep things consistent.

I would recommend to stop using Assembly.LoadFrom.

bording commented 5 years ago

I would recommend to stop using Assembly.LoadFrom.

I wasn't considering removing LoadFrom usage. What is the alternative when you have a file path and need to load the assembly from disk?

jkotas commented 5 years ago
bording commented 5 years ago

The library in question is multi-targeted for net452 and netstandard2.0, so that doesn't seem like a viable option.

vitek-karas commented 5 years ago

If you want the "ignore version" behavior (which I would not recommend as it may lead to other issues if the assemblies are actually different), you can achieve this by adding a handler to the AppDomain.CurrentDomain.AssemblyResolve and use Assembly.Load(simplename) (stripping the version information from the assembly name). Such code should work the same on core and framework.

You might be able to use Assembly.LoadFile instead, but that comes with its own set of specifics as it isolates the loaded assembly. It does not install any special resolution handlers, so it should not have the versioning issue mentioned above.

The cleanest solution would be to have slightly different code for framework and core, as the binding systems are just too different to behave exactly the same in all cases.