Add short circuit in routing #46713

Kahbazi · 2023-02-16T20:52:00Z

ghost · 2023-02-16T20:52:09Z

Thanks for your PR, @Kahbazi. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

davidfowl · 2023-02-16T21:00:02Z

src/Http/Routing/src/EndpointRoutingMiddleware.cs

+            var shortCircuitMetadata = endpoint.Metadata.GetMetadata<ShortCircuitMetadata>();
+            if (shortCircuitMetadata is not null)
+            {
+                if (!_routeOptions.SuppressCheckForUnhandledSecurityMetadata)
+                {
+                    if (endpoint.Metadata.GetMetadata<IAuthorizeData>() is not null)
+                    {
+                        ThrowMissingAuthMiddlewareException(endpoint);
+                    }
+
+                    if (endpoint.Metadata.GetMetadata<ICorsMetadata>() is not null)
+                    {
+                        ThrowMissingCorsMiddlewareException(endpoint);
+                    }
+                }
+
+                if (shortCircuitMetadata.StatusCode.HasValue)
+                {
+                    httpContext.Response.StatusCode = shortCircuitMetadata.StatusCode.Value;
+                }
+
+                if (endpoint.RequestDelegate is not null)
+                {
+                    return endpoint.RequestDelegate(httpContext);
+                }
+


There must be a more efficient way to do this since it's a first class feature now.

Which part concerns you, endpoint.Metadata.GetMetadata<ShortCircuitMetadata>();?

Thinking a bit further on this, I believe this will hurt performance in the common case (that your route matches) compared to the more exceptional case that you want to shortcircuit. I think that probably means that this should be opt-in, so that apps that don't have this concern don't have to pay the performance penalty.

An alternative design is to create and set a special type of endpoint ShortcutEndpoint with a fixed delegate and a Shortcut property, and cast to that instead of searching inside the metadata collection.

Our goal should be to keep logic that is not related to endpoint selection outside of routing as much as possible. Code that provides specific behaviors should in general go into their own middleware so that devs get a chance of choosing when to apply the behavior and so that routing doesn't have to be aware of such concerns.

What part is the perf overhead, endpoint.Metadata.GetMetadata<ShortCircuitMetadata>();? Rather than separating the components, we could mitigate that cost instead.

If GetMetadata is too slow then we should look into improving it, It gets called a lot per request.

Alternatives:

A new bool property on Endpoint?

A derived Endpoint?

The approach that is currently followed here is inheritance, see "Endpoint" and "RouteEndpoint".

Inheritance doesn't seem like a good fit in this case, we'd want this behavior to apply to existing kinds of endpoints, especially RouteEndpoint's.

The more this grows, the more it feels that the execution part should be in its own middleware, and we should keep the routing middleware as is.

It feels that this is a very specific behavior that is useful in a few cases but that everybody is now paying for.

Why does this deserve a special treatment within the endpoint routing middleware as compared to the authorization/authentication or other similar middleware. If the concern is that there needs to be an extra call in the pipeline that the user might forget, why can't it work similar to how the authentication middleware does and throw an exception if it detects the middleware is not present.

If the concern is that there needs to be an extra call in the pipeline that the user might forget, why can't it work similar to how the authentication middleware does and throw an exception if it detects the middleware is not present.

That ends up being the same check as this that gets called on every request.

The microbenchmarks do not show this to be significant overhead.

src/Http/Routing/src/EndpointRoutingMiddleware.cs

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs

src/Http/Routing/src/EndpointRoutingMiddleware.cs

Kahbazi · 2023-02-22T17:33:11Z

src/Http/Routing/perf/Microbenchmarks/EndpointRoutingShortCircuitBenchmark.cs

+
+namespace Microsoft.AspNetCore.Routing;
+
+public class EndpointRoutingShortCircuitBenchmark


Here's the result

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated

NormalEndpoint 665.7 ns 5.81 ns 4.53 ns 1,502,118.8 0.0343 - - 1 KB

ShortCircuitEndpoint 772.2 ns 9.12 ns 8.08 ns 1,295,049.2 0.0343 - - 1 KB

What's NormalEndpoints benchmark without your changes?

No it's with my changes. I will add another benchmark with previous implementation too. I assume we don't need to have two different implementation in the code base, right? I just run it in my local?

Correct, we just want to confirm the before-and-after to make sure this doesn't regress anything.

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated

New Implementation without short circuit 658.7 ns 4.03 ns 3.37 ns 1,518,082.2 0.0343 - - 1 KB

Old Implementation 639.7 ns 10.42 ns 12.41 ns 1,563,187.5 0.0343 - - 1 KB

New Implementation with short circuit 791.3 ns 3.24 ns 2.87 ns 1,263,689.7 0.0343 - - 1 KB

1,518,082.2 vs 1,563,187.5? That's less than 3% on a microbenchmark. I doubt it would even show up on the end-to-end. Adding the endpoint execution middleware to this benchmark would make it more representative in all cases.

Ah I looked at the wrong results.

I doubt it would even show up on the end-to-end

We don't have to guess, we can just run it.

New Implementation with short circuit

Why is this so much worse?

It's hitting the new code path that would normally not be hit until the EndpointMiddleware. If we include the EndpointMiddleware in the benchmark you shouldn't notice a difference.
https://github.com/dotnet/aspnetcore/pull/46713/files#diff-5019372fafa72e521b12fab1c28fd96919cb21c5294003b7777f5a46322cca54R113-R175
https://github.com/dotnet/aspnetcore/blob/main/src/Http/Routing/src/EndpointMiddleware.cs#L34-L78

Here's the result with both endpoint routing middleware and endpoint middleware together.

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated

New Implementation without short circuit 766.3 ns 2.28 ns 1.90 ns 1,304,930.5 0.0343 - - 1 KB

New Implementation with short circuit 775.2 ns 3.44 ns 2.87 ns 1,290,051.6 0.0343 - - 1 KB

Old Implementation 728.6 ns 6.77 ns 5.28 ns 1,372,541.8 0.0343 - - 1 KB

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointConventionBuilderExtensions.cs

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointConventionBuilderExtensions.cs

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs

src/Http/Routing/src/EndpointRoutingMiddleware.cs

davidfowl · 2023-03-02T00:39:26Z

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs

+            group.Map(routePrefix, _shortCircuitDelegate).ShortCircuit(statusCode);
+        }
+
+        return new EndpointConventionBuilder(group);


