Repository: netty/netty
Issue: HttpObjectDecoder ignores HTTP trailer header.
Labels: defect

Hi Netty team !

### Behavior
When I was writing test for https://github.com/netty/netty/pull/8721 in **io.netty.handler.codec.http.HttpContentDecoderTest** I encountered a problem with parsing trailer header using HttpObjectDecoder. 
HttpObjectDecoder is not able to handle following bytes sequence 
```java
assertTrue(channel.writeInbound(Unpooled.copiedBuffer("My-Trailer: 42", CharsetUtil.US_ASCII)));
assertTrue(channel.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));
assertTrue(channel.writeInbound(Unpooled.copiedBuffer("\r\n\r\n", CharsetUtil.US_ASCII)));
```
but is able to deal with that 
```java
assertTrue(channel.writeInbound(Unpooled.copiedBuffer("My-Trailer: 42\r\n\r\n\r\n", CharsetUtil.US_ASCII)));
```
After small debugging session, I noticed that HttpObjectDecoder is actually parsing trailer but that line of  code (**io.netty.handler.codec.http.HttpObjectDecoder:674**)
```java
line = headerParser.parse(buffer);
if (line == null) {
    return null;
}
```
cause to ignore my trailer, because **headerParser.parse(buffer)** on empty **buffer** will return null instead of parsed trailer.

### Minimal yet complete reproducer code (or URL to code)
Didn't want to make pull-request (almost exact test is already pushed here https://github.com/netty/netty/pull/8721), but following test (class **io.netty.handler.codec.http.HttpContentDecoderTest**) will reproduce the problem
```java
@Test
public void testChunkedRequestDecompression() {
    HttpResponseDecoder decoder = new HttpResponseDecoder();
    HttpContentDecoder decompressor = new HttpContentDecompressor();

    EmbeddedChannel channel = new EmbeddedChannel(decoder, decompressor, null);

    String headers = "HTTP/1.1 200 OK\r\n"
            + "Transfer-Encoding: chunked\r\n"
            + "Trailer: My-Trailer\r\n"
            + "Content-Encoding: gzip\r\n\r\n";

    channel.writeInbound(Unpooled.copiedBuffer((headers.getBytes(CharsetUtil.US_ASCII))));

    assertTrue(channel.writeInbound(Unpooled.copiedBuffer(Integer.toHexString(GZ_HELLO_WORLD.length) + "\r\n", CharsetUtil.US_ASCII)));
    assertTrue(channel.writeInbound(Unpooled.copiedBuffer(GZ_HELLO_WORLD)));
    assertTrue(channel.writeInbound(Unpooled.copiedBuffer("\r\n".getBytes(CharsetUtil.US_ASCII))));
    assertTrue(channel.writeInbound(Unpooled.copiedBuffer("0\r\n", CharsetUtil.US_ASCII)));
    assertTrue(channel.writeInbound(Unpooled.copiedBuffer("My-Trailer: 42", CharsetUtil.US_ASCII)));
    assertTrue(channel.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));
    assertTrue(channel.writeInbound(Unpooled.copiedBuffer("\r\n\r\n", CharsetUtil.US_ASCII)));

    Object ob1 = channel.readInbound();
    assertThat(ob1, is(instanceOf(DefaultHttpResponse.class)));

    Object ob2 = channel.readInbound();
    assertThat(ob1, is(instanceOf(DefaultHttpResponse.class)));
    HttpContent content = (HttpContent) ob2;
    assertEquals(HELLO_WORLD, content.content().toString(CharsetUtil.US_ASCII));

    Object ob3 = channel.readInbound();
    assertThat(ob1, is(instanceOf(DefaultHttpResponse.class)));
    LastHttpContent lastContent = (LastHttpContent) ob3;
    assertFalse(lastContent.trailingHeaders().isEmpty());
    assertEquals("42", lastContent.trailingHeaders().get("My-Trailer"));
    assertHasInboundMessages(channel, false);
    assertHasOutboundMessages(channel, false);
    assertFalse(channel.finish());
}
```
### Netty version
4.1.32.Final

Issue comments:

Pull request review comments:
This flow is a bit convoluted. How about we remove the surrounding if and make it clear why the condition is there:

```
if (line.length() == 0 && this.trailer == null) {
  return LastHttpContent.EMPTY_LAST_CONTENT; // optimization
}
// everything from the if today
LastHttpContent trailer = this.trailer;
if (trailer == null) {
...
return trailer;
```

