Could not load type 'SqlGuidCaster' from assembly Microsoft.Data.SqlClient

[Alerinos]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

Error:

System.Reflection.ReflectionTypeLoadException: „Unable to load one or more of the requested types.
Could not load type 'SqlGuidCaster' from assembly 'Microsoft.Data.SqlClient, Version=5.0.0.0, Culture=neutral, PublicKeyToken=23ec7fc2d6eaa4a5' because it contains an object field at offset 0 that is incorrectly aligned or overlapped by a non-object field.”

Code:

    public static IEnumerable<Assembly> GetAssembliesWithTypesImplementing<T>()
    {
        return AppDomain.CurrentDomain.GetAssemblies()
            .SelectMany(assembly => assembly.GetTypes())
            .Where(type => !type.IsAbstract && typeof(T).IsAssignableFrom(type))
            .Select(type => type.Assembly)
            .Distinct();
    }

line:
assembly.GetTypes()

The error appeared after upgrading from .net 7 to .net 8 preview 1.
I searched and I don't have such Microsoft.Data.SqlClient package installed

Expected Behavior

No response

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

No response

Anything else?

No response

[Wraith2]

Even if you aren't using it directly you must have a transitive reference to Microsoft.Data.SqlClient (probably through EFCore) in order to have it present in the assembly list. So can you provide the minimal reproduction that can be debugged which exhibits this problem?

[lcheunglci]

Hi @Alerinos Could you provide us with a sample repro? The provided code snippet is not enough information for us to investigate.

[Alerinos]

@Wraith2 @lcheunglci
I was able to reproduce the problem, here is the repo:
https://github.com/Alerinos/SQLError
Error:

[Alerinos]

@lcheunglci Let me know if there is a problem, if so is there a quick workaround? The problem occurs only on .net 8 in .net 7 everything is fine.

[Wraith2]

Ok. that's interesting. The problem isn't in SqlClient it's to do without how object layouts are computed in the Jit. It looks like a class that's laid out safely in 7 is now not safe in 8. Since 8 is still in very early preview this isn't a pressing problem but i'll see if i can find the people who might know what to do.

[Wraith2]

@David-Engel @MichalPetryka this is a result of the runtime optimization in dotnet/runtime#72549 which came from a suggestion on my new apis proposal dotnet/runtime#51836 (comment)

We've got a situation where the runtime was previously using struct SqlGuid{ byte[] _value; } and that has changed to struct SqlGuid{ Guid? _value; }. overlaying a struct with ref types in the same position was unsafe and icky but supported. In NET8 this changes to trying to overlay a value type and a ref type which is entirely illegal and causes the type loader to abort the assembly load.

I'm not sure of a good way to fix this. We can't detect the right pattern to use at runtime because we cannot have the old pattern in the assembly, the type loader will reject it.
If we choose to fix this only for net8 then we must have a day 1 build of SqlClient available to users when net8 launches or we prevent early adopters from using EFCore and NET8 which is a very poor PR look for this library.

[David-Engel]

@Wraith2
Well that's unfortunate. I thought we would be safe dynamically determining if we can call the new APIs at runtime, leaving the old SqlGuidCaster in there. If we create a .NET 8 target in the package to split it out, that would fix the problem, right? I had planned for us to do that later this year, but we might have to prioritize it sooner.

[Wraith2]

As long as no-one tries to load the non-8 assembly on 8 it should be ok. That would mean you can't take a <8 framework dependent install and run it on >= 8 but I'm not sure how important that scenario is.

I'd be nice to see if there's some way to have the <8 assembly work on 8 even if it was slow but i don't know how possible or performant that would be.

[David-Engel]

I forgot we left the hacky way in for .NET Core. It was significantly faster than the reflection method for the SqlTypeWorkarounds. So we have a couple of choices. Leave the hacks for < 8 and prioritize creating an 8+ assembly, or switch to reflection and take the perf hit to retain .NET 8 compatibility. Then we can add an 8+ assembly a little later and regain perf (on 8+ only) via the new SqlTypes APIs.

Edit: It was actually that the internal methods were stripped from .NET during the build and not available to even call via reflection. So perf would be even worse than I thought via the existing public APIs.

#1647

[tannergooding]

