Fixes #6105 - HttpConnection.getBytesIn() incorrect for requests with… (

#6108)

* 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 <simone.bordet@gmail.com>

jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java

@@ -258,9 +258,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.
@@ -343,8 +341,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())

jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java

@@ -37,6 +37,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;
@@ -45,12 +47,14 @@
 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.server.LocalConnector.LocalEndPoint;
 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.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 import org.eclipse.jetty.util.log.StacklessLogging;
@@ -65,9 +69,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
@@ -1361,6 +1367,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));

tests/test-integration/src/test/java/org/eclipse/jetty/test/GzipWithSendErrorTest.java

@@ -276,7 +276,8 @@ public long getLength()
         assertThat("Request Input Content Received should have seen content", inputContentReceived.get(), greaterThan(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 use the deferred content to complete writing of the request body content
         contentProvider.offer(ByteBuffer.wrap(compressedRequest, sizeActuallySent, compressedRequest.length - sizeActuallySent));
@@ -395,7 +396,8 @@ public void onComplete(Request request)
         assertThat("Request Input Content Received should have seen content", inputContentReceived.get(), greaterThan(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 use the deferred content to complete writing of the request body content
         contentProvider.offer(ByteBuffer.wrap(compressedRequest, sizeActuallySent, compressedRequest.length - sizeActuallySent));