Don't close the connection whenever Expect: 100-continue is missing.

Motivation:

The 4.1.0-Beta3 implementation of HttpObjectAggregator.handleOversizedMessage closes the
connection if the client sent oversized chunked data with no Expect:
100-continue header. This causes a broken pipe or "connection reset by
peer" error in some clients (tested on Firefox 31 OS X 10.9.5,
async-http-client 1.8.14).

This part of the HTTP 1.1 spec (below) seems to say that in this scenario the connection
should not be closed (unless the intention is to be very strict about
how data should be sent).

http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html

"If an origin server receives a request that does not include an
Expect request-header field with the "100-continue" expectation,
the request includes a request body, and the server responds
with a final status code before reading the entire request body
from the transport connection, then the server SHOULD NOT close
the transport connection until it has read the entire request,
or until the client closes the connection. Otherwise, the client
might not reliably receive the response message. However, this
requirement is not be construed as preventing a server from
defending itself against denial-of-service attacks, or from
badly broken client implementations."

Modifications:

Change HttpObjectAggregator.handleOversizedMessage to close the
connection only if keep-alive is off and Expect: 100-continue is
missing. Update test to reflect the change.

Result:

Broken pipe and connection reset errors on the client are avoided when
oversized data is sent.
This commit is contained in:
George Agnelli 2014-10-07 14:56:15 +01:00 committed by Norman Maurer
parent 789e323b79
commit 0666924e8c
2 changed files with 68 additions and 5 deletions

View File

@ -161,10 +161,9 @@ public class HttpObjectAggregator
});
// If the client started to send data already, close because it's impossible to recover.
// If 'Expect: 100-continue' is missing, close becuase it's impossible to recover.
// If keep-alive is off, 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 ||
!HttpHeaders.is100ContinueExpected(oversized) || !HttpHeaders.isKeepAlive(oversized)) {
(!HttpHeaders.is100ContinueExpected(oversized) && !HttpHeaders.isKeepAlive(oversized))) {
future.addListener(ChannelFutureListener.CLOSE);
}

View File

@ -223,6 +223,53 @@ public class HttpObjectAggregatorTest {
assertFalse(embedder.finish());
}
@Test
public void testRequestAfterOversized100ContinueAndDecoder() {
EmbeddedChannel embedder = new EmbeddedChannel(new HttpRequestDecoder(), new HttpObjectAggregator(15));
// Write first request with Expect: 100-continue
HttpRequest message = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.PUT, "http://localhost");
HttpHeaders.set100ContinueExpected(message);
HttpHeaders.setContentLength(message, 16);
HttpContent chunk1 = releaseLater(new DefaultHttpContent(Unpooled.copiedBuffer("some", CharsetUtil.US_ASCII)));
HttpContent chunk2 = releaseLater(new DefaultHttpContent(Unpooled.copiedBuffer("test", CharsetUtil.US_ASCII)));
HttpContent chunk3 = LastHttpContent.EMPTY_LAST_CONTENT;
// Send a request with 100-continue + large Content-Length header value.
assertFalse(embedder.writeInbound(message));
// The agregator should respond with '413 Request Entity Too Large.'
FullHttpResponse response = embedder.readOutbound();
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
assertEquals("0", response.headers().get(Names.CONTENT_LENGTH));
// An ill-behaving client could continue to send data without a respect, and such data should be discarded.
assertFalse(embedder.writeInbound(chunk1));
// The aggregator should not close the connection because keep-alive is on.
assertTrue(embedder.isOpen());
// Now send a valid request.
HttpRequest message2 = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.PUT, "http://localhost");
assertFalse(embedder.writeInbound(message2));
assertFalse(embedder.writeInbound(chunk2));
assertTrue(embedder.writeInbound(chunk3));
FullHttpRequest fullMsg = embedder.readInbound();
assertNotNull(fullMsg);
assertEquals(
chunk2.content().readableBytes() + chunk3.content().readableBytes(),
HttpHeaders.getContentLength(fullMsg));
assertEquals(HttpHeaders.getContentLength(fullMsg), fullMsg.content().readableBytes());
fullMsg.release();
assertFalse(embedder.finish());
}
private static void checkOversizedRequest(HttpRequest message) {
EmbeddedChannel embedder = new EmbeddedChannel(new HttpObjectAggregator(4));
@ -230,8 +277,25 @@ public class HttpObjectAggregatorTest {
HttpResponse response = embedder.readOutbound();
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
assertEquals("0", response.headers().get(Names.CONTENT_LENGTH));
assertFalse(embedder.isOpen());
assertFalse(embedder.finish());
if (serverShouldCloseConnection(message)) {
assertFalse(embedder.isOpen());
assertFalse(embedder.finish());
} else {
assertTrue(embedder.isOpen());
}
}
private static boolean serverShouldCloseConnection(HttpRequest message) {
// The connection should only be kept open if Expect: 100-continue is set,
// or if keep-alive is on.
if (HttpHeaders.is100ContinueExpected(message)) {
return false;
}
if (HttpHeaders.isKeepAlive(message)) {
return false;
}
return true;
}
@Test