Skip to content

Commit

Permalink
Fix #6562 last written bytebuffer
Browse files Browse the repository at this point in the history 
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?
  • Loading branch information
@gregw
gregw committed Jul 30, 2021
1 parent 735e97d commit 8539e1c
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 9 deletions.
16 changes: 11 additions & 5 deletions jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java
View file
Expand Up @@ -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())
Expand Down Expand Up @@ -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);
}
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand Down
78 changes: 74 additions & 4 deletions jetty-server/src/test/java/org/eclipse/jetty/server/HttpOutputTest.java
View file
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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; )
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -1068,13 +1133,15 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
{
out.sendContent(_contentInputStream);
_contentInputStream = null;
_closedAfterWrite = out.isClosed();
return;
}

if (_contentChannel != null)
{
out.sendContent(_contentChannel);
_contentChannel = null;
_closedAfterWrite = out.isClosed();
return;
}

Expand Down Expand Up @@ -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());
}
Expand All @@ -1136,7 +1204,7 @@ public void onError(Throwable t)
else
out.write(_arrayBuffer, 0, len);
}

_closedAfterWrite = out.isClosed();
return;
}

Expand Down Expand Up @@ -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;
}
}
Expand All @@ -1190,7 +1259,7 @@ public void onError(Throwable t)
BufferUtil.flipToFlush(_byteBuffer, 0);
out.write(_byteBuffer);
}

_closedAfterWrite = out.isClosed();
return;
}

Expand All @@ -1201,6 +1270,7 @@ public void onError(Throwable t)
else
out.sendContent(_content);
_content = null;
_closedAfterWrite = out.isClosed();
return;
}
}
Expand Down

0 comments on commit 8539e1c

Please sign in to comment.