Scrutor icon indicating copy to clipboard operation
Scrutor copied to clipboard

Azure Function Decoration issues in 4.2.0

Open woodylaurence opened this issue 2 years ago • 32 comments

Using Scrutor 4.2.0 with Azure Functions seems to cause issues.

Error when running the function (locally and remotely): "Microsoft.Azure.WebJobs.Script.WebHost: Registering implementation type FunctionApp1.BasicService is not assignable to service type FunctionApp1.IService."

When downgrading to version 4.1.0 this issue is resolved.

Repository for reproducing the issue: https://github.com/woodylaurence/ScrutorAzureFunctionsExample.

woodylaurence avatar Jun 13 '22 16:06 woodylaurence

Hi @woodylaurence! 👋

That's a really strange error as those types should definitely be assignable.

I'm finding traces of this being a functions bug, e.g. https://github.com/Azure/Azure-Functions/issues/2156, could it be something related to that?

I'll have to look into it more, but if you have any more information to go by, I'd be happy to hear it 😅

khellang avatar Jun 14 '22 21:06 khellang

There's also https://github.com/Azure/azure-functions-dotnet-extensions/issues/55, which looks similar.

khellang avatar Jun 14 '22 21:06 khellang

And https://github.com/Azure/azure-functions-host/issues/7027, which focus on covariance. Still not entirely the same problem though.

khellang avatar Jun 14 '22 21:06 khellang

Hi, thank you for the report and the sample code.

I have used it to confirm this is a problem with the latest version. The Azure project you provided uses the azure-functions-host for dependency injection. This does some additional checks on the types added into the DI that were not expected, in particular this line

It is trying to match the provided implementation type to the service type however in the latest release this has been restricted to not match.

A work around for now if this is blocking you would be to alter the first line to builder.Services.AddSingleton<IService>(new BasicService()); as the additional checks are only performed when a type is provided.

DanHarltey avatar Jun 14 '22 22:06 DanHarltey

As for a fix for Scrutor itself. The quick fix is to turn ServiceDescriptor with ImplementationType into a factory this will prevent any additional type checking.

public static ServiceDescriptor WithServiceType(this ServiceDescriptor descriptor, Type serviceType) => descriptor switch
    {
        { ImplementationType: not null } => new ServiceDescriptor(serviceType, (IServiceProvider sp) => ActivatorUtilities.CreateInstance(sp, descriptor.ImplementationType), descriptor.Lifetime),
        { ImplementationFactory: not null } => new ServiceDescriptor(serviceType, descriptor.ImplementationFactory, descriptor.Lifetime),
        { ImplementationInstance: not null } => new ServiceDescriptor(serviceType, descriptor.ImplementationInstance),
        _ => throw new ArgumentException($"No implementation factory or instance or type found for {descriptor.ServiceType}.", nameof(descriptor))
    };

DanHarltey avatar Jun 14 '22 22:06 DanHarltey

Thanks for jumping on this @DanHarltey! 🙏🏻

It is trying to match the provided implementation type to the service type however in the latest release this has been restricted to not match.

What does this mean exactly? Are we able to reproduce this in a unit test?

The quick fix is to turn ServiceDescriptor with ImplementationType into a factory this will prevent any additional type checking.

This sounds like a weird fix and probably will change other behaviors and/or performance. Ideally we should figure out how to comply with the additional checks.

khellang avatar Jun 15 '22 08:06 khellang

Hi @khellang , thanks for jumping on this so quickly!

Unfortunately I don't really have much more information to go on and it sounds like Dan has a much better grasp on what might be going on than I do!

Thanks @DanHarltey for your suggestions, for now I can run 4.1.0 of Scrutor in my project, and if I have a need to update before this has been fixed, I'll look to implement your other suggestion!

woodylaurence avatar Jun 15 '22 09:06 woodylaurence

Hi,

I will submit a PR with a test that recreates the validation that the Azure Function Host DI container is applying.

I will try to explain the cause of the issue better using the example provide. The ServiceDescriptor that it getting validated and causing the error could be described as new ServiceDescriptor(new DecoratedType(typeof(IService)), typeof(BasicService)); Note the DecoratedType as the BasicService has been decorated.

The validation being applied by the DI Container is attempting to ensure that the ServiceDescriptor would be valid, that the ImplementationType can be assigned to the ServiceType. It does this by analysing the ImplementationType, getting the interfaces and base classes that make it. So for this example BasicService one has one interface IService.

