Skip to content

Conversation

@shirhatti
Copy link
Contributor

Addresses #21163

@shirhatti
Copy link
Contributor Author

shirhatti commented Aug 13, 2020

Sample usage:

namespace Sample
{
    public class Startup
    {
        private DelegationRule _delegationRule;

        // This method gets called by the runtime. Use this method to add services to the container.
        // For more information on how to configure your application, visit https://go.microsoft.com/fwlink/?LinkID=398940
        public void ConfigureServices(IServiceCollection services)
        {
        }

        // This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
        public void Configure(IApplicationBuilder app, IHostApplicationLifetime lifetime, IWebHostEnvironment env)
        {
            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }
            
            var delegator = app.ServerFeatures.Get<IServerDelegationFeature>();
            _delegationRule = delegator.CreateDelegationRule("DefaultAppPool", "http://*:80/");

            app.Run(context =>
            {
                var transferFeature = context.Features.Get<IHttpSysRequestTransferFeature>();
                transferFeature.TransferRequest(_delegationRule);
                return Task.CompletedTask;
            });

            lifetime.ApplicationStopped.Register(() => _delegationRule.Dispose());
        }
    }
}

@shirhatti shirhatti added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 13, 2020
@ghost
Copy link

ghost commented Aug 13, 2020

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@shirhatti
Copy link
Contributor Author

shirhatti commented Aug 14, 2020

  • Move light-up into HttpApi.cs. Don't have handle EntryPointNotFound in multiple places
  • Don't always add the server feature, do light-up
  • Add test to verify feature isn't always added
  • Swap the order of MarkAsTransfered and Transfer. This ensures we can still write to the response body if transfer fails.
  • Throw InvalidOperationException if we attempt to transfer after reading from RequestBody
  • Add extension method to HttpContext to transfer request

@davidfowl
Copy link
Member

Add extension method to HttpContext to transfer request

We shouldn't do this, it isn't common enough to move top level, even as an extension method.

@shirhatti shirhatti requested a review from Tratcher August 15, 2020 00:09
@shirhatti shirhatti requested review from a team and SteveSandersonMS as code owners August 17, 2020 06:38
@shirhatti shirhatti force-pushed the shirhatti/delegaterequest branch from 33696b1 to 8726db0 Compare August 17, 2020 06:39
@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 17, 2020
@shirhatti shirhatti changed the base branch from master to release/5.0 August 18, 2020 00:05
@shirhatti
Copy link
Contributor Author

I ran a perf test with this program

            app.Run(async context =>
            {
                await context.Request.Body.DrainAsync(CancellationToken.None);
                context.Response.StatusCode = StatusCodes.Status200OK;
            });

Baseline from master:

application
CPU Usage (%) 74
Raw CPU Usage (%) 2,079.50
Working Set (MB) 373
Build Time (ms) 3,528
Start Time (ms) 286
Published Size (KB) 86,882
load
CPU Usage (%) 36
Raw CPU Usage (%) 1,007.63
Working Set (MB) 7
Build Time (ms) 3,613
Start Time (ms) 0
Published Size (KB) 76,390
First Request (ms) 67
Requests/sec 565,469
Requests 8,538,203
Mean latency (ms) 0.62
Max latency (ms) 32.83
Bad responses 0
Socket errors 0
Latency 50th (ms) 0.30
Latency 75th (ms) 0.71
Latency 90th (ms) 1.21
Latency 99th (ms) 4.04

My changes with HTTP_FLAGS.NONE (desired state)

application
CPU Usage (%) 74
Raw CPU Usage (%) 2,074.70
Working Set (MB) 375
Build Time (ms) 3,515
Start Time (ms) 298
Published Size (KB) 86,882
load
CPU Usage (%) 35
Raw CPU Usage (%) 988.69
Working Set (MB) 8
Build Time (ms) 3,436
Start Time (ms) 0
Published Size (KB) 76,390
First Request (ms) 90
Requests/sec 557,101
Requests 8,411,906
Mean latency (ms) 0.85
Max latency (ms) 114.54
Bad responses 0
Socket errors 0
Latency 50th (ms) 0.30
Latency 75th (ms) 0.73
Latency 90th (ms) 1.40
Latency 99th (ms) 9.64

My changes with HttpApiTypes.HTTP_FLAGS.HTTP_RECEIVE_REQUEST_FLAG_COPY_BODY (control)

application
CPU Usage (%) 74
Raw CPU Usage (%) 2,064.73
Working Set (MB) 378
Build Time (ms) 3,510
Start Time (ms) 319
Published Size (KB) 86,882
load
CPU Usage (%) 36
Raw CPU Usage (%) 1,000.74
Working Set (MB) 7
Build Time (ms) 3,538
Start Time (ms) 0
Published Size (KB) 76,390
First Request (ms) 90
Requests/sec 565,653
Requests 8,540,897
Mean latency (ms) 0.68
Max latency (ms) 53.49
Bad responses 0
Socket errors 0
Latency 50th (ms) 0.30
Latency 75th (ms) 0.72
Latency 90th (ms) 1.34
Latency 99th (ms) 5.53

@shirhatti
Copy link
Contributor Author

And good measure I ran my changes with HTTP_FLAGS.NONE again:

application
CPU Usage (%) 74
Raw CPU Usage (%) 2,080.62
Working Set (MB) 373
Build Time (ms) 3,518
Start Time (ms) 342
Published Size (KB) 86,882
load
CPU Usage (%) 36
Raw CPU Usage (%) 1,003.65
Working Set (MB) 7
Build Time (ms) 3,476
Start Time (ms) 0
Published Size (KB) 76,390
First Request (ms) 90
Requests/sec 568,510
Requests 8,584,097
Mean latency (ms) 0.60
Max latency (ms) 39.11
Bad responses 0
Socket errors 0
Latency 50th (ms) 0.30
Latency 75th (ms) 0.70
Latency 90th (ms) 1.24
Latency 99th (ms) 3.39

@shirhatti
Copy link
Contributor Author

TL;DR: No observable change in perf for the identified worst case with this change.

@shirhatti
Copy link
Contributor Author

@Pilchie Can I get permission to merge this change?

@Tratcher
Copy link
Member

@Pilchie Can I get permission to merge this change?

This is still tell mode, release/5.0 does not require additional merge permissions.

@shirhatti shirhatti merged commit b140c09 into release/5.0 Aug 19, 2020
@shirhatti shirhatti deleted the shirhatti/delegaterequest branch August 19, 2020 17:27
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants