System.NotSupportedException Thrown in 4.2.0 Using Decorate

[Mike-E-angelo]

This is a continuation of this comment:
#171 (comment)

I have created a SLN/failing test demonstrating this issue here:
https://github.com/Mike-E-angelo/Stash/tree/master/Scrutor.Decorate

Reverting to 4.1.0 removes this issue.

Please do let me know if there is any further information that I can provide and I will assist.

Thank you for any consideration/effort you can provide towards addressing this. 🙏

[DanHarltey]

Hi,

I had a look at this today. This is an issue between Scrutor and LightInject.Microsoft.DependencyInjection.

The issue is around where LightInject.Microsoft.DependencyInjection is converting the MS registrations into LightInject registrations. It is attempting to create a factory by using reflection to create a generic method at runtime. This is using the reflection method MakeGenericMethod. This method is failing because it expects the passed types to be a RuntimeType. However, Scrutor is passing the DecoratedType which inherits from Type. This results in it returning an Emit based MethodBuilderInstantiation and the subsequent exception when calling Invoke. DecoratedType cannot inherit from RuntimeType as it is a private .Net runtime type.

I created a test which recreates this scenario, it is here

I am surprised by the number of dependency injection frameworks in use, and the large variances between them. The newly introduced DecoratedType hack is causing this issue and another. However for this issue I cannot see any way to fix this whilst keeping the DecoratedType. Given this issue and the other open issue, it might be worth reverting to the previous decoration strategy for Scrutor.

Whilst it would be disappointing to lose the new added feature around #91. I do have a couple of different ideas to look into in the future which do not use this type of DecoratedType hack.

[Mike-E-angelo]

Thank you for your investigation and explanation @DanHarltey. Am I to understand correctly that this is due to a new feature to ensure Dispose is called on all decorated components resolved via Scrutor? If it helps, I always call Dispose on the decorated/inner instance from the outer/decorating implementation, so I am not sure if that helps me here (if I understand correctly).

Ideally, there should be a way to configure this feature so that you can enable it if needed. I would suggest a new method that allows you to opt-in & enable it. This way you preserve both the existing/previous functionality as well as preserve the new feature so everyone wins. :)

[khellang]

I'm actually leaning towards calling this a bug in LightInject. Type is a perfectly valid extensibility point and there's a way to fix this issue on their side:

It's not safe to automatically assume that you've been handed a RuntimeType. If you write code that has to handle Type objects, and those objects come from any other method than the typeof operator, you have to consider the possibility that the Type you have isn’t a RuntimeType. You also need to be aware of the places where RuntimeTypes end up being required.

The Type class has a property named UnderlyingSystemType, which can be used to acquire the actual RuntimeType when you’re using a type delegator. Anywhere where you know you need a RuntimeType, you should use the UnderlyingSystemType property to retrieve it from any Type objects you’ve been given. If you call UnderlyingSystemType on a RuntimeType, it simply returns itself, since it’s already a RuntimeType.

From When is a Type not a Type?

The version with the custom type has been out for a while and no one else has screamed, so it seems to be a bit of a fringe bug. Maybe we can get them to patch it on their side? Or send a PR?

[Mike-E-angelo]

Let's bring in @seesharper and see if they can help out here. 😀

[khellang]

Created a PR at seesharper/LightInject.Microsoft.DependencyInjection#195.

[Mike-E-angelo]

Thank you very much @khellang !!! 🙏🙏🙏

[seesharper]

LightInject.Microsoft.DependencyInjection 3.4.2 is now available on NuGet with these changes.
Also updated LightInject.Microsoft.Hosting (1.3.1) so that it pulls in 3.4.2 by default

Again, thank you all for contributing 👍❤️

[khellang]

Amazing. Thanks @seesharper! 🙏😘

[Mike-E-angelo]

Sorry to be a downer here @khellang / @seesharper but I upgraded to the latest versions all around and I am still getting the error in the provided solution above. Is there something I am overlooking here?

[khellang]

Hmm. @Mike-E-angelo is that the exact same exception as before? 🤔