The validation then performs the operation equivalent to typeof(IService) == new DecoratedType(typeof(IService))

As we have wrapped the IService type in a DecoratedType, and the equality is overloaded this returns false, failing the validation. The equality of DecoratedType is overloaded intentionally to allow the ServiceDescriptor to only resolve when it is requested by the Decorator. This means it cannot pass this test the way this is written.

I am confident the proposed fix should not cause any issues because:

  1. The ServiceDescriptor will only be resolved by the Scrutor Decorator.
  2. The previous behaviour of Scrutor was to use a very similar method and the ActivatorUtilities.CreateInstance method for these decorated ServiceDescriptors.
  3. All the Scrutor tests still pass and I have tested the Azure sample with fix in place.

DanHarltey avatar Jun 15 '22 22:06 DanHarltey

Ah, yeah, that makes sense. Thanks for the explanation!

I see that their GetImplementedTypes returns back the hierarchy of base types. I wonder if we could work around it by returning back ProxiedType from DecoratedType.BaseType instead?

https://github.com/khellang/Scrutor/blob/40320c7b962f63561aa27f83dbff43cd317ec8f1/src/Scrutor/DecoratedType.cs#L95

I haven't thought hard about what other side effects this could have, but I think it could work 😅

khellang avatar Jun 16 '22 07:06 khellang

Ah, no, it won't work. It's calling GetImplementedTypes on the implementation type, not the service type. Nevermind 😆

khellang avatar Jun 16 '22 08:06 khellang

I am confident the proposed fix should not cause any issues

Since this effectively makes all decorated registrations factory-based, I'm mainly concerned with performance as you can't really optimize a factory registration in the same way you can with type-based registrations, but it might not be a big problem.

I wonder why the host has implemented its own assignability check instead of using Type.IsAssignableFrom... 🤔

khellang avatar Jun 16 '22 08:06 khellang

I wonder why the host has implemented its own assignability check instead of using Type.IsAssignableFrom... 🤔

Yes I did wonder that as well. If they did do: serviceDescriptor.ServiceType.IsAssignableFrom(serviceDescriptor.ImplementationType); That would have worked as we proxy all the calls. It does look like they have copied that whole file from an older version of DryIoc but I am not sure about that.

As for performance I did a small test. The code is here. Scrutor did get a performance boost with the release of 4.2.0. We do lose performance by turning the type-based registration to factory registration. However the performance would still beat the previous 4.1.0 version. Although, we are talking nanoseconds here.

Method Mean Error StdDev Ratio RatioSD Gen 0 Allocated
PrProposed 674.0 ns 13.22 ns 12.98 ns 1.00 0.00 0.0935 392 B
DecorateV42 409.3 ns 3.92 ns 3.48 ns 0.61 0.01 0.0648 272 B
DecorateV41 710.3 ns 14.03 ns 27.36 ns 1.06 0.05 0.0877 368 B

I can not think of another way to pass this test that this DI container is applying. The only options I can think of are:

  1. Do not support this DI container & Azure functions.
  2. Use factory based registration and lose some performance.

On the note of performance. MS does a nice performance trick with its AddHttpClient extension. This performs the role very similarly to what Decorate does. I plan to look at it in the future. I did hack a small example in the performance test and it does make a big difference. I think if a similar technique was applied to both decorated and decorator it would be very fast. But one for future work.

DanHarltey avatar Jun 17 '22 14:06 DanHarltey

Hello all, I upgraded to 4.2.0 and am encountering a decoration issue as well. It appears that the generated ServiceDescriptor.ImplementationFactory results in a MethodBuilderInstantiation which throws a NotSupportedException when its Invoke is called.

Reverting back to 4.1.0 fixes the issue. Not sure if it's the same root cause here but wanted to mention it to see if I can assist in tracking this down further for you. 👍

Mike-E-angelo avatar Jun 18 '22 16:06 Mike-E-angelo

Hi @Mike-E-angelo,

Thank you very much for getting in touch and reporting an issue. Sorry you are having this issue, hopefully we can find the cause quickly. Would it be possible to provide a bit more detail. Maybe a stack trace? What Decorate method is causing the issue? What DI container is it, is it also an azure function?

DanHarltey avatar Jun 18 '22 20:06 DanHarltey

