Repository: netty/netty
Issue: HttpObjectAggregator closes the http/1.x connection without a 'Connection: close' header
Labels: defect

Netty version: 4.1.6.Final

The [HttpObjectAggregator](https://github.com/netty/netty/blob/eb7f751ba519cbcab47d640cd18757f09d077b55/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java) will take it upon itself to return a 413 response when the message has too large of body [here](https://github.com/netty/netty/blob/eb7f751ba519cbcab47d640cd18757f09d077b55/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java#L216-L249). On the response it sends back, it does not add a `Connection: close` header, presumably to place nice with 100-continue requests. However, for the common case the connection _is_ closed, and by unexpectedly closing the connection, netty can trigger problems for clients which attempt to reuse the connection since it has not received notice that its being closed.

Issue comments:
Fixed by https://github.com/netty/netty/pull/5983


Pull request review comments:
just make this one listener which always close after logging the error ? No need to have to listeners and at worst have both call close. After this I will merge.

@bryce-anderson thanks!

Done. Let me know if I misunderstood your comment. 😄 

@bryce-anderson remove empty line and we are good :)

Done. Sorry for the noise.


Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java
index 908ac8d12b4..4e8455236cf 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java
@@ -28,6 +28,7 @@
 import io.netty.util.internal.logging.InternalLogger;
 import io.netty.util.internal.logging.InternalLoggerFactory;
 
+import static io.netty.handler.codec.http.HttpHeaderNames.CONNECTION;
 import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_LENGTH;
 import static io.netty.handler.codec.http.HttpUtil.getContentLength;
 
@@ -89,12 +90,17 @@ public class HttpObjectAggregator
             new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.CONTINUE, Unpooled.EMPTY_BUFFER);
     private static final FullHttpResponse EXPECTATION_FAILED = new DefaultFullHttpResponse(
             HttpVersion.HTTP_1_1, HttpResponseStatus.EXPECTATION_FAILED, Unpooled.EMPTY_BUFFER);
-    private static final FullHttpResponse TOO_LARGE = new DefaultFullHttpResponse(
+    private static final FullHttpResponse TOO_LARGE_CLOSE = new DefaultFullHttpResponse(
             HttpVersion.HTTP_1_1, HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, Unpooled.EMPTY_BUFFER);
+    private static final FullHttpResponse TOO_LARGE = new DefaultFullHttpResponse(
+        HttpVersion.HTTP_1_1, HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, Unpooled.EMPTY_BUFFER);
 
     static {
         EXPECTATION_FAILED.headers().set(CONTENT_LENGTH, 0);
         TOO_LARGE.headers().set(CONTENT_LENGTH, 0);
+
+        TOO_LARGE_CLOSE.headers().set(CONTENT_LENGTH, 0);
+        TOO_LARGE_CLOSE.headers().set(CONNECTION, HttpHeaderValues.CLOSE);
     }
 
     private final boolean closeOnExpectationFailed;
@@ -216,22 +222,31 @@ protected void finishAggregation(FullHttpMessage aggregated) throws Exception {
     protected void handleOversizedMessage(final ChannelHandlerContext ctx, HttpMessage oversized) throws Exception {
         if (oversized instanceof HttpRequest) {
             // send back a 413 and close the connection
-            ChannelFuture future = ctx.writeAndFlush(TOO_LARGE.retainedDuplicate()).addListener(
-                    new ChannelFutureListener() {
-                @Override
-                public void operationComplete(ChannelFuture future) throws Exception {
-                    if (!future.isSuccess()) {
-                        logger.debug("Failed to send a 413 Request Entity Too Large.", future.cause());
-                        ctx.close();
-                    }
-                }
-            });
 
             // If the client started to send data already, close because it's impossible to recover.
             // If keep-alive is off and 'Expect: 100-continue' is missing, no need to leave the connection open.
             if (oversized instanceof FullHttpMessage ||
                 !HttpUtil.is100ContinueExpected(oversized) && !HttpUtil.isKeepAlive(oversized)) {
-                future.addListener(ChannelFutureListener.CLOSE);
+                ChannelFuture future = ctx.writeAndFlush(TOO_LARGE_CLOSE.retainedDuplicate());
+                future.addListener(new ChannelFutureListener() {
+                    @Override
+                    public void operationComplete(ChannelFuture future) throws Exception {
+                        if (!future.isSuccess()) {
+                            logger.debug("Failed to send a 413 Request Entity Too Large.", future.cause());
+                        }
+                        ctx.close();
+                    }
+                });
+            } else {
+                ctx.writeAndFlush(TOO_LARGE.retainedDuplicate()).addListener(new ChannelFutureListener() {
+                    @Override
+                    public void operationComplete(ChannelFuture future) throws Exception {
+                        if (!future.isSuccess()) {
+                            logger.debug("Failed to send a 413 Request Entity Too Large.", future.cause());
+                            ctx.close();
+                        }
+                    }
+                });
             }
 
             // If an oversized request was handled properly and the connection is still alive


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java
index ca4a5f3afa9..69464acf6aa 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java
@@ -162,7 +162,7 @@ private static void checkOversizedRequest(HttpRequest message) {
         assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
         assertEquals("0", response.headers().get(HttpHeaderNames.CONTENT_LENGTH));
 
-        if (serverShouldCloseConnection(message)) {
+        if (serverShouldCloseConnection(message, response)) {
             assertFalse(embedder.isOpen());
             assertFalse(embedder.finish());
         } else {
@@ -170,7 +170,11 @@ private static void checkOversizedRequest(HttpRequest message) {
         }
     }
 
-    private static boolean serverShouldCloseConnection(HttpRequest message) {
+    private static boolean serverShouldCloseConnection(HttpRequest message, HttpResponse response) {
+        // If the response wasn't keep-alive, the server should close the connection.
+        if (!HttpUtil.isKeepAlive(response)) {
+            return true;
+        }
         // The connection should only be kept open if Expect: 100-continue is set,
         // or if keep-alive is on.
         if (HttpUtil.is100ContinueExpected(message)) {