[Mike-E-angelo]

Good question, @khellang. Here is using LightInject.Microsoft.Hosting 1.3.0:

System.NotSupportedException
Specified method is not supported.
   at System.Reflection.Emit.MethodBuilderInstantiation.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.CreateFactoryDelegate(ServiceDescriptor serviceDescriptor)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.CreateServiceRegistrationForFactoryDelegate(ServiceDescriptor serviceDescriptor, Scope rootScope)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.CreateServiceRegistration(ServiceDescriptor serviceDescriptor, Scope rootScope)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.<>c__DisplayClass1_0.<RegisterServices>b__0(ServiceDescriptor d)
   at System.Linq.Enumerable.SelectIListIterator`2.ToList()
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.RegisterServices(IServiceContainer container, Scope rootScope, IServiceCollection serviceCollection)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.CreateServiceProvider(IServiceContainer container, IServiceCollection serviceCollection)

And here is using LightInject.Microsoft.Hosting 1.3.1:

System.NotSupportedException
Specified method is not supported.
   at System.Reflection.Emit.MethodBuilderInstantiation.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.CreateFactoryDelegate(ServiceDescriptor serviceDescriptor)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.CreateServiceRegistrationForFactoryDelegate(ServiceDescriptor serviceDescriptor, Scope rootScope)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.CreateServiceRegistration(ServiceDescriptor serviceDescriptor, Scope rootScope)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.<>c__DisplayClass1_0.<RegisterServices>b__0(ServiceDescriptor d)
   at System.Linq.Enumerable.SelectIListIterator`2.ToList()
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.RegisterServices(IServiceContainer container, Scope rootScope, IServiceCollection serviceCollection)
   at LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.CreateServiceProvider(IServiceContainer container, IServiceCollection serviceCollection)

They appear to be the same, although I could be doing something wrong? Any verification on your end would be greatly appreciated.

[seesharper]

@Mike-E-angelo Could you pull in LightInject.Microsoft.DependencyInjection 3.4.2 explicitly and see if it works then?

[seesharper]

I think I see what is wrong. LI.MI.Hosting pulls in 3.4.1 which I at 12PM last night thought was the latest version.😃 I'll update LI.MI.Hosting to pull in 3.4.2 which contains the fix from @khellang 👍

[seesharper]

@Mike-E-angelo LIghtInject.Microsoft.Hosting 1.3.2 is now out on NuGet. Could you try that? 😃

[khellang]

Boom! 💥

[khellang]

Thanks again @seesharper! 😁

[Mike-E-angelo]

I appreciate both of you being super responsive and engaging here. Thank you! The new version does seem to solve the problem captured in the solution, but now it appears it has moved onto a new one. 😁

When I open LIghtInject.Microsoft.Hosting 1.3.2 + Scrutor 4.2.0 I see the following:

 ---> System.NotSupportedException: Specified method is not supported.
System.Reflection.Emit.MethodBuilderInstantiation.GetParameters()
LightInject.Emitter.Emit(OpCode code, MethodInfo methodInfo)
LightInject.ServiceContainer.EmitLifetime(ServiceRegistration serviceRegistration, Action<IEmitter> emitMethod, IEmitter emitter)
LightInject.ServiceContainer+<>c__DisplayClass165_0.<CreateEmitMethodWrapper>b__0(IEmitter ms)
LightInject.ServiceContainer+<>c__DisplayClass165_0.<CreateEmitMethodWrapper>b__0(IEmitter ms)
LightInject.ServiceContainer.CreateDynamicMethodDelegate(Action<IEmitter> serviceEmitter)
LightInject.ServiceContainer.CreateDelegate(Type serviceType, string serviceName, bool throwError)
LightInject.ServiceContainer.CreateDefaultDelegate(Type serviceType, bool throwError)
LightInject.Scope.GetInstance(Type serviceType)
Scrutor.DecorationStrategy+<>c__DisplayClass8_0.<TypeDecorator>b__0(IServiceProvider serviceProvider)
LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions+<>c__DisplayClass10_0<T>.<CreateTypedFactoryDelegate>b__0(IServiceFactory serviceFactory)

