Response Compression middleware #52

Yves57 · 2016-08-04T00:27:47Z

A first PR proposition about #34

Included:

GZIP compression.
Options to set compression level, content minimum size threshold, accepted MIME types, additional compression providers.

Pending:

Probably optimizations in the temporary MemoryStream management.
A default accepted MIME types list?
A default value for MinimumSize option?
Other compression algorithms?

dnfclas · 2016-08-04T00:27:51Z

Hi @Yves57, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

Tratcher · 2016-08-05T17:00:02Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionMiddleware.cs

+
+            using (var uncompressedStream = new MemoryStream())
+            {
+                context.Response.Body = uncompressedStream;


Fully buffering all responses is not viable. We'll need to wrap the Response.Body stream and do the compression check on the first write.

Is there any special test to add, or I can assume that in any cases the MIME type is available when the first byte is written in the body Stream? In understand that the header is written before the body, but I want to be sure that there are not special cases.

- Do not buffer always the response body. - Remove optional parameters before analyzing the MIME type

Tratcher · 2016-08-05T22:42:51Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionMiddleware.cs

+                                else
+                                {
+                                    context.Response.Headers[HeaderNames.ContentEncoding] = compressionProvider.EncodingName;
+                                    context.Response.Headers[HeaderNames.ContentMD5] = StringValues.Empty;      // Reset the MD5 because the content changed.


Headers.Remove...

troydai · 2016-08-18T21:42:20Z

src/Microsoft.AspNetCore.ResponseCompression/BodyWrapperStream.cs

+using Microsoft.AspNetCore.Http;
+using Microsoft.Extensions.Primitives;
+using Microsoft.Net.Http.Headers;
+using System;


Please put System.* ahead of other namespaces.

troydai · 2016-08-19T15:48:56Z

samples/ResponseCompressionSample/ResponseCompressionSample.xproj

+    <OutputPath Condition="'$(OutputPath)'=='' ">.\bin\</OutputPath>
+  </PropertyGroup>
+  <PropertyGroup Label="Configuration">
+    <RootNamespace>C:\ws\github\BasicMiddleware\samples\RewriteSample\Properties\AssemblyInfo.csResponseCompressionSample</RootNamespace>


davidfowl · 2016-08-19T15:55:18Z

@Yves57 Can you get some basic performance data when this middleware is active and doing its job and when it's not since it always wraps the stream.

troydai · 2016-08-19T16:00:04Z

src/Microsoft.AspNetCore.ResponseCompression/GzipResponseCompressionProvider.cs

+            _level = level;
+        }
+
+        public string EncodingName


nit: follow form of property can make the code more condense and readable

public string EncodingName { get; } = "gzip";

blowdart · 2016-08-19T16:08:40Z

I'd like to see a flag, which is set by default, so compression doesn't happen if the request protocol is HTTPS to combat variations on Beast, Crime et al.

Tratcher · 2016-08-19T16:35:01Z

@DamianEdwards @muratg Are we actually planning to accept/ship this as an official package or should this be moved to contrib?

blowdart · 2016-08-19T16:50:49Z

If you're shipping it then I definitely want my flag :D

DamianEdwards · 2016-08-19T17:26:07Z

I'm more than happy to accept this from a function/product point of view (modulo usual API, security, perf, engineering reviews, etc.)

Yves57 · 2016-08-19T22:03:53Z

For the HTTPS flag, is EnableHttps a correct name for you?

Yves57 · 2016-08-19T23:23:46Z

@davidfowl I have used the TestServer to do very basic performance tests:

Body length: 100 bytes (so compression time itself is very small, but of course there is still the compression initialization time)
10000 requests
Test written in a unit test run in Release

The results on my computer:

No compression middleware: 965 ms
Compression active but not used: 1025 ms
Compression active and used : 1500 ms

I will push the perf test source code in my next PR (I can remove it later if you want).

Tratcher · 2016-08-20T15:58:17Z

How does it perform with Kestrel?

Yves57 · 2016-08-22T15:12:32Z

@Tratcher @davidfowl

Other test:

Using "go-wrk" the client side (with 1 or 4 connections during 10 seconds).
Using the PR sample application, except that the body length is a 100 byte constant string
first run result ignored (warmup)

On my computer (so all in local) with 1 connection:

No compression middleware: 379 requests/s
Compression active but not used: 379 requests/s
Compression active and used: 353 requests/s

On my computer (so all in local) with 4 connections:

No compression middleware: 499 requests/s
Compression active but not used: 498 requests/s
Compression active and used: 494 requests/s

So the stream wrapper seems to be more or less "free" in term of performance, even if I'm a little bit surprised by the small difference between compression/none with 4 connections.

- New 'EnableHttps' flag - Small source code feedbacks

RehanSaeed · 2016-08-25T15:44:31Z

I'm going to be building a Request Decompression middleware (I work for a company that sends large JSON logs from Android apps in rural Africa where GPRS internet connectivity is low and compression would make a massive difference). Request Decompression is essentially going to be a mirror image of Response Compression (Decompress, instead of Compress).

Is there any appetite for extending this PR or starting a new PR based on this one?

IMHO, I'm really surprised that Request Decompression is not a thing in .NET or IIS and even more surprised that I've never wondered why it was missing. No easy built in option for HttpClient either.

RehanSaeed · 2016-08-25T16:41:58Z

MIME Types

The MIME types have no default value, I propose the following list which only includes standard MIME types where possible and no common MIME type synonyms like text/javascript.

var mimeTypes = new string[]
{
    // Plain Text
    "text/html",
    "text/plain",
    "text/css",
    "text/mathml",
    "application/rtf",
    // JSON
    "application/javascript",
    "application/json",
    "application/manifest+json",
    "application/x-web-app-manifest+json",
    "text/cache-manifest",
    // XML
    "application/atom+xml",
    "application/rss+xml",
    "application/xslt+xml",
    "application/xml",
    // Fonts
    "font/opentype",
    "font/otf",
    "font/truetype",
    "application/font-woff",
    "application/vnd.ms-fontobject",
    "application/x-font-ttf",
    // Images
    "image/svg+xml",
    "image/x-icon"
};

You'd probably also want to use the above list for Request Compression, although the option to be different should still be there.

Minimum Size Option

IIS and NGINX provide an options which lets you specify a minimum size before compression is applied. This option should be added to the ResponseCompressionOptions.

NGINX also provides other options which I don't think are as important but should be considered:

The HTTP version to enable compression for. NGINX sets the default to 1.1 and not 1.0.
An option to use Regex to disable GZIP based on the User-Agent. This is commonly set to "MSIE [1-6]\." which disables GZIP for IE 1 to 6.

Yves57 · 2016-08-25T20:58:58Z

@davidfowl @Tratcher
I just made a memory profiling. Here are the results for 3368 requests.

I think the most important information in that DeflateStream allocates a significant amount of memory (GZipStream is in fact a wrapper around DeflateStream).

A managed buffer of 8192 bytes that is allocated each time.
Unmanaged memory buffers are allocated and destroyed each time in ZLib, while a native function deflateReset allows to reset ZLib without destroying buffers.

I think a good optimization would be to add a method GZipStream.Set(Stream newOutput) to reset an existing GZipStream without destroying buffers and assign a new output. With this method, we could have GZipStream instances in a pool, and use them on demand.

…ication

dodyg · 2016-09-11T12:54:19Z

I had to resort using the following MIME type statement instead of just application/json because that's what context.Response.ContentType returns.

app.UseResponseCompression(new string[]{ "application/json; charset=utf-8" });

Yves57 · 2016-09-11T16:07:40Z

@dodyg Are you sure that you have the latest version of the PR? The extension method with only MIME types does not exist anymore. And I just tried your sample without any problem.

davidfowl · 2016-09-11T19:07:07Z

src/Microsoft.AspNetCore.ResponseCompression/project.json

+        ]
+    },
+    "dependencies": {
+        "Microsoft.AspNetCore.Http": "1.1.0-*",


Middleware shouldn't need to reference Http directly. What are you using from here that isn't in abstractions?

You're right, "Http.Abstractions" is enough. I fix it.

dodyg · 2016-09-12T09:08:09Z

@Yves57 ah my fault. It works now.

            var compressionOptions = new ResponseCompressionOptions
            {
                MimeTypes = new string[]{ "application/json" }
            };
            app.UseResponseCompression(compressionOptions);

andrewlock · 2016-09-19T08:11:57Z

Just for reference, compression via middleware is available currently in WebMarkupMin.AspNetCore1 - I haven't compared the implementations, but there may be something of interest pertinent to this PR?

Tratcher · 2016-09-23T22:09:45Z

samples/ResponseCompressionSample/Properties/launchSettings.json

+      "commandName": "IISExpress",
+      "launchBrowser": true,
+      "environmentVariables": {
+        "ASPNET_ENV": "Development"


ASPNETCORE_ENVIRONMENT

I will fix, but just for information "ResponseBufferingSample" has the same error 😄 .

Feel free to fix the ResponseBufferingSample too :-)

Tratcher · 2016-09-23T22:09:50Z

samples/ResponseCompressionSample/Properties/launchSettings.json

+    "web": {
+      "commandName": "web",
+      "environmentVariables": {
+        "Hosting:Environment": "Development"


ASPNETCORE_ENVIRONMENT

Tratcher · 2016-09-23T22:16:04Z

src/Microsoft.AspNetCore.ResponseCompression/BodyWrapperStream.cs

+                IsMimeTypeCompressable(_response.ContentType);      // MIME type in the authorized list
+        }
+
+        private bool IsMimeTypeCompressable(string mimeType)


This should be extracted to a generic ShouldCompressResponse callback on Options for more customizable control. Then we'd have an implementation based on mime types.

Tratcher · 2016-09-23T22:17:53Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionExtensions.cs

+using Microsoft.AspNetCore.Builder;
+using Microsoft.Extensions.Options;
+
+namespace Microsoft.AspNetCore.ResponseCompression


IApplicationBuilder extensions go into the Microsoft.AspNetCore.Builder namespace

Sometimes we'll move the Options there too, but we're less consistent about that.

Tratcher · 2016-09-23T22:53:44Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionOptions.cs

+        /// 'False' to enable compression only on HTTP request. Enable compression on HTTPS request
+        /// may lead to security problems.
+        /// </summary>
+        public bool EnableHttps { get; set; } = false;


@blowdart should this option even be available?

No it shouldn't. Remove.

Tratcher · 2016-09-23T23:02:48Z

src/Microsoft.AspNetCore.ResponseCompression/project.json

+        ],
+        "xmlDoc": true
+    },
+    "description": "ASP.NET Core basic middleware for HTTP Response compression.",


