From 8539e1cb37038d1b03bb0bfe45d2bf76876c047b Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Fri, 30 Jul 2021 10:30:59 +1000 Subject: [PATCH] Fix #6562 last written bytebuffer Fixes #6562 the last written bytebuffer calculation. Also fixed an associated issue with unnecessary flush of an empty when last calculation already signalled last. The code coverage is not complete, so more tests are needed for this use case. Also strange that `write(ByteBuffer)` does not appear to every commence aggregation? --- .../org/eclipse/jetty/server/HttpOutput.java | 16 ++-- .../eclipse/jetty/server/HttpOutputTest.java | 78 ++++++++++++++++++- 2 files changed, 85 insertions(+), 9 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java index 2a9135f2974c..619c877ee392 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java @@ -863,10 +863,12 @@ public void write(byte[] b, int off, int len) throws IOException // Blocking write try { + boolean complete = false; // flush any content from the aggregate if (BufferUtil.hasContent(_aggregate)) { - channelWrite(_aggregate, last && len == 0); + complete = last && len == 0; + channelWrite(_aggregate, complete); // should we fill aggregate again from the buffer? if (len > 0 && !last && len <= _commitSize && len <= maximizeAggregateSpace()) @@ -896,7 +898,7 @@ public void write(byte[] b, int off, int len) throws IOException } channelWrite(view, last); } - else if (last) + else if (last && !complete) { channelWrite(BufferUtil.EMPTY_BUFFER, true); } @@ -923,7 +925,7 @@ public void write(ByteBuffer buffer) throws IOException { checkWritable(); long written = _written + len; - last = _channel.getResponse().isAllContentWritten(_written); + last = _channel.getResponse().isAllContentWritten(written); flush = last || len > 0 || BufferUtil.hasContent(_aggregate); if (last && _state == State.OPEN) @@ -967,13 +969,17 @@ public void write(ByteBuffer buffer) throws IOException { // Blocking write // flush any content from the aggregate + boolean complete = false; if (BufferUtil.hasContent(_aggregate)) - channelWrite(_aggregate, last && len == 0); + { + complete = last && len == 0; + channelWrite(_aggregate, complete); + } // write any remaining content in the buffer directly if (len > 0) channelWrite(buffer, last); - else if (last) + else if (last && !complete) channelWrite(BufferUtil.EMPTY_BUFFER, true); onWriteComplete(last, null); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpOutputTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpOutputTest.java index 4d1672625f25..5f5ae9f83162 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpOutputTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpOutputTest.java @@ -50,6 +50,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.endsWith; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -360,6 +361,7 @@ public void testWriteByteKnown() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, containsString("Content-Length")); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(true)); } @Test @@ -374,6 +376,7 @@ public void testWriteSmallKnown() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, containsString("Content-Length")); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(true)); } @Test @@ -388,6 +391,7 @@ public void testWriteMedKnown() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, containsString("Content-Length")); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(true)); } @Test @@ -402,6 +406,7 @@ public void testWriteLargeKnown() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, containsString("Content-Length")); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(true)); } @Test @@ -419,6 +424,7 @@ public void testWriteHugeKnown() throws Exception String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n"); assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, containsString("Content-Length")); + assertThat(_handler._closedAfterWrite, is(true)); } @Test @@ -433,6 +439,7 @@ public void testWriteBufferSmall() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.not(containsString("Content-Length"))); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(false)); } @Test @@ -447,6 +454,7 @@ public void testWriteBufferMed() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.not(containsString("Content-Length"))); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(false)); } @Test @@ -461,6 +469,52 @@ public void testWriteBufferLarge() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.not(containsString("Content-Length"))); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(false)); + } + + @Test + public void testWriteBufferSmallKnown() throws Exception + { + final Resource big = Resource.newClassPathResource("simple/big.txt"); + _handler._writeLengthIfKnown = true; + _handler._content = BufferUtil.toBuffer(big, false); + _handler._byteBuffer = BufferUtil.allocate(8); + + String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n"); + assertThat(response, containsString("HTTP/1.1 200 OK")); + assertThat(response, containsString("Content-Length")); + assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(true)); + } + + @Test + public void testWriteBufferMedKnown() throws Exception + { + final Resource big = Resource.newClassPathResource("simple/big.txt"); + _handler._writeLengthIfKnown = true; + _handler._content = BufferUtil.toBuffer(big, false); + _handler._byteBuffer = BufferUtil.allocate(4000); + + String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n"); + assertThat(response, containsString("HTTP/1.1 200 OK")); + assertThat(response, containsString("Content-Length")); + assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(true)); + } + + @Test + public void testWriteBufferLargeKnown() throws Exception + { + final Resource big = Resource.newClassPathResource("simple/big.txt"); + _handler._writeLengthIfKnown = true; + _handler._content = BufferUtil.toBuffer(big, false); + _handler._byteBuffer = BufferUtil.allocate(8192); + + String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n"); + assertThat(response, containsString("HTTP/1.1 200 OK")); + assertThat(response, containsString("Content-Length")); + assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(true)); } @Test @@ -476,6 +530,7 @@ public void testAsyncWriteByte() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.not(containsString("Content-Length"))); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(false)); } @Test @@ -491,6 +546,7 @@ public void testAsyncWriteSmall() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.not(containsString("Content-Length"))); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(false)); } @Test @@ -506,6 +562,7 @@ public void testAsyncWriteMed() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.not(containsString("Content-Length"))); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(false)); } @Test @@ -521,12 +578,13 @@ public void testAsyncWriteLarge() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.not(containsString("Content-Length"))); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(false)); } @Test public void testAsyncWriteHuge() throws Exception { - _handler._writeLengthIfKnown = true; + _handler._writeLengthIfKnown = false; _handler._content = BufferUtil.allocate(4 * 1024 * 1024); _handler._content.limit(_handler._content.capacity()); for (int i = _handler._content.capacity(); i-- > 0; ) @@ -538,7 +596,8 @@ public void testAsyncWriteHuge() throws Exception String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n"); assertThat(response, containsString("HTTP/1.1 200 OK")); - assertThat(response, containsString("Content-Length")); + assertThat(response, Matchers.not(containsString("Content-Length"))); + assertThat(_handler._closedAfterWrite, is(false)); } @Test @@ -554,6 +613,7 @@ public void testAsyncWriteBufferSmall() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.not(containsString("Content-Length"))); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(false)); } @Test @@ -569,6 +629,7 @@ public void testAsyncWriteBufferMed() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.not(containsString("Content-Length"))); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(false)); } @Test @@ -584,6 +645,7 @@ public void testAsyncWriteBufferLarge() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.not(containsString("Content-Length"))); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(false)); } @Test @@ -600,6 +662,7 @@ public void testAsyncWriteBufferLargeDirect() assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.not(containsString("Content-Length"))); assertThat(response, endsWith(toUTF8String(big))); + assertThat(_handler._closedAfterWrite, is(false)); } @Test @@ -634,6 +697,7 @@ public void testAsyncWriteSimpleKnown() throws Exception assertThat(response, containsString("HTTP/1.1 200 OK")); assertThat(response, containsString("Content-Length: 11")); assertThat(response, containsString("simple text")); + assertThat(_handler._closedAfterWrite, is(true)); } @Test @@ -1048,6 +1112,7 @@ static class ContentHandler extends AbstractHandler ReadableByteChannel _contentChannel; ByteBuffer _content; ChainedInterceptor _interceptor; + boolean _closedAfterWrite; @Override public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException @@ -1068,6 +1133,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques { out.sendContent(_contentInputStream); _contentInputStream = null; + _closedAfterWrite = out.isClosed(); return; } @@ -1075,6 +1141,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques { out.sendContent(_contentChannel); _contentChannel = null; + _closedAfterWrite = out.isClosed(); return; } @@ -1110,6 +1177,7 @@ public void onWritePossible() throws IOException out.write(_arrayBuffer[0]); else out.write(_arrayBuffer, 0, len); + _closedAfterWrite = out.isClosed(); } // assertFalse(out.isReady()); } @@ -1136,7 +1204,7 @@ public void onError(Throwable t) else out.write(_arrayBuffer, 0, len); } - + _closedAfterWrite = out.isClosed(); return; } @@ -1168,6 +1236,7 @@ public void onWritePossible() throws IOException BufferUtil.put(_content, _byteBuffer); BufferUtil.flipToFlush(_byteBuffer, 0); out.write(_byteBuffer); + _closedAfterWrite = out.isClosed(); isFirstWrite = false; } } @@ -1190,7 +1259,7 @@ public void onError(Throwable t) BufferUtil.flipToFlush(_byteBuffer, 0); out.write(_byteBuffer); } - + _closedAfterWrite = out.isClosed(); return; } @@ -1201,6 +1270,7 @@ public void onError(Throwable t) else out.sendContent(_content); _content = null; + _closedAfterWrite = out.isClosed(); return; } }