Please let me know if you are able to spot the problem here or if I need to track this down for you and add a new test to that solution.

[khellang]

Yeah, it's basically the same problem. I wonder what happens if we just use the UnderlyingSystemType when converting to LightInject ServiceRegistration? This entire thing was done to work around limitations in MS.Ext.DI, which LightInject might not have.

The other alternative is to go through LightInject and call UnderlyingSystemType in the various places it's needed. It would be nice to verify a solution before going down another dead end, though 😅

[seesharper]

The quick fix would probably be as @khellang is suggesting here. Make sure that we use UnderlyingSystemType for all registrations. It would however be really nice to have a failing test, preferably in the LightInject.Microsoft.DependencyInjection repo. @Mike-E-angelo Do you have a "simplest path to problem" here for this latest exception? 🙂

The long term solution would be to also fix this in LightInject 👍

[Mike-E-angelo]

Do you have a "simplest path to problem" here for this latest exception? 🙂

I was afraid you were going to ask that 😅 Not at the moment but I will try to get something for you here today. This is an especially tricky bugger because for whatever reason Visual Studio optimizes all type data and it's really difficult to know which registration it's failing on. I think I had mentioned there are over 1200 registrations in my application and made it to 500-something last time. We'll see how far I got this time. :P

[seesharper]

@khellang DecoratedType is used to sort of keep track of whether a given type is decorated, right? If we simple coerce the servicetype to its UnderlyingSystemType, could it bring trouble later?. I'm thinking stack overflow if Scrutor uses a type check to determine if the service should be decorated? I haven't studied the code in detail though 😃

[seesharper]

@khellang @Mike-E-angelo
Update. I cloned Scrutor and changed TestBase to return LightInjectServiceProvider and that caused 5 tests to fail.
I'm going to start there

[khellang]

@seesharper Yeah, it's simply a wrapper around Type to force reference equality. This lets us keep the existing registrations for decorated types in the container to track lifetime and disposal etc., without having stack overflows.

It could bring issues if the MS.Ext.DI adapter simply calls UnderlyingSystemType, yes. I just don't know enough about the container to tell 😅 That's why we should check to make sure we don't attempt another half-working patch 😜

Update. I cloned Scrutor and changed TestBase to return LightInjectServiceProvider and that caused 5 tests to fail.
I'm going to start there

That's a great idea!

[seesharper]

@khellang @Mike-E-angelo I had to fix this in LightInject. So this quicky rippled through all packages 😃. @Mike-E-angelo . You should get away with the new version of LightInject.Microsoft.Hosting which is now at version 1.3.3. This pulls in LightInject.Microsoft.DependencyInjection 3.4.3 which in turn pulls in LightInject 6.5.2.

Still possibly more work needed in LightInject as I have no tests for this right now. Late friday quick-fix. What could possibly go wrong. @khellang I might borrow DecoratedType into LightInject to write some tests if that is okay with you ? 😃

[khellang]

Haha, Friday night deployments are the best 😜 Sure, borrow all you want 👍

[Mike-E-angelo]

Woohoo! I can confirm all my 1200+ registrations now load without error now! 🎉 THANK YOU both @khellang and @seesharper for being so quick and collaborative around this and getting out a fix. It is very much appreciated!!!

[seesharper]

FYI: The fix from @khellang was still needed since we create the service factory in the adapter.👍 And with excellent feedback from @Mike-E-angelo I'd say this is joint effort at its best. 👍😃.

@khellang Would it make sense to publish the tests for Scrutor as a NuGet package so that other Ms.Ex.Di adapters can include tests for Scrutor? 😀

[khellang]

@khellang Would it make sense to publish the tests for Scrutor as a NuGet package so that other Ms.Ex.Di adapters can include tests for Scrutor? 😀

I guess. The library was always meant as a thin layer on top of MS.Ext.DI, but over time, as we've tried to squeeze as much as we can out of the built-in container, I guess we've reached a point where we're starting to feel the pain of the conforming container pattern 😅