aspnet / BasicMiddleware

[Archived] Basic middleware components for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
169 stars 84 forks source link

DisableResponseBuffering does not work as expected in ResponseCompression #137

Closed pakrym closed 8 years ago

pakrym commented 8 years ago

Writes would not flush automatically in the following code.

 app.Map("/slownb", a =>
            {
                a.UseResponseCompression();
                a.Run(async w =>
                {
                    w.Response.Headers["Content-Type"] = "text/html";

                    var nb = w.Features.Get<IHttpBufferingFeature>();
                    nb.DisableResponseBuffering();

                    while (!w.RequestAborted.IsCancellationRequested)
                    {
                        await w.Response.WriteAsync("1");
                        await Task.Delay(TimeSpan.FromSeconds(0.1));
                    }
                });
            });

@Tratcher

Tratcher commented 8 years ago

By design, call Flush/Async.

pakrym commented 8 years ago

Then why have it? Flush works even without DisableResponseBuffering

Tratcher commented 8 years ago

Only on Core, not on Net 4.5.1.

pakrym commented 8 years ago

I'm concerned that it's not consistent with ResponseBuffering in buffering if you call DisableResponseBuffering writes are immediately written to the wire.

Tratcher commented 8 years ago

@davidfowl

davidfowl commented 8 years ago

It's pretty strange that the same interface has different behavior depending on who implements it. Wouldn't it be better to make them all consistently disable all buffering all the time when this method is called?

Tratcher commented 8 years ago

Are you saying that because that's the name of the feature, or because that's the behavior end-users actually want? When I had this discussion with @DamianEdwards his primary concern was that we make Flush work.

The current implementation is a compromise that allows both compression and the ability to Flush, if supported.

davidfowl commented 8 years ago

Because the contract of the interface says get all buffering out of the way. An explicit flush shouldn't be required to make this work. Flushing without calling disable buffering can do this can't it?

Tratcher commented 8 years ago

Not on Net451

Tratcher commented 8 years ago

Offline discussion: DisableResponseBuffering should disable compression on .NET 4.5.1 and auto-flush writes on .NET Core.

muratg commented 8 years ago

https://github.com/aspnet/BasicMiddleware/pull/154/files