BodyWriter polyfill doesn't flush, causes truncation

[Tratcher]

#11268 identified a truncation issue with the HttpContext.Response.BodyWriter polyfill logic.

The BodyWriter polyfill automatically creates a pipe wrapped around the stream. If you use this pipe to call GetMemory and Advance but do not call FlushAsync, this data is not flushed to the underlying stream. When the request unwinds the pipe will eventually be completed by the OnCompleted event, but it's too late to flush the data as the response has already been disposed.

This is different behavior from any server that does expose pipes (Kestrel) where any data in the pipe is automatically flushed by the server when it completes the pipe.

Proposal: This polyfill should have been implemented as middleware that could auto-flush the pipe at the end of the request.

Note that polyfill middleware would also need to intercept CompleteAsync and flush in that case as well.

Counter proposal: The app is always required to call FlushAsync.

@davidfowl @JamesNK

[davidfowl]

Proposal: This polyfill should have been implemented as middleware that could auto-flush the pipe at the end of the request.

No way, that's extremely disruptive and shouldn't have to be a middleware, one thing we need to do to a work around problems like this (having a middleware) is to add a new event to the pipeline (IHttpResponse.OnCompleting) that fires before everything is torn down so that we can perform flushes like this without requiring a middleware as part of the pipeline. It's so much cleaner and is localized to the callsite and requires less moving parts.

This event would make it possible to implement the buffering middleware without middleware 😄

PS: Not calling FlushAsync on the Pipe is bad usage anyways so I don't think this is critical, not calling Complete is a much worse behavior. It's likely in the future that PipeWriter will implement IAsyncDisposable which will flush all pending buffers.

[Tratcher]

OnCompleting would be an ordering mess vs middleware already in the pipeline. It only works if everybody uses it, and in order.

E.g. You have the response compression middleware that replaces the response Body. The application writes to the BodyPipe via the polyfill. Then the response compression middleware completes and closes out the compression state. Then OnCompleting fires and the polfyfill tries to flush the BodyPipe. Disaster.

Actually, the above scenario isn't much better with the polyfill middleware, you would have to add the middleware after response compression in order for it to work.

[Tratcher]

Note the long term solution here is the merger of pipes and streams so the polyfill is built in at a lower level.

[davidfowl]

Actually, the above scenario isn't much better with the polyfill middleware, you would have to add the middleware after response compression in order for it to work.

I don't agree. Having to add a middleware to the pipeline to use a property on HttpContext is a mistake (it's why I hate having session on the HttpContext!). We would make everyone use the event, I don't see what the problem is with that, it's just like OnStarting, an established pattern in our stack (everyone has to use it an chain it to make "the right thing happen"). I don't see why we don't have an equivalent on the response side. It's a gap.

Note the long term solution here is the merger of pipes and streams so the polyfill is built in at a lower level.

How does that help?

[Tratcher]

We would make everyone use the event

OnStarting has been such a pain that we abandon most uses of it. It doesn't shim, chain, or fail well. Instead, we end up intercepting the response body in almost all cases.

Note the long term solution here is the merger of pipes and streams so the polyfill is built in at a lower level.

How does that help?

Would this adapter be implemented any differently if it were a polyfill built into the Stream class? Maybe it would auto-flush, maybe not. But it would lower the bar for implementing BodyWriter in each server, e.g. if all they had to do was call flush on their own stream at the end of a request, kind of like what James tried in his TestServer PR.

[davidfowl]

OnStarting has been such a pain that we abandon most uses of it. It doesn't shim, chain, or fail well. Instead, we end up intercepting the response body in almost all cases.

It's the pain we have today and because we don't buffering everything by default, we'll always need something like this. We need an event that fires at the end of the pipeline so things like this can be implemented without middleware.

Would this adapter be implemented any differently if it were a polyfill built into the Stream class? Maybe it would auto-flush, maybe not. But it would lower the bar for implementing BodyWriter in each server, e.g. if all they had to do was call flush on their own stream at the end of a request, kind of like what James tried in his TestServer PR.

It would not help, Streams have the exact same issue, anything that buffers has the exact same issue, it's the problem of the call site not calling DisposeAsync/Complete/FluhsAsync on anything that buffers, and expecting something else in the pipeline to do it. Merging pipes and streams solves nothing and we need to either put this behavior into every server or empower other pieces of the stack the way we do with OnStarting (i.e. OnCompleting) to achieve similar behavior. That's the solution I'd prefer for something like this. The middleware approach is a non-starter IMO.

[Tratcher]

• On hold until you get back *

[analogrelay]

Requring FlushAsync will break #11270

[JamesNK]

If you really want to require FlushAsync, perhaps CompleteAsync could be the way to send data and trailers together?