remove basic

Tratcher · 2016-09-23T23:04:14Z

src/Microsoft.AspNetCore.ResponseCompression/Properties/AssemblyInfo.cs

+// General Information about an assembly is controlled through the following
+// set of attributes. Change these attribute values to modify the information
+// associated with an assembly.
+[assembly: AssemblyConfiguration("")]


Copy from https://github.com/aspnet/BasicMiddleware/blob/dev/src/Microsoft.AspNetCore.HttpOverrides/Properties/AssemblyInfo.cs

Tratcher · 2016-09-23T23:13:14Z

test/Microsoft.AspNetCore.ResponseCompression.Tests/ResponseCompressionMiddlewareTest.cs

+            Assert.Equal(expectedLength, httpContext.Response.Body.Length);
+        }
+
+        private async Task<HttpResponse> InvokeMiddleware(int uncompressedBodyLength, string requestAcceptEncoding, string responseType, Action<HttpResponse> addResponseAction = null)


Use TestServer to arrange this. See https://github.com/aspnet/BasicMiddleware/blob/dev/test/Microsoft.AspNetCore.HttpOverrides.Tests/ForwardedHeadersMiddlewareTest.cs#L22

Tratcher · 2016-09-23T23:27:55Z

src/Microsoft.AspNetCore.ResponseCompression/BodyWrapperStream.cs

