From 8539e1cb37038d1b03bb0bfe45d2bf76876c047b Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Fri, 30 Jul 2021 10:30:59 +1000 Subject: [PATCH 1/3] 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; } } From 6d2c57c57521b7520cbea6f799cb8889a0255c1b Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Fri, 30 Jul 2021 15:56:59 +1000 Subject: [PATCH 2/3] Fix #6562 last written bytebuffer Removed the last flush of an empty buffer as was no path to that code. --- .../org/eclipse/jetty/server/HttpOutput.java | 16 +---- .../eclipse/jetty/server/HttpOutputTest.java | 63 ++++++++++--------- 2 files changed, 34 insertions(+), 45 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 619c877ee392..ba4b630f5e01 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,12 +863,10 @@ 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)) { - complete = last && len == 0; - channelWrite(_aggregate, complete); + channelWrite(_aggregate, last && len == 0); // should we fill aggregate again from the buffer? if (len > 0 && !last && len <= _commitSize && len <= maximizeAggregateSpace()) @@ -898,10 +896,6 @@ public void write(byte[] b, int off, int len) throws IOException } channelWrite(view, last); } - else if (last && !complete) - { - channelWrite(BufferUtil.EMPTY_BUFFER, true); - } onWriteComplete(last, null); } @@ -969,18 +963,12 @@ public void write(ByteBuffer buffer) throws IOException { // Blocking write // flush any content from the aggregate - boolean complete = false; if (BufferUtil.hasContent(_aggregate)) - { - complete = last && len == 0; - channelWrite(_aggregate, complete); - } + channelWrite(_aggregate, last && len == 0); // write any remaining content in the buffer directly if (len > 0) channelWrite(buffer, 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 5f5ae9f83162..5696e01caf5a 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 @@ -27,6 +27,7 @@ import java.nio.channels.ReadableByteChannel; import java.nio.charset.StandardCharsets; import java.util.Arrays; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import javax.servlet.AsyncContext; import javax.servlet.ServletException; @@ -41,6 +42,7 @@ import org.eclipse.jetty.server.handler.HotSwapHandler; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.FuturePromise; import org.eclipse.jetty.util.resource.Resource; import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; @@ -361,7 +363,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(true)); } @Test @@ -376,7 +378,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(true)); } @Test @@ -391,7 +393,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(true)); } @Test @@ -406,7 +408,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(true)); } @Test @@ -424,7 +426,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(true)); } @Test @@ -439,7 +441,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -454,7 +456,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -469,7 +471,7 @@ 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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -484,7 +486,7 @@ public void testWriteBufferSmallKnown() 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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(true)); } @Test @@ -499,7 +501,7 @@ public void testWriteBufferMedKnown() 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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(true)); } @Test @@ -514,7 +516,7 @@ public void testWriteBufferLargeKnown() 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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(true)); } @Test @@ -530,7 +532,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -546,7 +548,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -562,7 +564,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -578,7 +580,7 @@ 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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -597,7 +599,7 @@ 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, Matchers.not(containsString("Content-Length"))); - assertThat(_handler._closedAfterWrite, is(false)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -613,7 +615,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -629,7 +631,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -645,7 +647,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -662,7 +664,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(false)); } @Test @@ -697,7 +699,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)); + assertThat(_handler._closedAfterWrite.get(10, TimeUnit.SECONDS), is(true)); } @Test @@ -932,7 +934,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques aggregated += data.length; } - // write data that will not be aggregated + // write data that will not be aggregated because it is too large data = new byte[bufferSize + 1]; Arrays.fill(data, (byte)(fill++)); expected.write(data); @@ -1112,7 +1114,7 @@ static class ContentHandler extends AbstractHandler ReadableByteChannel _contentChannel; ByteBuffer _content; ChainedInterceptor _interceptor; - boolean _closedAfterWrite; + final FuturePromise _closedAfterWrite = new FuturePromise<>(); @Override public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException @@ -1133,7 +1135,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques { out.sendContent(_contentInputStream); _contentInputStream = null; - _closedAfterWrite = out.isClosed(); + _closedAfterWrite.succeeded(out.isClosed()); return; } @@ -1141,7 +1143,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques { out.sendContent(_contentChannel); _contentChannel = null; - _closedAfterWrite = out.isClosed(); + _closedAfterWrite.succeeded(out.isClosed()); return; } @@ -1168,6 +1170,7 @@ public void onWritePossible() throws IOException len = _arrayBuffer.length; if (len == 0) { + _closedAfterWrite.succeeded(out.isClosed()); async.complete(); break; } @@ -1177,9 +1180,7 @@ public void onWritePossible() throws IOException out.write(_arrayBuffer[0]); else out.write(_arrayBuffer, 0, len); - _closedAfterWrite = out.isClosed(); } - // assertFalse(out.isReady()); } @Override @@ -1204,7 +1205,7 @@ public void onError(Throwable t) else out.write(_arrayBuffer, 0, len); } - _closedAfterWrite = out.isClosed(); + _closedAfterWrite.succeeded(out.isClosed()); return; } @@ -1228,6 +1229,7 @@ public void onWritePossible() throws IOException assertTrue(out.isReady()); if (BufferUtil.isEmpty(_content)) { + _closedAfterWrite.succeeded(out.isClosed()); async.complete(); break; } @@ -1236,7 +1238,6 @@ public void onWritePossible() throws IOException BufferUtil.put(_content, _byteBuffer); BufferUtil.flipToFlush(_byteBuffer, 0); out.write(_byteBuffer); - _closedAfterWrite = out.isClosed(); isFirstWrite = false; } } @@ -1259,7 +1260,7 @@ public void onError(Throwable t) BufferUtil.flipToFlush(_byteBuffer, 0); out.write(_byteBuffer); } - _closedAfterWrite = out.isClosed(); + _closedAfterWrite.succeeded(out.isClosed()); return; } @@ -1270,7 +1271,7 @@ public void onError(Throwable t) else out.sendContent(_content); _content = null; - _closedAfterWrite = out.isClosed(); + _closedAfterWrite.succeeded(out.isClosed()); return; } } From 0f6d3e25c88ea4ed8288612c9790fa5f1061011a Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Sat, 31 Jul 2021 09:01:23 +1000 Subject: [PATCH 3/3] Fix #6562 last written bytebuffer Restored the last flush of an empty buffer as is needed for 0 length empty write. --- .../org/eclipse/jetty/server/HttpOutput.java | 16 ++- .../eclipse/jetty/server/HttpOutputTest.java | 97 +++++++++++++++++++ 2 files changed, 111 insertions(+), 2 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 ba4b630f5e01..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,6 +898,10 @@ public void write(byte[] b, int off, int len) throws IOException } channelWrite(view, last); } + else if (last && !complete) + { + channelWrite(BufferUtil.EMPTY_BUFFER, true); + } onWriteComplete(last, null); } @@ -963,12 +969,18 @@ 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 && !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 5696e01caf5a..36d8d34b7e01 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 @@ -28,6 +28,7 @@ import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import javax.servlet.AsyncContext; import javax.servlet.ServletException; @@ -763,6 +764,102 @@ public void setNext(Interceptor interceptor) assertThat(response, containsString("400\tTHIS IS A BIGGER FILE")); } + @Test + public void testEmptyArray() throws Exception + { + AtomicBoolean committed = new AtomicBoolean(); + AbstractHandler handler = new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + response.setStatus(200); + response.getOutputStream().write(new byte[0]); + committed.set(response.isCommitted()); + } + }; + + _swap.setHandler(handler); + handler.start(); + String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n"); + assertThat(response, containsString("HTTP/1.1 200 OK")); + assertThat(committed.get(), is(false)); + } + + @Test + public void testEmptyArrayKnown() throws Exception + { + AtomicBoolean committed = new AtomicBoolean(); + AbstractHandler handler = new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + response.setStatus(200); + response.setContentLength(0); + response.getOutputStream().write(new byte[0]); + committed.set(response.isCommitted()); + } + }; + + _swap.setHandler(handler); + handler.start(); + 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: 0")); + assertThat(committed.get(), is(true)); + } + + @Test + public void testEmptyBuffer() throws Exception + { + AtomicBoolean committed = new AtomicBoolean(); + AbstractHandler handler = new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + response.setStatus(200); + ((HttpOutput)response.getOutputStream()).write(ByteBuffer.wrap(new byte[0])); + committed.set(response.isCommitted()); + } + }; + + _swap.setHandler(handler); + handler.start(); + String response = _connector.getResponse("GET / HTTP/1.0\nHost: localhost:80\n\n"); + assertThat(response, containsString("HTTP/1.1 200 OK")); + assertThat(committed.get(), is(false)); + } + + @Test + public void testEmptyBufferKnown() throws Exception + { + AtomicBoolean committed = new AtomicBoolean(); + AbstractHandler handler = new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + response.setStatus(200); + response.setContentLength(0); + ((HttpOutput)response.getOutputStream()).write(ByteBuffer.wrap(new byte[0])); + committed.set(response.isCommitted()); + } + }; + + _swap.setHandler(handler); + handler.start(); + 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: 0")); + assertThat(committed.get(), is(true)); + } + @Test public void testAggregation() throws Exception {