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.
This commit is contained in:
Bryce Anderson 2016-11-04 14:22:16 -07:00 committed by Norman Maurer
parent 5eebe9a06c
commit f0f0edbf78
2 changed files with 33 additions and 14 deletions

View File

@ -28,6 +28,7 @@ import io.netty.handler.codec.TooLongFrameException;
import io.netty.util.internal.logging.InternalLogger; import io.netty.util.internal.logging.InternalLogger;
import io.netty.util.internal.logging.InternalLoggerFactory; 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.HttpHeaderNames.CONTENT_LENGTH;
import static io.netty.handler.codec.http.HttpUtil.getContentLength; 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); new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.CONTINUE, Unpooled.EMPTY_BUFFER);
private static final FullHttpResponse EXPECTATION_FAILED = new DefaultFullHttpResponse( private static final FullHttpResponse EXPECTATION_FAILED = new DefaultFullHttpResponse(
HttpVersion.HTTP_1_1, HttpResponseStatus.EXPECTATION_FAILED, Unpooled.EMPTY_BUFFER); 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); 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 { static {
EXPECTATION_FAILED.headers().set(CONTENT_LENGTH, 0); EXPECTATION_FAILED.headers().set(CONTENT_LENGTH, 0);
TOO_LARGE.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; private final boolean closeOnExpectationFailed;
@ -216,22 +222,31 @@ public class HttpObjectAggregator
protected void handleOversizedMessage(final ChannelHandlerContext ctx, HttpMessage oversized) throws Exception { protected void handleOversizedMessage(final ChannelHandlerContext ctx, HttpMessage oversized) throws Exception {
if (oversized instanceof HttpRequest) { if (oversized instanceof HttpRequest) {
// send back a 413 and close the connection // 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 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 keep-alive is off and 'Expect: 100-continue' is missing, no need to leave the connection open.
if (oversized instanceof FullHttpMessage || if (oversized instanceof FullHttpMessage ||
!HttpUtil.is100ContinueExpected(oversized) && !HttpUtil.isKeepAlive(oversized)) { !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 // If an oversized request was handled properly and the connection is still alive

View File

@ -162,7 +162,7 @@ public class HttpObjectAggregatorTest {
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status()); assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
assertEquals("0", response.headers().get(HttpHeaderNames.CONTENT_LENGTH)); assertEquals("0", response.headers().get(HttpHeaderNames.CONTENT_LENGTH));
if (serverShouldCloseConnection(message)) { if (serverShouldCloseConnection(message, response)) {
assertFalse(embedder.isOpen()); assertFalse(embedder.isOpen());
assertFalse(embedder.finish()); assertFalse(embedder.finish());
} else { } 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, // The connection should only be kept open if Expect: 100-continue is set,
// or if keep-alive is on. // or if keep-alive is on.
if (HttpUtil.is100ContinueExpected(message)) { if (HttpUtil.is100ContinueExpected(message)) {