Enabling buffering for all requests. Fixes #3765. #3851
Conversation
| var bufferingFeature = context.Features.Get<IHttpBufferingFeature>(); | ||
| bufferingFeature?.DisableRequestBuffering(); | ||
| bufferingFeature?.DisableResponseBuffering(); | ||
| } |
There was a problem hiding this comment.
As we discussed, we can't have these changes alone as they would potentially have a significant performance impact because of the buffering middleware introduced by proxies.
alrod
force-pushed
the
alrod-ancm-test
branch
from
e5e0be6
to
6c0f73d
Compare
| @@ -0,0 +1,19 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
|
|
|||
| <PropertyGroup> | |||
| } | ||
|
|
||
| var originalBufferingFeature = httpContext.Features.Get<IHttpBufferingFeature>(); | ||
| //var originalSendFileFeature = httpContext.Features.Get<IHttpSendFileFeature>(); |
| var bufferStream = new BufferingWriteStream(originalResponseBody); | ||
| httpContext.Response.Body = bufferStream; | ||
| httpContext.Features.Set<IHttpBufferingFeature>(new HttpBufferingFeature(bufferStream, originalBufferingFeature)); | ||
| //if (originalSendFileFeature != null) |
| @@ -49,6 +49,7 @@ | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="appinsights.testlogger" Version="1.0.0" /> | |||
| <PackageReference Include="Microsoft.AspNetCore.TestHost" Version="2.1.2-rtm-30908" /> | |||
alrod
force-pushed
the
alrod-ancm-test
branch
from
9ddbdf8
to
27cb71c
Compare
| { | ||
| public static class ResponseBufferingMiddlewareExtensions | ||
| { | ||
| public static IApplicationBuilder UseResponseBuffering(this IApplicationBuilder builder) |
There was a problem hiding this comment.
Are you missing some files? where is this being called?
There was a problem hiding this comment.
I don't think we ever called the extension method (always had an explicit middleware registration), so this file is not really needed.
There was a problem hiding this comment.
This extension is called in ResponseBufferingMiddlewareTests
There was a problem hiding this comment.
It would be nice to avoid increasing the product surface just to be able to use their tests. This is a convenience method and we can build the tests without it (or bring it into the tests instead, but it isn't required).
There was a problem hiding this comment.
Just wanted to keep tests the same as in original repo. ResponseBufferingMiddlewareExtensions was used 8 times here. Anyway removed the class.
| try | ||
| { | ||
| // Shim the response stream | ||
| var bufferStream = new BufferingWriteStream(originalResponseBody); |
There was a problem hiding this comment.
We should either do it with this PR or open another work item to track this, we should not allocate a BufferStream with the size of the respondeBody for non-proxy functions. this has a perf impact and I missed it when initially took a dependency on BufferingMiddleware code from aspnet team.
There was a problem hiding this comment.
At least an issue needs to be opened to track this. This would be pretty straight forward to address, but a little outside of the scope of this change.
alrod
force-pushed
the
alrod-ancm-test
branch
from
27cb71c
to
8cfb218
Compare
Fixes #3765
New ANCM disables gzip compression on "DisableRequestBuffering" call. With this change we will buffer all requests, not only proxy requests.