You could also detect which is applicable once via reflection (or statically based on TFM), caching it in a static readonly bool (this becomes a JIT time constant in Tier 1 code after rejit). -- Static detection based on TFM (e.g. using the pre-defined #if NET8_0_OR_GREATER) would also work, but wouldn't cover any "roll forward" considerations for users pulling in the .NET 7 version of the binary on .NET 8. That may or may not be acceptable

You can then use that + Unsafe.As<TFrom, TTo> to perform the "correct" conversion based on which is correct. You then don't need to rely on a union which may cause a TypeLoadException.

Of course, if there is a need for a fast/efficient API I'd really recommend opening an API proposal so this can be done safely and without any reflection or unsafe hacks: https://github.com/dotnet/runtime/issues/new?assignees=&labels=api-suggestion&template=02_api_proposal.yml&title=%5BAPI+Proposal%5D%3A+

The general API review process is detailed here: https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md

[David-Engel]

Of course, if there is a need for a fast/efficient API I'd really recommend opening an API proposal so this can be done safely and without any reflection or unsafe hacks: https://github.com/dotnet/runtime/issues/new?assignees=&labels=api-suggestion&template=02_api_proposal.yml&title=%5BAPI+Proposal%5D%3A+

We did that part last year and the new APIs made it into .NET 7. Now we need to switch to them and account for this subsequent optimization of the internal SqlGuid references in .NET 8.

[Wraith2]

We did that part last year and the new APIs made it into .NET 7. Now we need to switch to them and account for this subsequent optimization of the internal SqlGuid references in .NET 8.

All done in the linked PR, #1934

[David-Engel]

@roji Just a heads up. This may have an impact on EF8 if EF8 wants to take a dependency on MDS 5.1. We need to figure out a way to ensure MDS 5.1 remains compatible with .NET 8.

[roji]

Thanks for the heads up, /cc @ajcvickers.

FYI there's a pretty good chance EF 8.0 will end up targeting net8.0; as long as MDS5.1 works on that things should be OK.

[Wraith2]

We need to figure out a way to ensure MDS 5.1 remains compatible with .NET 8.

Just backport my PR. It removes the SqlGuidCaster because it was unused. No other type has changed. In general if the user hadn't used reflection to discover the type the error would never have occurred so people on current 5.1 may be fine as long as they don't try to interact with our internal types through reflection.

[David-Engel]

Just backport my PR. It removes the SqlGuidCaster because it was unused. No other type has changed. In general if the user hadn't used reflection to discover the type the error would never have occurred so people on current 5.1 may be fine as long as they don't try to interact with our internal types through reflection.

Maybe I'm not remembering correctly, but I'm pretty sure SqlGuidCaster is still used against .NET 6 and that's the only .NET (Core) library we include in the 5.1 package. Good to know it's only reflection that has an issue, though.

[Wraith2]

On netcore we're using the SqlGuid(Guid) ctor because we can use the Guid(ReadOnlySpan<byte>) ctor to avoid the allocation. We aren't using the SqlGuidCaster in netcore at all. It can just be deleted.

[tippmar-nr]

This same issue is present in System.Data.SqlClient as I reported here.

Should we expect the fix for this issue to be backported from Microsoft.Data.SqlClient to System.Data.SqlClient?

[sl-yannavondo]

Just seeing 5.2.0 is releasing ;)

[noahkiss]

I’ll be interested to see if this release resolves the issue for everyone. Since this has been marked fixed for the better part of a year, but wasn’t.

[jeffjulian]

Early review of the 5.2.0 version (plus System.Data.SqlClient 4.8.6) resolves the issue I had with .NET 8.0 and SqlGuidCaster throwing up.

[mnikolovski]

Or use Signals to cut through the slack.

[rodion-m]

This bug is relevant for a version 5.2.1. It works on MacOS, but in Windows it shows an error:

  Message: 
System.Reflection.ReflectionTypeLoadException : Unable to load one or more of the requested types.
Could not load type 'SqlGuidCaster' from assembly 'System.Data.SqlClient, Version=4.6.1.6, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' because it contains an object field at offset 0 that is incorrectly aligned or overlapped by a non-object field.

  Stack Trace: 
RuntimeModule.GetTypes(RuntimeModule module)
RuntimeModule.GetTypes()

Update: We used System instead of Microsoft version. @Wraith2 Thanks a lot for clarifying it!

[Wraith2]

In the error message you can see that it says "System.Data.SqlClient", that means you're loading the system version of the library which as has been discussed in this thread will not be updated. Stop doing that.

[ltrzesniewski]

Here's an extension method you can use instead of Assembly.GetTypes(), if it can help:

public static Type[] GetLoadableTypes(this Assembly assembly)
{
    try
    {
        return assembly.GetTypes();
    }
    catch (ReflectionTypeLoadException ex)
    {
        return ex.Types.Where(t => t is not null).ToArray()!;
    }
}

[aaron777collins]

For anyone who has this issue but is also worried about #2378, v5.2.0-preview5.24024.3 supposedly solves both issues on .net 8. I'll keep y'all posted how it goes.

[ErikEJ]

@aaron777collins How about 5.2.1 ?

[aaron777collins]

@aaron777collins How about 5.2.1 ?

Supposedly the connectivity issue is even worse on 5.2.1. It's not a solution since the apps we have are under high load.