+
+        public override Task FlushAsync(CancellationToken cancellationToken)
+        {
+            if (_compressionStream != null)


This needs to call OnWrite() if the user is just trying to flush the headers

Tratcher · 2016-09-23T23:27:59Z

src/Microsoft.AspNetCore.ResponseCompression/BodyWrapperStream.cs

+
+        public override void Flush()
+        {
+            if (_compressionStream != null)


- Replace MIME types filter by a more generic delegate

Tratcher · 2016-09-26T16:46:24Z

src/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionOptions.cs

+        /// 'False' to enable compression only on HTTP requests. Enable compression on HTTPS requests
+        /// may lead to security problems.
+        /// </summary>
+        public bool EnableHttps { get; set; } = false;


We discussed this offline. It can stay, but will remain off by default. bing, google, msn, and wikipedia all enable this, so we can't outright prohibit it.

Tratcher · 2016-09-26T17:05:34Z

@RehanSaeed there are a few different issues with request compression. A) There's no opportunity for the client and server to negotiate the compression protocol, the client needs to know in advance what the server supports. B) the server may not need to decompress the body for a given operation. E.g. for your compressed log files, what if the web server only wanted to save them to disk in their compressed format.

Let's not try to put these two behaviors into one middleware. However, there may be some re-usable components between the two.

You can check with @DamianEdwards, but I don't anticipate accepting a contribution for request decompression. https://github.com/aspnet-contrib may be a better place for it.

Tratcher

Thanks for the update, it looks pretty good. The options still need some refinements, but I'll merge it and take over from here.

Tratcher · 2016-09-26T17:43:29Z

Rebased, squashed, and merged.

Response Compression middleware

1329888

dnfclas added the cla-already-signed label Aug 4, 2016

Tratcher reviewed Aug 5, 2016
View reviewed changes

PR review

2e3367c

- Do not buffer always the response body. - Remove optional parameters before analyzing the MIME type

Tratcher reviewed Aug 5, 2016
View reviewed changes

PR Review: Removing temporary memory stream.

8982609

troydai reviewed Aug 18, 2016
View reviewed changes

PR Review: 'using' statements order

b439ecd

troydai reviewed Aug 19, 2016
View reviewed changes

PR Review:

f1f2590

- New 'EnableHttps' flag - Small source code feedbacks

RehanSaeed mentioned this pull request Sep 2, 2016

Add HTTP compression middleware #34

Closed

Yves57 added 3 commits September 3, 2016 12:34

Compression provider selection closer to RFC 'Accept-Encoding' specif…

844c9d9

…ication

Clean SLN file

fd63117

Do not use 'RequestHeaders' class anymore (one less allocation)

47d84f7

davidfowl reviewed Sep 11, 2016
View reviewed changes

Using "Http.Abstractions" is enough.

8646d37

Tratcher self-assigned this Sep 22, 2016

Tratcher suggested changes Sep 23, 2016

View reviewed changes

Yves57 added 2 commits September 25, 2016 15:32

- Coding style

6a73b8e

- Replace MIME types filter by a more generic delegate

Removing AssemblyInfo.cs in ResponseCompressionTest assembly

61032ae

Tratcher reviewed Sep 26, 2016

View reviewed changes

Tratcher approved these changes Sep 26, 2016

View reviewed changes

Tratcher added this to the 1.1.0 milestone Sep 26, 2016

Tratcher closed this Sep 26, 2016

Tratcher added 3 - Done enhancement labels Sep 26, 2016

Response Compression middleware #52

Response Compression middleware #52

Conversation

Yves57 commented Aug 4, 2016 • edited

dnfclas commented Aug 4, 2016

Choose a reason for hiding this comment

Yves57 Aug 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Aug 19, 2016 • edited

Choose a reason for hiding this comment

blowdart commented Aug 19, 2016

Tratcher commented Aug 19, 2016

blowdart commented Aug 19, 2016

DamianEdwards commented Aug 19, 2016

Yves57 commented Aug 19, 2016

Yves57 commented Aug 19, 2016 • edited

Tratcher commented Aug 20, 2016

Yves57 commented Aug 22, 2016 • edited

RehanSaeed commented Aug 25, 2016 • edited

RehanSaeed commented Aug 25, 2016 • edited

MIME Types

Minimum Size Option

Yves57 commented Aug 25, 2016 • edited

dodyg commented Sep 11, 2016

Yves57 commented Sep 11, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dodyg commented Sep 12, 2016

andrewlock commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Sep 26, 2016

Tratcher left a comment

Choose a reason for hiding this comment

Tratcher commented Sep 26, 2016

Yves57 commented Aug 4, 2016 •

edited

Yves57 Aug 5, 2016 •

edited

davidfowl commented Aug 19, 2016 •

edited

Yves57 commented Aug 19, 2016 •

edited

Yves57 commented Aug 22, 2016 •

edited

RehanSaeed commented Aug 25, 2016 •

edited

RehanSaeed commented Aug 25, 2016 •

edited

Yves57 commented Aug 25, 2016 •

edited

Yves57 commented Sep 11, 2016 •

edited