ResponseBuffering middleware #6
Conversation
|
Hi @Tratcher, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
| @@ -0,0 +1,16 @@ | |||
| { | |||
| "version": "1.0.0-*", | |||
| "description": "Microsoft.AspNet.Buffering Class Library", | |||
There was a problem hiding this comment.
Description could use some improvement 😄
|
Just some fairly small comments, the code overall looks good. I like the new unit tests. |
|
Open question: Should there be a (configurable) limit on how much data to buffer before disabling buffering and flushing? |
|
Updated |
| } | ||
| else | ||
| { | ||
| base.EndWrite(asyncResult); |
Tratcher
force-pushed
the
tratcher/responseBuffer
branch
from
372df7e to
32e1ecd
Compare
| public static class ResponseBufferingMiddlewareExtensions | ||
| { | ||
| /// <summary> | ||
| /// Enabled full buffering of response bodies. This can be disabled on a per request basis using IHttpBufferingFeature. |
There was a problem hiding this comment.
Not sure where my comment went... Enables on the first word here.
|
Looks |
to me.
|
|
||
| namespace Microsoft.AspNet.Buffering | ||
| { | ||
| internal class HttpBufferingFeature : IHttpBufferingFeature |
There was a problem hiding this comment.
Q : Curious what is the idea for exceptions thrown when serializers throw exceptions(example in MVC)? Like if we know the response is being buffered, then we can send back a friendly error message. But with this interface it doesn't seem to be possible to check 'if' the response is buffered...
There was a problem hiding this comment.
You can check HttpResponse.HasStarted or HttpResponse.Body.CanSeek to know if the response body is buffered and/or if the headers can still be modified. Then Set Position and SetLength 0 zero to clear the body.
That said, MVC probably shouldn't render serialization exceptions inline. Throw it and let the exception handling logic decide.
There was a problem hiding this comment.
Yeah I'd be surprised if MVC does anything.
There was a problem hiding this comment.
We do something here for input, but it's easily circumvented. What @Tratcher is suggesting is a better choice than what we have.
There was a problem hiding this comment.
I believe there was also talk of HttpResponse convenience API for that case:
bool HttpResponse.CanClear { get{
true if Response.Body.CanSeek && !ResponseHeadersSent
}}
void HttpResponse.Clear() {
reset body and headers.
}
There was a problem hiding this comment.
Since they're very straightforward we should add these methods also, can you do that as this PR?
There was a problem hiding this comment.
This is the wrong repo/project to add them to. I think they'd go into Microsoft.AspNet.Http.Extensions.
|
|
|
@davidfowl @lodejard waiting on an explicit sign-off to confirm the design concepts. |
|
|
||
| public async Task Invoke(HttpContext httpContext) | ||
| { | ||
| var originalBufferingFeature = httpContext.GetFeature<IHttpBufferingFeature>(); |
There was a problem hiding this comment.
if httpContext.Response.HasStarted== true, or originalResponseBody.CanSeek == true, then this middleware should probably no-op instead of wrap.
There was a problem hiding this comment.
If HasStarted, Invoke shouldn't even be called. Do we have any scenarios where middleware writes to the response and then calls next?
|
Updated with Louis's feedback about preventing random access to the buffer. Added a sample. |
Restrict buffer to reset. Add sample. Cleanup.
Tratcher
force-pushed
the
tratcher/responseBuffer
branch
from
8c70662 to
c91cc89
Compare
aspnet/Hosting#299
This PR seeks to formalize the discussion from https://github.com/aspnet/HttpAbstractions/wiki/Rolling-Notes about how response buffering components should behave. In this case we've implemented a fully buffered response. Buffering is disabled via a feature interface or by calling Flush.
The one notable departure from the notes is that we use Stream.SetLength to truncate/reset the buffer, where Position is just used to move around your write marker.
@davidfowl @lodejard @Eilon