Fix for issue #54500: Middleware keyed dependency injection #55722

NicoBrabers · 2024-05-14T23:04:21Z

Middleware keyed dependency injection (fix for issue #54500)

You've read the Contributor Guide and Code of Conduct.
You've included unit or integration tests for your change, where applicable.
You've included inline docs for your change, where applicable.
There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

Added Microsoft.Extensions.DependencyInjection to Microsoft.AspNetCore.Http.Abstractions and utilized the ActivatorUtilities it provides to obtain a middleware instance. Also changed the ReflectionMiddlewareBinder to be able to handle keyed injection.

Fixes #54500

…ction to Microsoft.AspNetCore.Http.Abstractions and utilized the ActivatorUtilities it provides to obtain a middleware instance. Also changed the ReflectionMiddlewareBinder to be able to handle keyed injection.

NicoBrabers · 2024-05-15T09:11:30Z

@NicoBrabers please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
@dotnet-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"
Contributor License Agreement

@dotnet-policy-service agree company="Progresity"

gfoidl

Two small nits.

src/Http/Http.Abstractions/src/Extensions/UseMiddlewareExtensions.cs

src/Http/Http.Abstractions/test/UsePathBaseExtensionsTests.cs

…new lines) based on pull-request feedback.

amcasey · 2024-05-15T20:53:30Z

FYI @benjaminpetit @captainsafia

BrennanConroy · 2024-05-15T22:00:14Z

src/Http/Http.Abstractions/src/Extensions/UseMiddlewareExtensions.cs

-                methodArguments[i] = GetService(serviceProvider, parameters[i].ParameterType, methodInfo.DeclaringType!);
+                var parameter = parameters[i];
+
+                var hasServiceKey = TryGetServiceKey(parameter, out object? key);


This is going to happen on every single request, probably worth moving outside of the factory method since the data is the same for every single call.

Hi Brennan,

Thanks for your valuable feedback.

I've made some changes to reflect the feedback. I now precompute and cache the results for each parameter.

@BrennanConroy, I could eliminate the condition check (hasServiceKey ? GetKeyedService(...) : GetService(...)) as wel by compiling the method calls into delegates, and calling to the delegate directly within the factory. We'd probably be talking about a microsecond though. The code might become slightly less readable though.

What's your opinion on the matter?

… parameter within the ReflectionFallback.

BrennanConroy · 2024-05-17T18:29:50Z

src/Http/Http.Abstractions/src/Extensions/UseMiddlewareExtensions.cs

@@ -209,19 +210,69 @@ public RequestDelegate CreateMiddleware(RequestDelegate next)
            }
        }

+        // Performance optimization: Precompute and cache the results for each parameter
+        var parameterData = new (bool hasServiceKey, object? key, Type parameterType, Type declaringType)[parameters.Length];


You only really need to store an array of object? key. The other properties are already being captured by the closure and the check for whether to get a keyed service is key == null ? GetService : GetKeyedService.
https://github.com/dotnet/runtime/blob/26bcc1d12f895d654e24f7cbcfed466b6a34e65f/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs#L381

@benjaminpetit should we be throwing if null is passed in for key in FromKeyedServices?

@BrennanConroy, I'll change the logic according to your advice.

The reason as to why I've precomputed and cached the parameter- and declaringType is due to the fact that these are actually method calls to get the property value which will happen every time the factory is used – twice since we'd be retrieving two values. As well as obtain the parameter from the parameters array. Since using the tuple provides direct memory access, I figured it would be performing better for high throughput applications.

I love improving perf, so appreciate you thinking about that 😃
But we probably shouldn't prematurely optimize without measuring. You can try it out in https://github.com/dotnet/aspnetcore/tree/main/src/Http/Http.Abstractions/perf/Microbenchmarks if you're up for it.

@BrennanConroy, totally up for it! Tomorrow though, I need some sleep first 🥱

@BrennanConroy, I've added some benchmarks, as well as the possible optimizations. It seems that precomputing and caching all the parameters is the most optimized option.
benchmarks.txt