Why not return the group?

I agree groups are convenient for this. The one problem I see is that even though the declared type is IEndpointConventionBuilder, someone could call (RouteGroupBuilder)app.MapShortCircuit(404, "/prefix1", "/prfix2") and then add additional routes.

While not exactly a problem, it's a little weird, and would prevent us from returning another IEndpointConventionBuilder that's not derived from the currently-sealed RouteGroupBuilder without going through the breaking change process since it'd be considered a behavioral breaking change to the return value. So probably better to be safe and copy the private CompositeEndpointConventionBuilder. Maybe we could make it shared source.

From #46713 (comment)

@halter73 is this really a concern?

Sorry I missed this, but yes. If we think it's valuable to return a RouteGroupBuilder, we should make that the declared return type. You know people will end up casting it anyway if we let them.

The current implementation is fine, if cautious. We can decide to remove the wrapper later if we think that caution is not needed. I don't think we want to change the API return type though. Publicly returning the group exposes an implementation detail and sets the wrong expectation about how that return type can be used. The group APIs don't make sense for this scenario. We only return the builder so that other metadata can be added like http method constraints.

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs

Tratcher · 2023-03-03T00:11:50Z

@davidfowl, we addressed the feedback here and in API review. We'll keep this in the routing middleware for now.

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs

ghost · 2023-03-15T15:00:51Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs

Tratcher · 2023-03-17T22:05:20Z

/azp run

azure-pipelines · 2023-03-17T22:05:38Z

Azure Pipelines successfully started running 3 pipeline(s).

Tratcher · 2023-03-18T04:06:31Z

