Skip to content

Add new Use middleware extension method #31784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 21, 2021
Merged

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Apr 13, 2021

Description

Add a new app.Use overload that requires users to pass the HttpContext to the next function which will save 2 per-request allocations over the previous extension method.

Customer Impact

Less per-request allocations when using simple middleware.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

[Justify the selection above]
Terminal middleware that used app.Use will need to update to use app.Run otherwise they'll get an ambiguous method compile error. We'd like to get this in early to get feedback on how bad that is, or if it isn't a problem.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

Fixes #31463


Main change is located at https://github.com/dotnet/aspnetcore/compare/brecon/use?expand=1#diff-e80bc3e234b6da05aea986e2aaa5b92bf492f085e00e7497d268e107169c9c6c
and
https://github.com/dotnet/aspnetcore/compare/brecon/use?expand=1#diff-dcc7a146ff62319dc1f9ab1847a9697596a4d899e7f75b588ed3c74d594ea305

The rest is changing all our tests and samples to use the new overload, or switch to app.Run if they never called next.

Should we file a breaking change announcement that people calling app.Use without calling next will be broken?

Note: We should apply this same change to the ConnectionBuilder

TODO: File docs issue to update code to use new overload

@davidfowl
Copy link
Member

Is this a breaking change?

/// {
/// return next(context);
/// });
/// </code>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a quip about preferring Run if next is unused?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Tratcher
Copy link
Member

I'm curious to get this into a preview and see if we get any feedback about it. People will only notice it on re-compile, and I wonder how obvious it will be if they need to switch to Run.

@davidfowl
Copy link
Member

This should be in for preview4 if we're going to do it.

@BrennanConroy BrennanConroy changed the base branch from main to release/6.0-preview4 April 20, 2021 16:18
@BrennanConroy BrennanConroy added the Servicing-consider Shiproom approval is required for the issue label Apr 20, 2021
@ghost
Copy link

ghost commented Apr 20, 2021

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@BrennanConroy BrennanConroy added this to the 6.0-preview4 milestone Apr 20, 2021
@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Apr 20, 2021
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, and I think we should do this for preview4. I'm not sure the risk is "low" though. This breaking change broke a couple samples and a lot of tests that were using Use instead of Run for terminal middleware, right? I don't think it's that uncommon of a pattern.

This is at least "medium" risk, right?

@BrennanConroy
Copy link
Member Author

I see risk as more of "possibility of breaking your applications behavior unexpectedly". This wont break your apps behavior, you might just need to modify your code slightly to compile.

@halter73
Copy link
Member

So compile-time breaking changes that don't change runtime behavior of apps that still compile without changes are considered "low" risk? And the number existing apps that work in .NET 5 and won't compile after this change has absolutely no impact on the risk?

I guess I can get behind that for previews since we can always revert and there's a simple code fix. I'm just surprised that's the standard.

@BrennanConroy
Copy link
Member Author

I mean, that's my opinion, not a standard. And risk in this case doesn't mean the same thing as it does for patches and RC releases. If we were making a compile time breaking change in a patch, then this would be Extremely high risk.

But like you noted, since this is a preview and we can revert it later, low risk.

@halter73
Copy link
Member

Should we file a breaking change announcement that people calling app.Use without calling next will be broken?

Yes. What does the compile error look like exactly BTW?

@BrennanConroy
Copy link
Member Author

CS0121 The call is ambiguous between the following methods or properties: 'UseExtensions.Use(IApplicationBuilder, Func<HttpContext, Func, Task>)' and 'UseExtensions.Use(IApplicationBuilder, Func<HttpContext, RequestDelegate, Task>)'

@BrennanConroy
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).
@BrennanConroy
Copy link
Member Author

@dotnet/aspnet-build
Feel free to merge whenever

@Rick-Anderson
Copy link
Contributor

TODO: File docs issue to update code to use new overload

dotnet/AspNetCore.Docs#23688

@ghost
Copy link

ghost commented Oct 29, 2021

Hi @Rick-Anderson. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-http-abstractions Servicing-approved Shiproom has approved the issue
9 participants