Fix the incorrect content-encoding header when sendFile is used in non-SSL

Author: chtyim

src/main/java/io/cdap/http/internal/BasicHttpResponder.java

@@ -26,7 +26,9 @@
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelFutureListener;
+import io.netty.channel.ChannelHandler;
 import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelPipeline;
 import io.netty.channel.DefaultFileRegion;
 import io.netty.channel.FileRegion;
 import io.netty.handler.codec.http.DefaultFullHttpResponse;
@@ -51,6 +53,7 @@
 import java.io.IOException;
 import java.io.RandomAccessFile;
 import java.nio.charset.StandardCharsets;
+import java.util.NoSuchElementException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import javax.annotation.Nullable;
 
@@ -127,10 +130,21 @@ public void sendFile(File file, HttpHeaders headers) throws IOException {
         // The HttpChunkedInput will write out the last content
         channel.writeAndFlush(new HttpChunkedInput(new ChunkedFile(raf, 8192)));
       } else {
-        // The FileRegion will close the file channel when it is done sending.
-        FileRegion region = new DefaultFileRegion(raf.getChannel(), 0, file.length());
-        channel.write(region);
-        channel.writeAndFlush(LastHttpContent.EMPTY_LAST_CONTENT);
+        final Runnable completion = prepareSendFile(channel);
+        try {
+          // The FileRegion will close the file channel when it is done sending.
+          FileRegion region = new DefaultFileRegion(raf.getChannel(), 0, file.length());
+          channel.write(region);
+          channel.writeAndFlush(LastHttpContent.EMPTY_LAST_CONTENT).addListener(new ChannelFutureListener() {
+            @Override
+            public void operationComplete(ChannelFuture future) {
+              completion.run();
+            }
+          });
+        } catch (Throwable t) {
+          completion.run();
+          throw t;
+        }
       }
     } catch (Throwable t) {
       try {
@@ -228,6 +242,35 @@ private void checkNotResponded() {
     }
   }
 
+  /**
+   * Prepares the given {@link Channel} for the sending file.
+   *
+   * @param channel the channel to prepare
+   * @return a {@link Runnable} that should be called when the send file is completed to revert the action
+   */
+  private Runnable prepareSendFile(Channel channel) {
+    // Remove the "compressor" from the pipeline to skip content encoding since FileRegion do zero-copy write,
+    // hence bypassing any user space data operation.
+    try {
+      final ChannelPipeline pipeline = channel.pipeline();
+      final ChannelHandler compressor = pipeline.remove("compressor");
+      return new Runnable() {
+        @Override
+        public void run() {
+          pipeline.addAfter("codec", "compressor", compressor);
+        }
+      };
+    } catch (NoSuchElementException e) {
+      // Ignore if there is no compressor
+      return new Runnable() {
+        @Override
+        public void run() {
+          // no-op
+        }
+      };
+    }
+  }
+
   /**
    * A {@link ChunkedInput} implementation that produce chunks using {@link BodyProducer}.
    */

src/test/java/io/cdap/http/HttpServerTest.java

@@ -280,6 +280,10 @@ public void testUploadDisconnect() throws Exception {
   public void testSendFile() throws IOException {
     File filePath = new File(tmpFolder.newFolder(), "test.txt");
     HttpURLConnection urlConn = request("/test/v1/stream/file", HttpMethod.POST);
+    // Enable accepting compression. For HTTP case, the response won't be compressed since FileRegion is being used.
+    // For HTTPs, the response will be compressed.
+    // See https://github.com/cdapio/netty-http/issues/68
+    urlConn.setRequestProperty(HttpHeaderNames.ACCEPT_ENCODING.toString(), HttpHeaderValues.GZIP_DEFLATE.toString());
     urlConn.setRequestProperty("File-Path", filePath.getAbsolutePath());
     urlConn.getOutputStream().write("content".getBytes(StandardCharsets.UTF_8));
     Assert.assertEquals(200, urlConn.getResponseCode());
@@ -845,7 +849,7 @@ public void testCompressResponse() throws Exception {
     urlConn.setRequestProperty(HttpHeaderNames.ACCEPT_ENCODING.toString(), HttpHeaderValues.GZIP_DEFLATE.toString());
 
     Assert.assertEquals(HttpResponseStatus.OK.code(), urlConn.getResponseCode());
-    Assert.assertTrue(urlConn.getHeaderField(HttpHeaderNames.CONTENT_ENCODING.toString()) != null);
+    Assert.assertNotNull(urlConn.getHeaderField(HttpHeaderNames.CONTENT_ENCODING.toString()));
 
     Assert.assertEquals("Testing message", getContent(urlConn));
 
@@ -854,7 +858,7 @@ public void testCompressResponse() throws Exception {
     urlConn.setRequestProperty(HttpHeaderNames.ACCEPT_ENCODING.toString(), HttpHeaderValues.GZIP_DEFLATE.toString());
 
     Assert.assertEquals(HttpResponseStatus.OK.code(), urlConn.getResponseCode());
-    Assert.assertTrue(urlConn.getHeaderField(HttpHeaderNames.CONTENT_ENCODING.toString()) != null);
+    Assert.assertNotNull(urlConn.getHeaderField(HttpHeaderNames.CONTENT_ENCODING.toString()));
 
     Assert.assertEquals("Testing message chunk", getContent(urlConn));
   }