Alternatively the if could be changed to `line.length() > 0 || this.trailer != null`, but it's non-obvious why it would be that way. (It's still more obvious than this code, in my mind, though.)

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
index af1d642a039..f8220cdb04e 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
@@ -640,49 +640,50 @@ private LastHttpContent readTrailingHeaders(ByteBuf buffer) {
         if (line == null) {
             return null;
         }
+        LastHttpContent trailer = this.trailer;
+        if (line.length() == 0 && trailer == null) {
+            // We have received the empty line which signals the trailer is complete and did not parse any trailers
+            // before. Just return an empty last content to reduce allocations.
+            return LastHttpContent.EMPTY_LAST_CONTENT;
+        }
+
         CharSequence lastHeader = null;
-        if (line.length() > 0) {
-            LastHttpContent trailer = this.trailer;
-            if (trailer == null) {
-                trailer = this.trailer = new DefaultLastHttpContent(Unpooled.EMPTY_BUFFER, validateHeaders);
-            }
-            do {
-                char firstChar = line.charAt(0);
-                if (lastHeader != null && (firstChar == ' ' || firstChar == '\t')) {
-                    List<String> current = trailer.trailingHeaders().getAll(lastHeader);
-                    if (!current.isEmpty()) {
-                        int lastPos = current.size() - 1;
-                        //please do not make one line from below code
-                        //as it breaks +XX:OptimizeStringConcat optimization
-                        String lineTrimmed = line.toString().trim();
-                        String currentLastPos = current.get(lastPos);
-                        current.set(lastPos, currentLastPos + lineTrimmed);
-                    }
-                } else {
-                    splitHeader(line);
-                    CharSequence headerName = name;
-                    if (!HttpHeaderNames.CONTENT_LENGTH.contentEqualsIgnoreCase(headerName) &&
+        if (trailer == null) {
+            trailer = this.trailer = new DefaultLastHttpContent(Unpooled.EMPTY_BUFFER, validateHeaders);
+        }
+        while (line.length() > 0) {
+            char firstChar = line.charAt(0);
+            if (lastHeader != null && (firstChar == ' ' || firstChar == '\t')) {
+                List<String> current = trailer.trailingHeaders().getAll(lastHeader);
+                if (!current.isEmpty()) {
+                    int lastPos = current.size() - 1;
+                    //please do not make one line from below code
+                    //as it breaks +XX:OptimizeStringConcat optimization
+                    String lineTrimmed = line.toString().trim();
+                    String currentLastPos = current.get(lastPos);
+                    current.set(lastPos, currentLastPos + lineTrimmed);
+                }
+            } else {
+                splitHeader(line);
+                CharSequence headerName = name;
+                if (!HttpHeaderNames.CONTENT_LENGTH.contentEqualsIgnoreCase(headerName) &&
                         !HttpHeaderNames.TRANSFER_ENCODING.contentEqualsIgnoreCase(headerName) &&
                         !HttpHeaderNames.TRAILER.contentEqualsIgnoreCase(headerName)) {
-                        trailer.trailingHeaders().add(headerName, value);
-                    }
-                    lastHeader = name;
-                    // reset name and value fields
-                    name = null;
-                    value = null;
-                }
-
-                line = headerParser.parse(buffer);
-                if (line == null) {
-                    return null;
+                    trailer.trailingHeaders().add(headerName, value);
                 }
-            } while (line.length() > 0);
-
-            this.trailer = null;
-            return trailer;
+                lastHeader = name;
+                // reset name and value fields
+                name = null;
+                value = null;
+            }
+            line = headerParser.parse(buffer);
+            if (line == null) {
+                return null;
+            }
         }
 
-        return LastHttpContent.EMPTY_LAST_CONTENT;
+        this.trailer = null;
+        return trailer;
     }
 
     protected abstract boolean isDecodingRequest();


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java
index 017dbd5ff94..f062da2bf1f 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java
@@ -683,4 +683,33 @@ public void testConnectionClosedBeforeHeadersReceived() {
         assertThat(message.decoderResult().cause(), instanceOf(PrematureChannelClosureException.class));
         assertNull(channel.readInbound());
     }
+
+    @Test
+    public void testTrailerWithEmptyLineInSeparateBuffer() {
+        HttpResponseDecoder decoder = new HttpResponseDecoder();
+        EmbeddedChannel channel = new EmbeddedChannel(decoder);
+
+        String headers = "HTTP/1.1 200 OK\r\n"
+                + "Transfer-Encoding: chunked\r\n"
+                + "Trailer: My-Trailer\r\n";
+        assertFalse(channel.writeInbound(Unpooled.copiedBuffer(headers.getBytes(CharsetUtil.US_ASCII))));
+        assertTrue(channel.writeInbound(Unpooled.copiedBuffer("\r\n".getBytes(CharsetUtil.US_ASCII))));
+
+        assertTrue(channel.writeInbound(Unpooled.copiedBuffer("0\r\n", CharsetUtil.US_ASCII)));
+        assertTrue(channel.writeInbound(Unpooled.copiedBuffer("My-Trailer: 42\r\n", CharsetUtil.US_ASCII)));
+        assertTrue(channel.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));
+
+        HttpResponse response = channel.readInbound();
+        assertEquals(2, response.headers().size());
+        assertEquals("chunked", response.headers().get(HttpHeaderNames.TRANSFER_ENCODING));
+        assertEquals("My-Trailer", response.headers().get(HttpHeaderNames.TRAILER));
+
+        LastHttpContent lastContent = channel.readInbound();
+        assertEquals(1, lastContent.trailingHeaders().size());
+        assertEquals("42", lastContent.trailingHeaders().get("My-Trailer"));
+        assertEquals(0, lastContent.content().readableBytes());
+        lastContent.release();
+
+        assertFalse(channel.finish());
+    }
 }