IHttpBufferingFeature.DisableRequestBuffering is named incorrectly

[DamianEdwards]

This method currently actually disables dynamic compression on the response when on IIS. It has nothing to do with buffering the request. In fact, if you're desire is to stop buffering of outgoing streaming responses, you need to call this API when on IIS.

It should probably be called DisableResponseCompression (because that's what it actually does and is intended to do) but that might look odd on the IHttpBufferingFeature interface (why are there compression methods on the buffering feature?).

Perhaps the better approach is simply remove this method altogether and reduce the interface to just have DisableResponseBuffering and in IIS have it disable both the IIS response buffers and the dynamic compression module. After all, that's what you need to do to truly turn of response buffering on IIS anyway.

/cc @davidfowl @lodejard @Tratcher @GrabYourPitchforks

[Tratcher]

The naming is by design [after much heated debate]. This is a generic feature interface, so it was named after the desired behavior. How the implementer achieves that behavior is an internal implementation detail we don't need to know about.

That said, I agree that something is odd about the current implementation (Helios).

        void IHttpBufferingFeature.DisableRequestBuffering()
        {
            _httpContext.Response.DisableDynamicCompression();
        }

        void IHttpBufferingFeature.DisableResponseBuffering()
        {
            _httpContext.Response.DisableBuffering();
        }

I would expect it to look more like this:

        void IHttpBufferingFeature.DisableRequestBuffering()
        {
        }

        void IHttpBufferingFeature.DisableResponseBuffering()
        {
            _httpContext.Response.DisableDynamicCompression();
            _httpContext.Response.DisableBuffering();
        }

[GrabYourPitchforks]

The Helios implementation is intentional. There are actually two distinct behaviors that applications might want. The first is that all calls to Write should be auto-flushed without requiring an explicit call to Flush. Most applications that want to disable response buffering do so because they're working with large in-memory buffers, and they're more concerned with resource utilization than latency. So this behavior is the default "disable buffering" behavior.

The second behavior is that calls to Flush should really, honestly, truly go out onto the wire with minimal additional processing or delay by the server. This is the desired behavior for specialty frameworks like SignalR, where low latency takes priority over low resource utilization.

Two distinct behaviors = two distinct APIs.

[davidfowl]

If we keep the IHttpBufferingFeature interface then we should delete the DisableRequestBuffering method because it makes no sense there. Helios should have a specific interface (assembly neutral of course) that has DisableDynamicCompression() on it.

[muratg]

What's the decision on this one?

/cc: @glennc

[davidfowl]

We need to make sure we revisit this entire area. We started and took tons of notes:

• https://github.com/aspnet/HttpAbstractions/wiki/Rolling-Notes
• https://github.com/aspnet/HttpAbstractions/wiki/Rolling-Notes---Response-Styles
• https://github.com/aspnet/HttpAbstractions/wiki/Rolling-Notes-Response-Stream-Contract

But we haven't finished it. We need to finish it and implement something.

/cc @lodejard @DamianEdwards @GrabYourPitchforks @Tratcher

[muratg]

@glennc Can we get this discussed in the next design meeting? @davidfowl @lodejard bring in your thoughts :)

[muratg]

@davidfowl Do we need another API review for HttpAbstractions?

[analogrelay]

This will be covered by aspnet/Hosting#299 (according to @davidfowl)