I've also introduced a new optimization which would cache a delegate based on the GetService or GetKeyedService method call, however, calling to the cached delegate itself seems to have more overhead than the key check and direct access to the method.

Based on these benchmarks, should we go with the optimization to cache all the parameters rather than just the key? What's your opinion on the matter?

@BrennanConroy I'm wondering about your follow up on the above.

…ach parameter within the ReflectionFallback, removed the parameterType and declaringType from the precomputed cache.

captainsafia · 2024-05-17T22:07:57Z

src/Http/Http.Abstractions/src/Extensions/UseMiddlewareExtensions.cs

+    {
+        if (parameterInfo.CustomAttributes != null)
+        {
+            foreach (var attribute in parameterInfo.GetCustomAttributes(true))


parameterInfo.OfType<FromKeyedServicesAttribute>().FirstOrDefault() might be more straightforward here.

@captainsafia, I've taken your feedback into account and changed the method to be more straightforward. Also changed the Inherit parameter from true to false, to meet the ActivatorUtilities from the Microsoft.Extensions.DependencyInjections.Abstractions package.

https://github.com/dotnet/runtime/blob/0540ea54f3ab96ed02e22e242fe9064cbda2a63c/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs#L416

https://github.com/dotnet/runtime/blob/0540ea54f3ab96ed02e22e242fe9064cbda2a63c/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs#L749

https://github.com/dotnet/runtime/blob/0540ea54f3ab96ed02e22e242fe9064cbda2a63c/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs#L532C3-L532C41

@captainsafia, @BrennanConroy, the changes within this pull-request target main, and thus the .NET 9.0 version. If I'd want the changes to be available in the latest .NET 8.x.x version as well, what would the appropriate steps for me to take?

@captainsafia, @BrennanConroy, any follow-up on my question above?

I don't think this is a backport candidate. This change is a feature, not fixing a major bug/regression introduced in 8.0.

src/Http/Http.Abstractions/src/Extensions/UseMiddlewareExtensions.cs

BrennanConroy

The issue mentions adding support for constructor injection as well. Could you add a test for that?

src/Http/Http.Abstractions/src/Extensions/UseMiddlewareExtensions.cs

src/Http/Http.Abstractions/test/UseMiddlewareTest.cs

BrennanConroy · 2024-05-17T23:16:12Z

src/Http/Http.Abstractions/src/Extensions/UseMiddlewareExtensions.cs

@@ -209,19 +210,69 @@ public RequestDelegate CreateMiddleware(RequestDelegate next)
            }
        }

+        // Performance optimization: Precompute and cache the results for each parameter
+        var parameterData = new (bool hasServiceKey, object? key, Type parameterType, Type declaringType)[parameters.Length];


I love improving perf, so appreciate you thinking about that 😃
But we probably shouldn't prematurely optimize without measuring. You can try it out in https://github.com/dotnet/aspnetcore/tree/main/src/Http/Http.Abstractions/perf/Microbenchmarks if you're up for it.

…d added the NotNullWhen(true) attribute. Also changed the Inherit param from true to false when obtaining the custom attribute FromKeyedServicesAttribute.

…test. Added Benchmark(s) for the ReflectionFallback method.

BrennanConroy · 2024-06-04T17:15:48Z

The microbenchmark essentially duplicates the code in UseMiddleware. While that's fine for a one-off run, it's not great from a maintainability standpoint as we'd need to modify both the microbenchmark and the UseMiddleware code any time we make a change. I'm wondering if we can find a way to toggle whether we use reflection or not so that the microbenchmark just calls the UseMiddleware code directly.

BrennanConroy · 2024-07-22T18:35:55Z

Hey @NicoBrabers we're starting to wrap up 9.0, would love for this change to make it but not a fan of merging the microbenchmark code as-is (see #55722 (comment)). Could we remove it and leave everything else? At a minimum we have this PR history to show the microbenchmarking 😃

NicoBrabers · 2024-07-22T19:49:35Z

Hi @BrennanConroy, I've bought a new house, and I've just been away on a holiday, hence I haven't been able to further improve on the microbenchmarks. Would love to continue that in the future.

I agree that for now it's best to remove the benchmarks and wrap it all up! Will do so tomorrow 👍🏼. I'll make sure to tag you after the changes have been made and pushed 👍🏼

BrennanConroy · 2024-07-22T19:56:39Z

Awesome thanks!

I've bought a new house

And congrats 😃

NicoBrabers · 2024-07-23T19:55:14Z

Awesome thanks!

I've bought a new house

And congrats 😃

Thank you @BrennanConroy!

I've removed the microbenchmark code for now, the changes have been pushed.

BrennanConroy · 2024-07-23T21:17:48Z

The issue mentions adding support for constructor injection as well. Could you add a test for that?

Looks like you didn't get around to this? Unless I just missed it 😆

NicoBrabers · 2024-07-24T07:18:43Z

The issue mentions adding support for constructor injection as well. Could you add a test for that?

Looks like you didn't get around to this? Unless I just missed it 😆

Hi @BrennanConroy, might have missed that comment, or forgot about it later on. Will check it a.s.a.p.

…pport unit-tests.

NicoBrabers · 2024-07-24T09:14:55Z

The issue mentions adding support for constructor injection as well. Could you add a test for that?

Looks like you didn't get around to this? Unless I just missed it 😆

Hi @BrennanConroy, I've added additional unit-tests to test the constructor injection support.

BrennanConroy · 2024-07-24T16:10:00Z

src/Http/Http.Abstractions/src/Resources.resx

+  <data name="Exception_NoServiceRegistered" xml:space="preserve">
+    <value>No service for type '{0}' has been registered.</value>
+  </data>
+  <data name="Exception_ActivateMiddlewareNoService" xml:space="preserve">


We shouldn't add resource strings that are only used by tests.

Ah, agreed, I'll remove it from the resource. Shall I add it as a constant to the testmethod?

Inline is fine

I've pushed the changes 👍

src/Http/Http.Abstractions/test/UseMiddlewareTest.cs

Co-authored-by: Brennan <brecon@microsoft.com>

…on check in test method..

BrennanConroy · 2024-07-24T20:38:05Z

src/Http/Http.Abstractions/src/Resources.resx

+  <data name="Exception_KeyedServicesNotSupported" xml:space="preserve">
+    <value>This service provider doesn't support keyed services.</value>
+  </data>
+  <data name="Exception_NoServiceRegistered" xml:space="preserve">


Sorry, there is this one too 😢

Hehe, don't sweat it, should've checked it. Will run by all resources 👍

…on check in test method.

BrennanConroy · 2024-07-25T16:09:51Z

Thanks @NicoBrabers !

NicoBrabers · 2024-07-25T16:50:37Z

Thanks @NicoBrabers !

It's been a pleasure @BrennanConroy!

Fix for issue dotnet#54500: Added Microsoft.Extensions.DependencyInje…

33c9cf4

…ction to Microsoft.AspNetCore.Http.Abstractions and utilized the ActivatorUtilities it provides to obtain a middleware instance. Also changed the ReflectionMiddlewareBinder to be able to handle keyed injection.

NicoBrabers requested review from BrennanConroy, halter73 and captainsafia as code owners May 14, 2024 23:04

dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label May 14, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 14, 2024

gfoidl added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware and removed area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels May 15, 2024

gfoidl reviewed May 15, 2024

View reviewed changes

src/Http/Http.Abstractions/src/Extensions/UseMiddlewareExtensions.cs Outdated Show resolved Hide resolved

src/Http/Http.Abstractions/test/UsePathBaseExtensionsTests.cs Outdated Show resolved Hide resolved

NicoBrabers requested a review from gfoidl May 15, 2024 12:41

Fix for issue dotnet#54500: Added different method formatting (added …

c4a279f

…new lines) based on pull-request feedback.

NicoBrabers force-pushed the 54500-keyed-service-middleware-injection branch from f01cba9 to c4a279f Compare May 15, 2024 13:08

mkArtakMSFT assigned adityamandaleeka May 15, 2024

BrennanConroy reviewed May 15, 2024

