Repository: netty/netty
Issue: io.netty.handler.codec.http.HttpContentCompressor need compress threshold
Labels: improvement
feature

### Expected behavior
the response body is compressed, when the response body exceeds the threshold
### Actual behavior
no threshold can be set
### Steps to reproduce
-
### Minimal yet complete reproducer code (or URL to code)
-
### Netty version
4.1.16.Final
### JVM version (e.g. `java -version`)
java 1.8
### OS version (e.g. `uname -a`)
-

Issue comments:
@teaey we will need some more details etc to be able to fix this. Can you give some more or share some code ?
@normanmaurer  Please see : https://github.com/teaey/netty/commit/1cf24675c9e90d29d799e6184a9da13a2984a0f9

Can I make a PR for this Issue.
@teaey looks useful... yeah please open a PR and I will comment there. Thanks!
@normanmaurer  Done
@normanmaurer ，I suggested he to make a PR, he is sitting beside me now.
@hepin1989 awesome :) Thanks @teaey
@teaey  I enjoy this enhancement, good job. 
This one is a little less obvious and there are two components. For starters, if you’re compressing data that are smaller than the MTU size of a TCP packet, you’re wasting CPU time. 1500 bytes is the MTU size for the internet since that is the largest size allowed at the network layer. If you take data package that is 1300 bytes and compress it to 800 bytes, it’s still transmitted in that same 1500 byte packet regardless, so you’ve gained nothing. That being the case, you should restrict the gzip compression to data bytes with a size greater than a single packet, 1400 bytes (1.4KB) is a safe value.

just set this threshold with 1400+ as default value. avoid to CPU cost.
@pli2014 I think we should keep 0 as default value to not change behaviour. 
@normanmaurer  using this default value in constructor method or comment this explanation within code.  I propose this for other users.
Maybe we could introduce a static constant field as a suggested value?
@hepin1989 @pli2014 I think we should not suggest anything as what makes sense highly depends... for example even if you use smaller buffers as the mtu size it may make sense if you do a gathering write.
Let users decide how to use it.
Merged...

Pull request review comments:
nit: can you move this to the other final variables.
add whitespace after `,`
We need to keep the existing constructor to not break the API. Please just create an extra constructor and delegate to it.
Explain that `<= 0` will disable this.
add space after `if`

@teaey please reword to:

Returns {@code true} if the compression should be used. May be override to change default behaviour
rename to `useCompression(...)`
nit: s/If/if/
change to: `{@code 0}` will enable compression for all responses.
@teaey sorry one last nit... I think this should be the first thing to check as we not need to even compare headers if it is false. Please move it before the header check
This looks like it only works properly for a FullHttpResponse. If a HttpResponse was sent separately than a HttpContent(s), then this won't compress. Is that intended?
sizeThreshold? payloadSizeThreshold? compressThreshold just makes me question what it means (e.g., threshold of compression ratio?).
Why is this protected?
I'd feel better with throwing if negative.
Hi @ejona86. 

Users can implement the subclass to overwrite the useCompression method, which may access the compressThreshold field.
Yes, throwing is  in line with other fields.
@normanmaurer What's your opinion?
👍 
What about contentSizeThreshold?
This is in line with the original class.
SGTM
Oh, I see, although the extending class passed it in (so it could save it itself) and I can't come up with anything it would do with it.

