From 5ced37e204778bdd64c8956ea099864a3b917d03 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 8 Apr 2021 12:19:17 +0200 Subject: [PATCH] Fixes #6105 - HttpConnection.getBytesIn() incorrect for requests with chunked content Moved recording of bytes to fillRequestBuffer(), so they are accounted also for async reads. Added test case. Fixed test that was too strictly comparing HttpConnection.bytesIn, that now report a correct, but larger value. Signed-off-by: Simone Bordet (cherry picked from commit aed20abcbe157d06a7e70e0550586222b2aa433c) --- .../eclipse/jetty/server/HttpConnection.java | 9 ++- .../jetty/server/HttpConnectionTest.java | 68 +++++++++++++++++++ .../jetty/test/GzipWithSendErrorTest.java | 6 +- 3 files changed, 76 insertions(+), 7 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index 824b1b216763..3b57dae35457 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -263,9 +263,7 @@ public void onFillable() { // Fill the request buffer (if needed). int filled = fillRequestBuffer(); - if (filled > 0) - bytesIn.add(filled); - else if (filled == -1 && getEndPoint().isOutputShutdown()) + if (filled < 0 && getEndPoint().isOutputShutdown()) close(); // Parse the request buffer. @@ -352,8 +350,9 @@ private int fillRequestBuffer() if (filled == 0) // Do a retry on fill 0 (optimization for SSL connections) filled = getEndPoint().fill(_requestBuffer); - // tell parser - if (filled < 0) + if (filled > 0) + bytesIn.add(filled); + else if (filled < 0) _parser.atEOF(); if (LOG.isDebugEnabled()) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java index 4963430c8189..4d1c0d29d4c1 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java @@ -32,6 +32,8 @@ import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import java.util.stream.Stream; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; @@ -40,6 +42,7 @@ import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpParser; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.logging.StacklessLogging; @@ -47,6 +50,7 @@ import org.eclipse.jetty.server.handler.AbstractHandler; import org.eclipse.jetty.server.handler.ErrorHandler; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.IO; import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -59,9 +63,11 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.not; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; public class HttpConnectionTest @@ -1353,6 +1359,68 @@ public void testCONNECT() throws Exception } } + @Test + public void testBytesIn() throws Exception + { + String chunk1 = "0123456789ABCDEF"; + String chunk2 = IntStream.range(0, 64).mapToObj(i -> chunk1).collect(Collectors.joining()); + long dataLength = chunk1.length() + chunk2.length(); + server.stop(); + server.setHandler(new AbstractHandler() + { + @Override + public void handle(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException + { + jettyRequest.setHandled(true); + IO.copy(request.getInputStream(), IO.getNullStream()); + + HttpConnection connection = HttpConnection.getCurrentConnection(); + long bytesIn = connection.getBytesIn(); + assertThat(bytesIn, greaterThan(dataLength)); + } + }); + server.start(); + + LocalEndPoint localEndPoint = connector.executeRequest("" + + "POST / HTTP/1.1\r\n" + + "Host: localhost\r\n" + + "Content-Length: " + dataLength + "\r\n" + + "\r\n" + + chunk1); + + // Wait for the server to block on the read(). + Thread.sleep(500); + + // Send more content. + localEndPoint.addInput(chunk2); + + HttpTester.Response response = HttpTester.parseResponse(localEndPoint.getResponse()); + assertEquals(response.getStatus(), HttpStatus.OK_200); + localEndPoint.close(); + + localEndPoint = connector.executeRequest("" + + "POST / HTTP/1.1\r\n" + + "Host: localhost\r\n" + + "Transfer-Encoding: chunked\r\n" + + "\r\n" + + Integer.toHexString(chunk1.length()) + "\r\n" + + chunk1 + "\r\n"); + + // Wait for the server to block on the read(). + Thread.sleep(500); + + // Send more content. + localEndPoint.addInput("" + + Integer.toHexString(chunk2.length()) + "\r\n" + + chunk2 + "\r\n" + + "0\r\n" + + "\r\n"); + + response = HttpTester.parseResponse(localEndPoint.getResponse()); + assertEquals(response.getStatus(), HttpStatus.OK_200); + localEndPoint.close(); + } + private int checkContains(String s, int offset, String c) { assertThat(s.substring(offset), Matchers.containsString(c)); diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java index c71b048cef3d..5261ba53b48a 100644 --- a/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java @@ -251,7 +251,8 @@ public long getLength() assertThat("Request Input Content Received", inputContentReceived.get(), is(0L)); assertThat("Request Input Content Received less then initial buffer", inputContentReceived.get(), lessThanOrEqualTo((long)sizeActuallySent)); assertThat("Request Connection BytesIn should have some minimal data", inputBytesIn.get(), greaterThanOrEqualTo(1024L)); - assertThat("Request Connection BytesIn read should not have read all of the data", inputBytesIn.get(), lessThanOrEqualTo((long)sizeActuallySent)); + long requestBytesSent = sizeActuallySent + 512; // Take into account headers and chunked metadata. + assertThat("Request Connection BytesIn read should not have read all of the data", inputBytesIn.get(), lessThanOrEqualTo(requestBytesSent)); // Now provide rest content.offer(ByteBuffer.wrap(compressedRequest, sizeActuallySent, compressedRequest.length - sizeActuallySent)); @@ -351,7 +352,8 @@ public void onComplete(Request request) assertThat("Request Input Content Received", inputContentReceived.get(), read ? greaterThan(0L) : is(0L)); assertThat("Request Input Content Received less then initial buffer", inputContentReceived.get(), lessThanOrEqualTo((long)sizeActuallySent)); assertThat("Request Connection BytesIn should have some minimal data", inputBytesIn.get(), greaterThanOrEqualTo(1024L)); - assertThat("Request Connection BytesIn read should not have read all of the data", inputBytesIn.get(), lessThanOrEqualTo((long)sizeActuallySent)); + long requestBytesSent = sizeActuallySent + 512; // Take into account headers and chunked metadata. + assertThat("Request Connection BytesIn read should not have read all of the data", inputBytesIn.get(), lessThanOrEqualTo(requestBytesSent)); // Now provide rest content.offer(ByteBuffer.wrap(compressedRequest, sizeActuallySent, compressedRequest.length - sizeActuallySent));