Repository: netty/netty
Issue: Http compression error
Labels: defect

Hi,

Using netty-3.5.0.Final http compression works just fine. After upgrading to netty-3.5.2.Final I found that some resources do not compress completely.

This is evident in the actual sizes of the files as well:

> -rw-rw-r--. 1 roelof roelof 192628 Jul  6 16:14 netty-3.5.0.js
> -rw-rw-r--. 1 roelof roelof 167636 Jul  6 16:14 netty-3.5.2.js

Entries from my access log also confirm this.

netty-3.5.0.Final

> 127.0.0.1 30.507 - 06/07/2012:16:12:42 +0200 "GET /mtnusa_web/style/jquery/jquery-ui-1.7.2.custom.min.js HTTP/1.1" 200 <b>46315</b> "-" "curl/7.21.3 (x86_64-redhat-linux-gnu) libcurl/7.21.3 NSS/3.13.1.0 zlib/1.2.5 libidn/1.19 libssh2/1.2.7"

netty-3.5.2.Final

> 127.0.0.1 31.547 - 06/07/2012:16:13:47 +0200 "GET /mtnusa_web/style/jquery/jquery-ui-1.7.2.custom.min.js HTTP/1.1" 200 <b>40978</b> "-" "curl/7.21.3 (x86_64-redhat-linux-gnu) libcurl/7.21.3 NSS/3.13.1.0 zlib/1.2.5 libidn/1.19 libssh2/1.2.7"

Notice the difference in not only file sizes but also final response sizes.

