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?

…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 · 2024-05-17T22:08:14Z

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

 }

 return (Task)methodInfo.Invoke(middleware, BindingFlags.DoNotWrapExceptions, binder: null, methodArguments, culture: null)!;
 };
 }

+ private static bool TryGetServiceKey(ParameterInfo parameterInfo, out object? key)


Suggested change

private static bool TryGetServiceKey(ParameterInfo parameterInfo, out object? key)

private static bool TryGetServiceKey(ParameterInfo parameterInfo, [MemberNotNullWhen(true)] out object? key)

@captainsafia, thanks for the feedback. I've implemented it together with some other changes. Would've used the "Commit suggestion" otherwise.

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

BrennanConroy · 2024-05-17T23:14:22Z

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

@@ -293,13 +379,27 @@ private class MiddlewareInjectInvokeNoService
 public Task Invoke(HttpContext context, object value) => Task.CompletedTask;
 }

+ private class MiddlewareKeyedInjectInvokeNoService


Doesn't seem like this class is needed? Just use the other class and don't add services to DI?

@BrennanConroy, hmm, appears to be the case. I'll double check it in the morning and adjust it accordingly.

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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NicoBrabers commented May 14, 2024 •

edited

NicoBrabers commented May 15, 2024

gfoidl left a comment

amcasey commented May 15, 2024

BrennanConroy May 15, 2024

NicoBrabers May 16, 2024

NicoBrabers May 17, 2024 •

edited

BrennanConroy May 17, 2024

NicoBrabers May 17, 2024 •

edited

BrennanConroy May 17, 2024

NicoBrabers May 17, 2024

NicoBrabers May 21, 2024

captainsafia May 17, 2024

NicoBrabers May 17, 2024

NicoBrabers May 22, 2024

captainsafia May 17, 2024

NicoBrabers May 17, 2024

BrennanConroy left a comment

BrennanConroy May 17, 2024

NicoBrabers May 17, 2024

BrennanConroy May 17, 2024

	private static bool TryGetServiceKey(ParameterInfo parameterInfo, out object? key)
	private static bool TryGetServiceKey(ParameterInfo parameterInfo, [MemberNotNullWhen(true)] out object? key)

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

Are you sure you want to change the base?

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

Conversation

NicoBrabers commented May 14, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicoBrabers May 17, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicoBrabers commented May 14, 2024 •

edited

NicoBrabers May 17, 2024 •

edited

NicoBrabers May 17, 2024 •

edited