Thanks @Kahbazi !

ghost · 2023-03-24T01:29:43Z

@Kahbazi, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

WeihanLi · 2023-11-08T14:55:27Z

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs

+        foreach (var routePrefix in routePrefixes)
+        {
+            string route;
+            if (routePrefix.EndsWith("/", StringComparison.OrdinalIgnoreCase))


Would it better if using if (routePrefix[^1] is '/') instead

Kahbazi requested a review from javiercn as a code owner February 16, 2023 20:52

ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Feb 16, 2023

davidfowl reviewed Feb 16, 2023

View reviewed changes

Kahbazi force-pushed the kahbazi/shortCircuit branch from d530ba5 to 5443182 Compare February 16, 2023 21:09

mkArtakMSFT assigned adityamandaleeka Feb 16, 2023

Tratcher self-assigned this Feb 16, 2023

Tratcher reviewed Feb 16, 2023

View reviewed changes

src/Http/Routing/src/EndpointRoutingMiddleware.cs Outdated Show resolved Hide resolved

Tratcher requested a review from halter73 February 16, 2023 23:02

javiercn reviewed Feb 17, 2023

View reviewed changes

src/Http/Routing/src/EndpointRoutingMiddleware.cs Outdated Show resolved Hide resolved

javiercn reviewed Feb 17, 2023

View reviewed changes

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs Outdated Show resolved Hide resolved

Tratcher reviewed Feb 17, 2023

View reviewed changes

Kahbazi commented Feb 22, 2023

View reviewed changes

Tratcher reviewed Feb 22, 2023

View reviewed changes

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointConventionBuilderExtensions.cs Outdated Show resolved Hide resolved

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs Outdated Show resolved Hide resolved

Kahbazi commented Feb 23, 2023

View reviewed changes

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointConventionBuilderExtensions.cs Show resolved Hide resolved

halter73 reviewed Feb 23, 2023

View reviewed changes

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs Outdated Show resolved Hide resolved

Tratcher reviewed Feb 23, 2023

View reviewed changes

src/Http/Routing/src/EndpointRoutingMiddleware.cs Outdated Show resolved Hide resolved

Tratcher reviewed Feb 23, 2023

View reviewed changes

src/Http/Routing/src/EndpointRoutingMiddleware.cs Show resolved Hide resolved

Kahbazi added 6 commits March 1, 2023 23:02

Add route short circuit

859b841

Add benchmark

67939ca

Review feedback

ba36f70

remove log

f420070

Reuse Short Circuit metadata

a412348

Review Feedback

8abecae

Kahbazi force-pushed the kahbazi/shortCircuit branch from a0fb6a7 to 8abecae Compare March 1, 2023 19:33

build-analysis bot mentioned this pull request Mar 1, 2023

Request failed "500 Internal Server Error" dotnet/arcade#11737

Open

5 tasks

davidfowl reviewed Mar 2, 2023

View reviewed changes

Tratcher approved these changes Mar 2, 2023

View reviewed changes

Tratcher reviewed Mar 2, 2023

View reviewed changes

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs Outdated Show resolved Hide resolved

Tratcher assigned davidfowl Mar 3, 2023

Set the order

a3823d6

javiercn reviewed Mar 3, 2023

View reviewed changes

src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointRouteBuilderExtensions.cs Outdated Show resolved Hide resolved

Set display name, add tests

baaabf5

ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 15, 2023

adityamandaleeka reviewed Mar 16, 2023

View reviewed changes

src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs Outdated Show resolved Hide resolved

adityamandaleeka reviewed Mar 16, 2023

View reviewed changes

src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs Outdated Show resolved Hide resolved

adityamandaleeka reviewed Mar 16, 2023

View reviewed changes

src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs Outdated Show resolved Hide resolved

Minor edits

4c5c1d4

adityamandaleeka reviewed Mar 16, 2023

View reviewed changes

src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs Outdated Show resolved Hide resolved

adityamandaleeka reviewed Mar 16, 2023

View reviewed changes

src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs Outdated Show resolved Hide resolved

Spelling

f40f14b

adityamandaleeka removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 16, 2023

adityamandaleeka reviewed Mar 17, 2023

View reviewed changes

src/Http/Routing/test/UnitTests/EndpointRoutingMiddlewareTest.cs Outdated Show resolved Hide resolved

Update security metadata throw test to include cases without a status.

91ef740

Tratcher enabled auto-merge (squash) March 17, 2023 22:05

Tratcher merged commit c52fd93 into dotnet:main Mar 17, 2023

ghost added this to the 8.0-preview3 milestone Mar 17, 2023

Kahbazi deleted the kahbazi/shortCircuit branch March 18, 2023 08:30

adityamandaleeka added the blog-candidate Consider mentioning this in the release blog post label Mar 24, 2023

adityamandaleeka mentioned this pull request Apr 10, 2023

Add docs for short circuiting dotnet/AspNetCore.Docs#28935

Closed

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023

WeihanLi reviewed Nov 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add short circuit in routing #46713

Add short circuit in routing #46713

Kahbazi commented Feb 16, 2023

ghost commented Feb 16, 2023

davidfowl Feb 16, 2023

Tratcher Feb 16, 2023

javiercn Feb 17, 2023

javiercn Feb 17, 2023

Tratcher Feb 17, 2023

Tratcher Feb 17, 2023

javiercn Feb 17, 2023

Tratcher Feb 17, 2023

javiercn Mar 1, 2023

Tratcher Mar 1, 2023

Kahbazi Feb 22, 2023

Tratcher Feb 22, 2023

Kahbazi Feb 22, 2023

Tratcher Feb 22, 2023

Kahbazi Feb 23, 2023

Tratcher Feb 23, 2023

davidfowl Feb 23, 2023

davidfowl Feb 23, 2023

Tratcher Feb 23, 2023

Kahbazi Mar 1, 2023

davidfowl Mar 2, 2023

Kahbazi Mar 2, 2023

davidfowl Mar 2, 2023

halter73 Mar 16, 2023

Tratcher Mar 17, 2023

Tratcher commented Mar 3, 2023

ghost commented Mar 15, 2023

Tratcher commented Mar 17, 2023

azure-pipelines bot commented Mar 17, 2023

Tratcher commented Mar 18, 2023

ghost commented Mar 24, 2023

WeihanLi Nov 8, 2023


		namespace Microsoft.AspNetCore.Routing;

		public class EndpointRoutingShortCircuitBenchmark

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
NormalEndpoint	665.7 ns	5.81 ns	4.53 ns	1,502,118.8	0.0343	-	-	1 KB
ShortCircuitEndpoint	772.2 ns	9.12 ns	8.08 ns	1,295,049.2	0.0343	-	-	1 KB

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
New Implementation without short circuit	658.7 ns	4.03 ns	3.37 ns	1,518,082.2	0.0343	-	-	1 KB
Old Implementation	639.7 ns	10.42 ns	12.41 ns	1,563,187.5	0.0343	-	-	1 KB
New Implementation with short circuit	791.3 ns	3.24 ns	2.87 ns	1,263,689.7	0.0343	-	-	1 KB

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
New Implementation without short circuit	766.3 ns	2.28 ns	1.90 ns	1,304,930.5	0.0343	-	-	1 KB
New Implementation with short circuit	775.2 ns	3.44 ns	2.87 ns	1,290,051.6	0.0343	-	-	1 KB
Old Implementation	728.6 ns	6.77 ns	5.28 ns	1,372,541.8	0.0343	-	-	1 KB

Add short circuit in routing #46713

Add short circuit in routing #46713

Conversation

Kahbazi commented Feb 16, 2023

ghost commented Feb 16, 2023

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

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

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

Tratcher commented Mar 3, 2023

ghost commented Mar 15, 2023

Tratcher commented Mar 17, 2023

azure-pipelines bot commented Mar 17, 2023

Tratcher commented Mar 18, 2023

ghost commented Mar 24, 2023

Choose a reason for hiding this comment