No problem @DanHarltey I'm a big fan of this project so I'll help where I can. :) In my case, it is not Azure Functions and I am using LightInject. I do not have a simple repro unfortunately and it might take some time for me to get to it. The best that I can surmise is that the generated ImplementationFactory is somehow producing these MethodBuilderInstantiations. I am using the generic Decorate<TInterface, TImplementation> as well. I have a lot of registrations as well, over 1200 of them. 😲

I will see if I can carve out some time to provide a reproduction for you. 👍

Mike-E-angelo avatar Jun 18 '22 20:06 Mike-E-angelo

FWIW, I was thankfully able to reproduce this with an SLN for your review and captured/moving to #175 as it sounds like a different root cause to this one.

Mike-E-angelo avatar Jun 19 '22 09:06 Mike-E-angelo

@khellang @DanHarltey This issue is blocking me from using Scrutor in an Azure function. Propose we introduce a DecorateWithFactory and TryDecorateWithFactory.
This would stop this blocker and not cause any performance issues for those users not needing functions.

For non-azure functions, continue to use Decorate and TryDecorate When we are registering with azure functions - we can use DecorateWithFactory and TryDecorateWithFactory

Or maybe easier - an optional parameter to Decorate:

Decorate(typeof(IDecoratedService), typeof(Decorator), useFactory = false);

Thoughts?

waynebrantley avatar Feb 25 '23 21:02 waynebrantley

@khellang If I create a PR for this will you accept? Optionally I could likely do a PR that makes some things you have as built-in internal something package users can replace and we could do this with no help from the package.

waynebrantley avatar Mar 03 '23 14:03 waynebrantley

I would prefer that Azure Functions fixed their 💩 instead 😅 Has anyone opened an issue over there? They (DryIoC) basically have a bug by not calling UnderlyingSystemType

khellang avatar Mar 03 '23 14:03 khellang

I see that @dadhi commented on https://github.com/seesharper/LightInject/pull/571 way back when. Maybe he's applied some fixes to DryIoc?

khellang avatar Mar 03 '23 14:03 khellang

I would rather Azure fix that too - but if that is what I have to wait on - we will all be retired! Zero chance of that. How about just let me change extensibility of your library (some internal things exposed) so that I can just do it myself without forking your library if you dont want to actually add the feature.

waynebrantley avatar Mar 03 '23 14:03 waynebrantley

It kinda depends on the extensibility points. If it's turning the entire codebase inside out, I might be opposed, but if it's just adding a hook here or there, it could work 😅 If it isn't too much work, you could just send a quick PR and then we could discuss it from there. Otherwise it's probably best to discuss it before you do a lot of work.

khellang avatar Mar 03 '23 15:03 khellang

Based on this work https://github.com/khellang/Scrutor/compare/master...DanHarltey:Scrutor:performance-example
I think all that is needed is one tiny thing: https://github.com/khellang/Scrutor/pull/194

waynebrantley avatar Mar 03 '23 15:03 waynebrantley

@khellang if you are up for the above easy change and will merge/release - I will build the full prototype of what is needed to make this work and test it...if that all works I will share that here for further review to see if you want to add or just leave in this ticket for others to implement if needed.

waynebrantley avatar Mar 03 '23 23:03 waynebrantley

@khellang Any chance you could approve the above PR to make a class public?

waynebrantley avatar Mar 13 '23 21:03 waynebrantley

@khellang I am sure you are busy - but can you look at the pr above - super simple - so I am not blocked and dont have to fork this repo to get this working? Is there any other maintainer that maybe could do it if you dont have time?

waynebrantley avatar Mar 25 '23 15:03 waynebrantley

Hey @waynebrantley! Sorry it took so long. I've pushed v4.2.2 to NuGet now 😅

khellang avatar Mar 27 '23 09:03 khellang

I seem to be having this issue with 4.2.2 - was this intended to solve it or just make it possible to solve?

phatcher avatar Sep 08 '23 10:09 phatcher

I'm also getting this with 4.1.0; I'll try and make a repo :-(

phatcher avatar Sep 08 '23 10:09 phatcher

AFAIK, it hasn't really been fixed. I think @waynebrantley fixed it himself, he just needed some minor parts of the library exposed publically.

These problems will hopefully go away after #209 is merged, but it relies on .NET 8, which isn't released until november. Plus it probably takes some time for Azure Functions to support it.

khellang avatar Sep 08 '23 10:09 khellang