From 3554646a600d8b20570cad498d7a5c5ce7a966de Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Wed, 8 Nov 2017 07:41:23 -0800 Subject: [PATCH] Correctly convert empty HttpContent to ByteBuf Motivation: 93130b172a61815354267f0f3d8f5af52de39754 introduced a regression where we not "converted" an empty HttpContent to ByteBuf and just passed it on in the pipeline. This can lead to the situation that other handlers in the pipeline will see HttpContent instances which is not expected. Modifications: - Correctly convert HttpContent to ByteBuf when empty - Add unit test. Result: Handlers in the pipeline will see the expected message type. --- .../handler/codec/http/HttpObjectEncoder.java | 15 ++++--- .../codec/http/HttpRequestEncoderTest.java | 43 +++++++++++++++++-- 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java index 7ebfd10782..fe03378bba 100755 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java @@ -17,18 +17,17 @@ package io.netty.handler.codec.http; import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufUtil; +import io.netty.buffer.Unpooled; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.FileRegion; import io.netty.handler.codec.MessageToMessageEncoder; import io.netty.util.CharsetUtil; -import io.netty.util.ReferenceCountUtil; import io.netty.util.internal.StringUtil; import java.util.Iterator; import java.util.List; import java.util.Map.Entry; -import static io.netty.buffer.Unpooled.EMPTY_BUFFER; import static io.netty.buffer.Unpooled.directBuffer; import static io.netty.buffer.Unpooled.unreleasableBuffer; import static io.netty.handler.codec.http.HttpConstants.CR; @@ -142,6 +141,7 @@ public abstract class HttpObjectEncoder extends MessageTo break; } + // fall-through! case ST_CONTENT_ALWAYS_EMPTY: @@ -150,8 +150,13 @@ public abstract class HttpObjectEncoder extends MessageTo out.add(buf); } else { // Need to produce some output otherwise an - // IllegalStateException will be thrown - out.add(EMPTY_BUFFER); + // IllegalStateException will be thrown as we did not write anything + // Its ok to just write an EMPTY_BUFFER as if there are reference count issues these will be + // propagated as the caller of the encode(...) method will release the original + // buffer. + // Writing an empty buffer will not actually write anything on the wire, so if there is a user + // error with msg it will not be visible externally + out.add(Unpooled.EMPTY_BUFFER); } break; @@ -213,7 +218,7 @@ public abstract class HttpObjectEncoder extends MessageTo } else if (contentLength == 0) { // Need to produce some output otherwise an // IllegalStateException will be thrown - out.add(ReferenceCountUtil.retain(msg)); + out.add(encodeAndRetain(msg)); } } diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestEncoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestEncoderTest.java index 6a2ad13e60..a965a6783c 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestEncoderTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestEncoderTest.java @@ -28,9 +28,7 @@ import java.util.concurrent.ExecutionException; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.fail; +import static org.junit.Assert.*; /** */ @@ -146,7 +144,7 @@ public class HttpRequestEncoderTest { } @Test - public void testEmptydBufferShouldPassThrough() throws Exception { + public void testEmptyBufferShouldPassThrough() throws Exception { HttpRequestEncoder encoder = new HttpRequestEncoder(); EmbeddedChannel channel = new EmbeddedChannel(encoder); ByteBuf buffer = Unpooled.buffer(); @@ -154,4 +152,41 @@ public class HttpRequestEncoderTest { channel.finishAndReleaseAll(); assertEquals(0, buffer.refCnt()); } + + @Test + public void testEmptyContentChunked() throws Exception { + testEmptyContent(true); + } + + @Test + public void testEmptyContentNotChunked() throws Exception { + testEmptyContent(false); + } + + private void testEmptyContent(boolean chunked) throws Exception { + HttpRequestEncoder encoder = new HttpRequestEncoder(); + EmbeddedChannel channel = new EmbeddedChannel(encoder); + HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/"); + if (chunked) { + HttpUtil.setTransferEncodingChunked(request, true); + } + assertTrue(channel.writeOutbound(request)); + + ByteBuf contentBuffer = Unpooled.buffer(); + assertTrue(channel.writeOutbound(new DefaultHttpContent(contentBuffer))); + + ByteBuf lastContentBuffer = Unpooled.buffer(); + assertTrue(channel.writeOutbound(new DefaultHttpContent(lastContentBuffer))); + + // Ensure we only produce ByteBuf instances. + ByteBuf head = channel.readOutbound(); + assertTrue(head.release()); + + ByteBuf content = channel.readOutbound(); + content.release(); + + ByteBuf lastContent = channel.readOutbound(); + lastContent.release(); + assertFalse(channel.finish()); + } }