View reviewed changes

Fix for issue dotnet#54500: Precompute and cache the results for each…

d28858e

… parameter within the ReflectionFallback.

NicoBrabers requested a review from BrennanConroy May 16, 2024 20:25

Trigger CI

420f3ed

BrennanConroy reviewed May 17, 2024

View reviewed changes

Fix for issue dotnet#54500: Precompute and cache the key result for e…

393802a

…ach parameter within the ReflectionFallback, removed the parameterType and declaringType from the precomputed cache.

captainsafia reviewed May 17, 2024

View reviewed changes

BrennanConroy reviewed May 17, 2024

View reviewed changes

NicoBrabers added 2 commits May 18, 2024 01:47

Fix for issue dotnet#54500: Refactored the TryGetServiceKey method an…

2bc5913

…d added the NotNullWhen(true) attribute. Also changed the Inherit param from true to false when obtaining the custom attribute FromKeyedServicesAttribute.

Fix for issue dotnet#54500: Removed unnecessary mock class from unit-…

e9b37eb

…test. Added Benchmark(s) for the ReflectionFallback method.

build-analysis bot mentioned this pull request May 22, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

Trigger CI

44a072c

NicoBrabers requested review from captainsafia and BrennanConroy May 22, 2024 12:52

Merge branch 'main' into 54500-keyed-service-middleware-injection

d938d4b

dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 11, 2024

Fix for issue dotnet#54500: Removed Microbenchmarks for the time being.

56d998c

Fix for issue dotnet#54500: Added additional constructor injection su…

9b5025f

…pport unit-tests.

BrennanConroy approved these changes Jul 24, 2024

View reviewed changes

NicoBrabers and others added 2 commits July 24, 2024 19:34

Update src/Http/Http.Abstractions/test/UseMiddlewareTest.cs

879b4fc

Co-authored-by: Brennan <brecon@microsoft.com>

Fix for issue dotnet#54500: Removed resource and added inline excepti…

24a913a

…on check in test method..

BrennanConroy reviewed Jul 24, 2024

View reviewed changes

Fix for issue dotnet#54500: Removed resource and added inline excepti…

e90de6e

…on check in test method.

BrennanConroy merged commit e29ea10 into dotnet:main Jul 25, 2024
22 of 25 checks passed

dotnet-policy-service bot added this to the 9.0-rc1 milestone Jul 25, 2024

BrennanConroy removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jul 25, 2024

amcasey mentioned this pull request Aug 23, 2024

[9.0.100-rc.1.24406.1] EasyCMS app launch failed with 'Multiple constructors accepting all given argument types have been found in type 'Microsoft.AspNetCore.CookiePolicy.CookiePolicyMiddleware' exception #57231

Closed

1 task

amcasey mentioned this pull request Oct 21, 2024

[Breaking change]: Handling of middleware types with multiple constructors aspnet/Announcements#514

Open

3 tasks

Fix for issue #54500: Middleware keyed dependency injection #55722

Fix for issue #54500: Middleware keyed dependency injection #55722

Conversation

NicoBrabers commented May 14, 2024 • edited Loading

Middleware keyed dependency injection (fix for issue #54500)

Description

NicoBrabers commented May 15, 2024

gfoidl left a comment

Choose a reason for hiding this comment

amcasey commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicoBrabers May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicoBrabers May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrennanConroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrennanConroy commented Jun 4, 2024

BrennanConroy commented Jul 22, 2024

NicoBrabers commented Jul 22, 2024

BrennanConroy commented Jul 22, 2024

NicoBrabers commented Jul 23, 2024

BrennanConroy commented Jul 23, 2024

NicoBrabers commented Jul 24, 2024 • edited Loading

NicoBrabers commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrennanConroy commented Jul 25, 2024

NicoBrabers commented Jul 25, 2024

NicoBrabers commented May 14, 2024 •

edited

Loading

NicoBrabers May 17, 2024 •

edited

Loading

NicoBrabers May 17, 2024 •

edited

Loading

NicoBrabers commented Jul 24, 2024 •

edited

Loading