Fixes #12249 - HTTP/2 responses with Content-Length may have no conte…

…nt. (#12250)

Fixed HttpReceiverOverHTTP2.read() to return a failed chunk if the stream has been reset.
This closes the window where a RST_STREAM frame may be received, drain the DATA frames in HTTP2Stream, and the application reading exactly at that point.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java

@@ -68,6 +68,7 @@ public Content.Chunk read(boolean fillInterestIfNeeded)
         Stream stream = getHttpChannel().getStream();
         if (stream == null)
             return Content.Chunk.from(new EOFException("Channel has been released"));
+
         Stream.Data data = stream.readData();
         if (LOG.isDebugEnabled())
             LOG.debug("Read stream data {} in {}", data, this);
@@ -77,13 +78,25 @@ public Content.Chunk read(boolean fillInterestIfNeeded)
                 stream.demand();
             return null;
         }
+
         DataFrame frame = data.frame();
         boolean last = frame.remaining() == 0 && frame.isEndStream();
         if (!last)
-            return Content.Chunk.asChunk(frame.getByteBuffer(), last, data);
+            return Content.Chunk.asChunk(frame.getByteBuffer(), false, data);
+
         data.release();
-        responseSuccess(getHttpExchange(), null);
-        return Content.Chunk.EOF;
+
+        if (stream.isReset())
+        {
+            Throwable failure = new EOFException("Stream has been reset");
+            responseFailure(failure, Promise.noop());
+            return Content.Chunk.from(failure);
+        }
+        else
+        {
+            responseSuccess(getHttpExchange(), null);
+            return Content.Chunk.EOF;
+        }
     }
 
     @Override

jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HttpClientTransportOverHTTP2Test.java

@@ -46,6 +46,7 @@
 import org.eclipse.jetty.client.Response;
 import org.eclipse.jetty.client.Result;
 import org.eclipse.jetty.http.HttpFields;
+import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpMethod;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpURI;
@@ -91,6 +92,7 @@
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -826,6 +828,54 @@ public boolean handle(Request request, org.eclipse.jetty.server.Response respons
         assertEquals(HttpStatus.OK_200, response.getStatus());
     }
 
+    @Test
+    public void testUnreadRequestContentDrainsResponseContent() throws Exception
+    {
+        start(new Handler.Abstract()
+        {
+            @Override
+            public boolean handle(Request request, org.eclipse.jetty.server.Response response, Callback callback)
+            {
+                // Do not read the request content,
+                // the server will reset the stream,
+                // then send a response with content.
+                ByteBuffer content = ByteBuffer.allocate(1024);
+                response.getHeaders().put(HttpHeader.CONTENT_LENGTH, content.remaining());
+                response.write(true, content, callback);
+                return true;
+            }
+        });
+
+        AtomicReference<Content.Source> contentSourceRef = new AtomicReference<>();
+        AtomicReference<Content.Chunk> chunkRef = new AtomicReference<>();
+        CountDownLatch responseFailureLatch = new CountDownLatch(1);
+        AtomicReference<Result> resultRef = new AtomicReference<>();
+        httpClient.newRequest("localhost", connector.getLocalPort())
+            .method(HttpMethod.POST)
+            .body(new AsyncRequestContent(ByteBuffer.allocate(1024)))
+            .onResponseContentSource((response, contentSource) -> contentSourceRef.set(contentSource))
+            // The request is failed before the response, verify that
+            // reading at the request failure event yields a failure chunk.
+            .onRequestFailure((request, failure) -> chunkRef.set(contentSourceRef.get().read()))
+            .onResponseFailure((response, failure) -> responseFailureLatch.countDown())
+            .send(resultRef::set);
+
+        // Wait for the RST_STREAM to arrive and drain the response content.
+        assertTrue(responseFailureLatch.await(5, TimeUnit.SECONDS));
+
+        // Verify that the chunk read at the request failure event is a failure chunk.
+        Content.Chunk chunk = chunkRef.get();
+        assertTrue(Content.Chunk.isFailure(chunk, true));
+        // Reading more also yields a failure chunk.
+        chunk = contentSourceRef.get().read();
+        assertTrue(Content.Chunk.isFailure(chunk, true));
+
+        Result result = await().atMost(5, TimeUnit.SECONDS).until(resultRef::get, notNullValue());
+        assertEquals(HttpStatus.OK_200, result.getResponse().getStatus());
+        assertNotNull(result.getRequestFailure());
+        assertNotNull(result.getResponseFailure());
+    }
+
     @Test
     @Tag("external")
     public void testExternalServer() throws Exception

jetty-ee10/jetty-ee10-tests/jetty-ee10-test-client-transports/src/test/java/org/eclipse/jetty/ee10/test/client/transport/HttpClientContinueTest.java

@@ -269,11 +269,18 @@ public void onComplete(Result result)
                 {
                     assertTrue(result.isFailed());
                     assertNotNull(result.getRequestFailure());
-                    assertNull(result.getResponseFailure());
-                    byte[] content = getContent();
-                    assertNotNull(content);
-                    assertTrue(content.length > 0);
                     assertEquals(error, result.getResponse().getStatus());
+                    Throwable responseFailure = result.getResponseFailure();
+                    // For HTTP/2 the response may fail because the
+                    // server may not fully read the request content,
+                    // and sends a reset that may drop the response
+                    // content and cause the response failure.
+                    if (responseFailure == null)
+                    {
+                        byte[] content = getContent();
+                        assertNotNull(content);
+                        assertTrue(content.length > 0);
+                    }
                     latch.countDown();
                 }
             });
@@ -828,7 +835,7 @@ public void testExpect100ContinueWithContentLengthZero() throws Exception
         startServer(Transport.HTTP, new HttpServlet()
         {
             @Override
-            protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+            protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException
             {
                 assertEquals(0, request.getContentLengthLong());
                 assertNotNull(request.getHeader(HttpHeader.EXPECT.asString()));

jetty-ee9/jetty-ee9-tests/jetty-ee9-test-client-transports/src/test/java/org/eclipse/jetty/ee9/test/client/transport/HttpClientContinueTest.java

@@ -196,11 +196,18 @@ public void onComplete(Result result)
                 {
                     assertTrue(result.isFailed());
                     assertNotNull(result.getRequestFailure());
-                    assertNull(result.getResponseFailure());
-                    byte[] content = getContent();
-                    assertNotNull(content);
-                    assertTrue(content.length > 0);
                     assertEquals(error, result.getResponse().getStatus());
+                    Throwable responseFailure = result.getResponseFailure();
+                    // For HTTP/2 the response may fail because the
+                    // server may not fully read the request content,
+                    // and sends a reset that may drop the response
+                    // content and cause the response failure.
+                    if (responseFailure == null)
+                    {
+                        byte[] content = getContent();
+                        assertNotNull(content);
+                        assertTrue(content.length > 0);
+                    }
                     latch.countDown();
                 }
             });