Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #12249 - HTTP/2 responses with Content-Length may have no content. #12250

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
});
Expand Down Expand Up @@ -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()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
});
Expand Down
Loading