Azure Function Decoration issues in 4.2.0

[woodylaurence]

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.

[khellang]

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. Azure/Azure-Functions#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]

There's also Azure/azure-functions-dotnet-extensions#55, which looks similar.

[khellang]

And Azure/azure-functions-host#7027, which focus on covariance. Still not entirely the same problem though.

[DanHarltey]

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(new BasicService()); as the additional checks are only performed when a type is provided.

[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))
    };

[khellang]

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.

[woodylaurence]

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!

[DanHarltey]

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.

[khellang]

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?

Scrutor/src/Scrutor/DecoratedType.cs

Line 95 in 40320c7

public override Type? BaseType => ProxiedType.BaseType;

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

[khellang]

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

[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... 🤔

[DanHarltey]

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.

[Mike-E-angelo]

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. 👍

[DanHarltey]

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?

[Mike-E-angelo]

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]

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.

[waynebrantley]

@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]

@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.

[khellang]

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]

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

[waynebrantley]

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.

[khellang]

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.

[waynebrantley]

Based on this work master...DanHarltey:Scrutor:performance-example
I think all that is needed is one tiny thing: #194

[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]

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

[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?

[khellang]

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

[phatcher]

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]

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

[khellang]

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.

[phatcher]

Given that they haven't released a new version of that library since 2020, I'm not holding out much hope.

I managed to get around it at the moment by making a simple factory registration, but would like to solve it properly. It's odd that it doesn't work for me in v4.1.0 as the code looks very similar to the examples, but there are a couple of additional interfaces on the classes which is my guess as to why it breaks, but I need to do a proper repo

@waynebrantley Any chance you can share your fix here, or point me at a branch?

[waynebrantley]

@phatcher Armed with the latest library and public items exposed....along with the POC code in the branch linked above I added a TryDecorate method that takes a useFactory param. This defaults to false, so nothing changes for most calls. However, if you are in Azure function - you need the factory, so you just call it with true and you will be all set.
In the end it is not a big lift or lots of code.

https://gist.github.com/waynebrantley/b4a9c3e6161c4edbc47bece6f492123a

for what it is worth - we completely left azure functions. They are just too 'different' from the rest of the world and require such special setup and configuration, it was just not worth it. Azure container apps for the win....