Speaking of which, why is `useCompression` protected? If I'm extending this class can't I override `beginEncode` with basically same amount of difficulty? It also wasn't necessary before; why is it needed now with threshold?

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentCompressor.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentCompressor.java
index fcf22e011ad..e80469f26e4 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentCompressor.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentCompressor.java
@@ -32,6 +32,7 @@ public class HttpContentCompressor extends HttpContentEncoder {
     private final int compressionLevel;
     private final int windowBits;
     private final int memLevel;
+    private final int contentSizeThreshold;
     private ChannelHandlerContext ctx;
 
     /**
@@ -52,7 +53,7 @@ public HttpContentCompressor() {
      *        compression level is {@code 6}.
      */
     public HttpContentCompressor(int compressionLevel) {
-        this(compressionLevel, 15, 8);
+        this(compressionLevel, 15, 8, 0);
     }
 
     /**
@@ -75,6 +76,33 @@ public HttpContentCompressor(int compressionLevel) {
      *        at the expense of memory usage.  The default value is {@code 8}
      */
     public HttpContentCompressor(int compressionLevel, int windowBits, int memLevel) {
+        this(compressionLevel, windowBits, memLevel, 0);
+    }
+
+    /**
+     * Creates a new handler with the specified compression level, window size,
+     * and memory level..
+     *
+     * @param compressionLevel
+     *        {@code 1} yields the fastest compression and {@code 9} yields the
+     *        best compression.  {@code 0} means no compression.  The default
+     *        compression level is {@code 6}.
+     * @param windowBits
+     *        The base two logarithm of the size of the history buffer.  The
+     *        value should be in the range {@code 9} to {@code 15} inclusive.
+     *        Larger values result in better compression at the expense of
+     *        memory usage.  The default value is {@code 15}.
+     * @param memLevel
+     *        How much memory should be allocated for the internal compression
+     *        state.  {@code 1} uses minimum memory and {@code 9} uses maximum
+     *        memory.  Larger values result in better and faster compression
+     *        at the expense of memory usage.  The default value is {@code 8}
+     * @param contentSizeThreshold
+     *        The response body is compressed when the size of the response
+     *        body exceeds the threshold. The value should be a non negative
+     *        number. {@code 0} will enable compression for all responses.
+     */
+    public HttpContentCompressor(int compressionLevel, int windowBits, int memLevel, int contentSizeThreshold) {
         if (compressionLevel < 0 || compressionLevel > 9) {
             throw new IllegalArgumentException(
                     "compressionLevel: " + compressionLevel +
@@ -88,9 +116,14 @@ public HttpContentCompressor(int compressionLevel, int windowBits, int memLevel)
             throw new IllegalArgumentException(
                     "memLevel: " + memLevel + " (expected: 1-9)");
         }
+        if (contentSizeThreshold < 0) {
+            throw new IllegalArgumentException(
+                    "contentSizeThreshold: " + contentSizeThreshold + " (expected: non negative number)");
+        }
         this.compressionLevel = compressionLevel;
         this.windowBits = windowBits;
         this.memLevel = memLevel;
+        this.contentSizeThreshold = contentSizeThreshold;
     }
 
     @Override
@@ -100,6 +133,13 @@ public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
 
     @Override
     protected Result beginEncode(HttpResponse headers, String acceptEncoding) throws Exception {
+        if (this.contentSizeThreshold > 0) {
+            if (headers instanceof HttpContent &&
+                    ((HttpContent) headers).content().readableBytes() < contentSizeThreshold) {
+                return null;
+            }
+        }
+
         String contentEncoding = headers.headers().get(HttpHeaderNames.CONTENT_ENCODING);
         if (contentEncoding != null) {
             // Content-Encoding was set, either as something specific or as the IDENTITY encoding


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java
index c6ff3bd6bc0..6ab2ab88c62 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpContentCompressorTest.java
@@ -29,11 +29,7 @@
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.CoreMatchers.nullValue;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertSame;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.*;
 
 public class HttpContentCompressorTest {
 
@@ -454,6 +450,52 @@ public void testCustomEncoding() throws Exception {
         assertTrue(ch.finishAndReleaseAll());
     }
 
+    @Test
+    public void testCompressThresholdAllCompress() throws Exception {
+        EmbeddedChannel ch = new EmbeddedChannel(new HttpContentCompressor());
+        assertTrue(ch.writeInbound(newRequest()));
+
+        FullHttpResponse res1023 = new DefaultFullHttpResponse(
+                HttpVersion.HTTP_1_1, HttpResponseStatus.OK,
+                Unpooled.wrappedBuffer(new byte[1023]));
+        assertTrue(ch.writeOutbound(res1023));
+        DefaultHttpResponse response1023 = ch.readOutbound();
+        assertThat(response1023.headers().get(HttpHeaderNames.CONTENT_ENCODING), is("gzip"));
+        ch.releaseOutbound();
+
+        assertTrue(ch.writeInbound(newRequest()));
+        FullHttpResponse res1024 = new DefaultFullHttpResponse(
+                HttpVersion.HTTP_1_1, HttpResponseStatus.OK,
+                Unpooled.wrappedBuffer(new byte[1024]));
+        assertTrue(ch.writeOutbound(res1024));
+        DefaultHttpResponse response1024 = ch.readOutbound();
+        assertThat(response1024.headers().get(HttpHeaderNames.CONTENT_ENCODING), is("gzip"));
+        assertTrue(ch.finishAndReleaseAll());
+    }
+
+    @Test
+    public void testCompressThresholdNotCompress() throws Exception {
+        EmbeddedChannel ch = new EmbeddedChannel(new HttpContentCompressor(6, 15, 8, 1024));
+        assertTrue(ch.writeInbound(newRequest()));
+
+        FullHttpResponse res1023 = new DefaultFullHttpResponse(
+                HttpVersion.HTTP_1_1, HttpResponseStatus.OK,
+                Unpooled.wrappedBuffer(new byte[1023]));
+        assertTrue(ch.writeOutbound(res1023));
+        DefaultHttpResponse response1023 = ch.readOutbound();
+        assertFalse(response1023.headers().contains(HttpHeaderNames.CONTENT_ENCODING));
+        ch.releaseOutbound();
+
+        assertTrue(ch.writeInbound(newRequest()));
+        FullHttpResponse res1024 = new DefaultFullHttpResponse(
+                HttpVersion.HTTP_1_1, HttpResponseStatus.OK,
+                Unpooled.wrappedBuffer(new byte[1024]));
+        assertTrue(ch.writeOutbound(res1024));
+        DefaultHttpResponse response1024 = ch.readOutbound();
+        assertThat(response1024.headers().get(HttpHeaderNames.CONTENT_ENCODING), is("gzip"));
+        assertTrue(ch.finishAndReleaseAll());
+    }
+
     private static FullHttpRequest newRequest() {
         FullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/");
         req.headers().set(HttpHeaderNames.ACCEPT_ENCODING, "gzip");