From f0f0edbf7829ddaf9d795124cf6ce6a8ec5b2f51 Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Fri, 4 Nov 2016 14:22:16 -0700 Subject: [PATCH] HttpObjectAggregator adds 'Connection: close' header if necessary Motivation: The HttpObjectAggregator never appends a 'Connection: close' header to the response of oversized messages even though in the majority of cases its going to close the connection. Modification: This PR addresses that by ensuring the requisite header is present when the connection is going to be closed. Result: Gracefully signal that we are about to close the connection. --- .../codec/http/HttpObjectAggregator.java | 39 +++++++++++++------ .../codec/http/HttpObjectAggregatorTest.java | 8 +++- 2 files changed, 33 insertions(+), 14 deletions(-) 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 908ac8d12b..4e8455236c 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.handler.codec.TooLongFrameException; 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 @@ public class HttpObjectAggregator 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 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 ca4a5f3afa..69464acf6a 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 @@ public class HttpObjectAggregatorTest { 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 @@ public class HttpObjectAggregatorTest { } } - 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)) {