Eclipse
/
jetty.project
mirror of https://github.com/jetty/jetty.project.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fixes #12249 - HTTP/2 responses with Content-Length may have no content. (#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>
This commit is contained in:
Simone Bordet 2024-09-09 18:20:17 +03:00 committed by GitHub
parent 5bff971fb0
commit fa0502db54
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 89 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -68,6 +68,7 @@ public class HttpReceiverOverHTTP2 extends HttpReceiver implements HTTP2Channel.
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 class HttpReceiverOverHTTP2 extends HttpReceiver implements HTTP2Channel.
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

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

@ -46,6 +46,7 @@ import org.eclipse.jetty.client.Origin;
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.greaterThanOrEqualTo;
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 class HttpClientTransportOverHTTP2Test extends AbstractTest
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

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

@ -269,11 +269,18 @@ public class HttpClientContinueTest extends AbstractTest
{
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 class HttpClientContinueTest extends AbstractTest
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()));

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

@ -196,11 +196,18 @@ public class HttpClientContinueTest extends AbstractTest
{
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();
}
});