The only related changes I could find are the following two issues:
[#404](https://github.com/netty/netty/pull/404)
[#406](https://github.com/netty/netty/issues/406)

The http server is running with jdk 7 and thus the <i>JdkZlibEncoder</i> would be used.

Are there any known issues with using JdkZlibEncoder?

Took a quick look at the code for JdkZlipEncoder. Lines 236 - 238:

``` java
           if (!deflater.finished()) {
              numBytes = deflater.deflate(out, 0, out.length);
            }
```

Should the deflate not be called with <b>SYNC_FLUSH</b>?


Issue comments:
not necessary after calling 'deflater.finish()' on line 235

On Fri, Jul 6, 2012 at 8:00 AM, naude-r
reply@reply.github.com
wrote:

> Hi,
> 
> Using netty-3.5.0.Final http compression works just fine. After upgrading to netty-3.5.2.Final I found that some resources do not compress completely.
> 
> This is evident in the actual sizes of the files as well:
> 
> > -rw-rw-r--. 1 roelof roelof 192628 Jul  6 16:14 netty-3.5.0.js
> > -rw-rw-r--. 1 roelof roelof 167636 Jul  6 16:14 netty-3.5.2.js
> 
> Entries from my access log also confirm this.
> 
> netty-3.5.0.Final
> 
> > 127.0.0.1 30.507 - 06/07/2012:16:12:42 +0200 "GET /mtnusa_web/style/jquery/jquery-ui-1.7.2.custom.min.js HTTP/1.1" 200 <b>46315</b> "-" "curl/7.21.3 (x86_64-redhat-linux-gnu) libcurl/7.21.3 NSS/3.13.1.0 zlib/1.2.5 libidn/1.19 libssh2/1.2.7"
> 
> netty-3.5.2.Final
> 
> > 127.0.0.1 31.547 - 06/07/2012:16:13:47 +0200 "GET /mtnusa_web/style/jquery/jquery-ui-1.7.2.custom.min.js HTTP/1.1" 200 <b>40978</b> "-" "curl/7.21.3 (x86_64-redhat-linux-gnu) libcurl/7.21.3 NSS/3.13.1.0 zlib/1.2.5 libidn/1.19 libssh2/1.2.7"
> 
> Notice the difference in not only file sizes but also final response sizes.
> 
> The only related changes I could find are the following two issues:
> [#404](https://github.com/netty/netty/pull/404)
> [#406](https://github.com/netty/netty/issues/406)
> 
> The http server is running with jdk 7 and thus the <i>JdkZlibEncoder</i> would be used.
> 
> Are there any known issues with using JdkZlibEncoder?
> 
> Took a quick look at the code for JdkZlipEncoder. Lines 236 - 238:
> 
> ``` java
>            if (!deflater.finished()) {
>               numBytes = deflater.deflate(out, 0, out.length);
>             }
> ```
> 
> Should the deflate not be called with <b>SYNC_FLUSH</b>?
> 
> ---
> 
> Reply to this email directly or view it on GitHub:
> https://github.com/netty/netty/issues/444

@naude-r would it be possible to provide a unit test that shows the error ?

hi,

managed to find and fixed the bug. there could still be uncompressed data available when calling <i>deflater.deflate</i> after <i>deflater.finish</i>.

the fix is to loop on <i>deflater.finished()</i> and write all output to the channel. my attempt to fix is below. feel free to update as required ;-)

```
--- netty-3.5.2.Final/src/main/java/org/jboss/netty/handler/codec/compression/JdkZlibEncoder.java       2012-07-05 10:32:08.000000000 +0200
+++ netty-3.5.2.Final.new/src/main/java/org/jboss/netty/handler/codec/compression/JdkZlibEncoder.java   2012-07-09 12:35:24.288543119 +0200
@@ -232,14 +232,16 @@
         ChannelBuffer footer = ChannelBuffers.EMPTY_BUFFER;
         synchronized (deflater) {
             int numBytes = 0;
+            int footerSize = gzip ? numBytes + 8 : numBytes;
+            final int estimatedSize = footerSize + out.length;
+            footer = ChannelBuffers.dynamicBuffer(estimatedSize, ctx.getChannel().getConfig().getBufferFactory());
             deflater.finish();
-            if (!deflater.finished()) {
+            while (!deflater.finished()) {
               numBytes = deflater.deflate(out, 0, out.length);
+              footer.writeBytes(out, 0, numBytes);
+              footerSize += numBytes;
             }
-            int footerSize = gzip ? numBytes + 8 : numBytes;
             if (footerSize > 0) {
-                footer = ctx.getChannel().getConfig().getBufferFactory().getBuffer(footerSize);
-                footer.writeBytes(out, 0, numBytes);
                 if (gzip) {
                     int crcValue = (int) crc.getValue();
                     int uncBytes = deflater.getTotalIn();

```

@jpinner sounds good.. wdyt ?

issued pull request: https://github.com/netty/netty/pull/446

@jpinner merged in.. thanks..

@naude-r could you check if the changes which I just pulled in also fix it for you ?

@normanmaurer thank you. that worked perfectly.


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/jboss/netty/handler/codec/compression/JdkZlibEncoder.java b/src/main/java/org/jboss/netty/handler/codec/compression/JdkZlibEncoder.java
index cd7b161be10..59d78a67d33 100644
--- a/src/main/java/org/jboss/netty/handler/codec/compression/JdkZlibEncoder.java
+++ b/src/main/java/org/jboss/netty/handler/codec/compression/JdkZlibEncoder.java
@@ -222,42 +222,41 @@ public void handleDownstream(ChannelHandlerContext ctx, ChannelEvent evt)
     }
 
     private ChannelFuture finishEncode(final ChannelHandlerContext ctx, final ChannelEvent evt) {
+        ChannelFuture future = Channels.succeededFuture(ctx.getChannel());
+
         if (!finished.compareAndSet(false, true)) {
             if (evt != null) {
                 ctx.sendDownstream(evt);
             }
-            return Channels.succeededFuture(ctx.getChannel());
+            return future;
         }
 
-        ChannelBuffer footer = ChannelBuffers.EMPTY_BUFFER;
+        ChannelBuffer footer = ChannelBuffers.dynamicBuffer(ctx.getChannel().getConfig().getBufferFactory());
         synchronized (deflater) {
-            int numBytes = 0;
             deflater.finish();
-            if (!deflater.finished()) {
-              numBytes = deflater.deflate(out, 0, out.length);
-            }
-            int footerSize = gzip ? numBytes + 8 : numBytes;
-            if (footerSize > 0) {
-                footer = ctx.getChannel().getConfig().getBufferFactory().getBuffer(footerSize);
+            while (!deflater.finished()) {
+                int numBytes = deflater.deflate(out, 0, out.length);
                 footer.writeBytes(out, 0, numBytes);
-                if (gzip) {
-                    int crcValue = (int) crc.getValue();
-                    int uncBytes = deflater.getTotalIn();
-                    footer.writeByte(crcValue);
-                    footer.writeByte(crcValue >>> 8);
-                    footer.writeByte(crcValue >>> 16);
-                    footer.writeByte(crcValue >>> 24);
-                    footer.writeByte(uncBytes);
-                    footer.writeByte(uncBytes >>> 8);
-                    footer.writeByte(uncBytes >>> 16);
-                    footer.writeByte(uncBytes >>> 24);
-                }
+            }
+            if (gzip) {
+                int crcValue = (int) crc.getValue();
+                int uncBytes = deflater.getTotalIn();
+                footer.writeByte(crcValue);
+                footer.writeByte(crcValue >>> 8);
+                footer.writeByte(crcValue >>> 16);
+                footer.writeByte(crcValue >>> 24);
+                footer.writeByte(uncBytes);
+                footer.writeByte(uncBytes >>> 8);
+                footer.writeByte(uncBytes >>> 16);
+                footer.writeByte(uncBytes >>> 24);
             }
             deflater.end();
         }
 
-        ChannelFuture future = Channels.future(ctx.getChannel());
-        Channels.write(ctx, future, footer);
+        if (footer.readable()) {
+            future = Channels.future(ctx.getChannel());
+            Channels.write(ctx, future, footer);
+        }
 
         if (evt != null) {
             future.addListener(new ChannelFutureListener() {


Test patch: