API http.sys: EnableKernelResponseBuffering

[mgravell]

Background and Motivation

Additional configuration option for http.sys : #47776

Proposed API

namespace Microsoft.AspNetCore.Server.HttpSys;

public class HttpSysOptions
{
+    public bool EnableKernelResponseBuffering { get; set; } 
}

Usage Examples

builder.UseHttpSys(options =>
{
    options.EnableKernelResponseBuffering = true;
});

Alternative Designs

for back-port and compat; app-context switch that also impacts initial value

Risks

None; no change if not used; no overload resolution issues.

[ghost]

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.

[davidfowl]

Is there any reason to want to do this per request rather than per server instance?

[mgravell]

@davidfowl I can see that being a requested option - we have discussed it a few times; I did consider some new optional feature interface, but my view was that ultimately: we don't make buffering per-request for Kestrel (or even an option at all), so it seems weird to say that it would be a hard demand here - so keep the API as simple as possible. But if you disagree, I know how we'd do it. I welcome your thoughts.

[halter73]

A per-request feature could be useful to libraries that do a bunch of small writes, care about HttpSys perf, and want to avoid mucking with global HttpSysOptions. It's also possible that app developers might only have a specific endpoint they want to enable buffering on.

Are there any scenarios where enabling this would be very bad? If so, we should definitely add a feature. If not, should we change the default behavior staring in .NET 8? We could wait for someone to ask for a per-request feature if it's not clear anyone will use it. But if it's easy, why not?

[mgravell]

@halter73 I'm happy to make it a per-request thing - I was mostly trying to minimize the API impact; the actual code for per-request would be minimal:

• change the ResponseBody.EnableKernelResponseBuffering from a pass-thru to {options} to instead be a per-instance that is initialized from {options}
• define a new feature (naming is hard... IHttpResponseBufferFeature with method or property to set value?)
• make RequestContext implement it (already implements tons) and register via StandardFeatureCollection
• in feature impl: if no change; no-op; if change, apply UNLESS Response.HasStarted - in which case ignore? throw?

That ^^^ is the per-request change-set; isn't huge - trickiest bit is naming the feature and API.

By contrast: if we keep it simple, the API change is already fairly obvious. Thoughts?

[Tratcher]

Don't overcomplicate it. The current proposal fulfils the requirements. The feature could be added later if needed.

[halter73]

API Review Notes:

1. The per-request feature is additive and could be added later. It sounds like it could be useful, but no customer has specifically asked for it.
2. As to the question about whether we should enable kernel response buffering by default, @davidni made the following comment on the PR pointing out that there are real downsides to enabling it, so it seems safest not to

   Side-note for posterity -- when I played with HTTP.sys perf on Asp .NET Core back in the day, one of our experiments was to NOT enable HTTP.sys buffering, and instead to do overlapped writes, ensuring there were always at least N outstanding writes. This helped improve perf in our scenarios (proxying large responses from a destination service -- before YARP), but (surprisingly at the time) we were never able to achieve the same throughput that http.sys kernel buffering afforded.

API Approved!

namespace Microsoft.AspNetCore.Server.HttpSys;

public class HttpSysOptions
{
+    public bool EnableKernelResponseBuffering { get; set; } 
}

[davidni]

davidni made the following comment on the PR pointing out that there are real downsides to enabling it, so it seems safest not to

@halter73 there might have been a misunderstanding, my comment was that the alternative to enabling kernel buffering was not as performant (i.e. attempting overlapping writes instead of simply enabling kernel buffering didn't work so well). I have no evidence of any downsides of enabling kernel buffering, much the contrary.

[halter73]

Thanks for the correction @davidni. I'd be tempted to change the default then if we don't think it's going to hurt latency in streaming scenarios or anything like that. I understand wanting to be conservative with this kind of change though.

If we did decide to change the default before shipping .NET 8, we'd probably want the feature to selectively disable it just in case. And I'd want to change the option name to DisableKernelResponseBuffering to keep the default value false. It's not a big deal if we change the default value to true in a later release though.

[mgravell]

@halter73 in some ways it is tempting to remove the "enable" vs "disable" nomenclature, preferring just "use", but: that isn't the existing convention :(

[BrennanConroy]

Why is the issue closed?

[mgravell]

@BrennanConroy if I'm doing it wrong: let me know; I thought it was "closed as complete" since approved

[BrennanConroy]

Usually issues are closed when the PR is merged.