Skip to content

Commit

Permalink
Fix #6562 last written bytebuffer
Browse files Browse the repository at this point in the history 
Removed the last flush of an empty buffer as was no path to that code.
  • Loading branch information
@gregw
gregw committed Jul 30, 2021
1 parent 8539e1c commit 6d2c57c
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 45 deletions.
16 changes: 2 additions & 14 deletions jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java
View file
Expand Up @@ -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())
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down
63 changes: 32 additions & 31 deletions jetty-server/src/test/java/org/eclipse/jetty/server/HttpOutputTest.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -1112,7 +1114,7 @@ static class ContentHandler extends AbstractHandler
ReadableByteChannel _contentChannel;
ByteBuffer _content;
ChainedInterceptor _interceptor;
boolean _closedAfterWrite;
final FuturePromise<Boolean> _closedAfterWrite = new FuturePromise<>();

@Override
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
Expand All @@ -1133,15 +1135,15 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
{
out.sendContent(_contentInputStream);
_contentInputStream = null;
_closedAfterWrite = out.isClosed();
_closedAfterWrite.succeeded(out.isClosed());
return;
}

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

Expand All @@ -1168,6 +1170,7 @@ public void onWritePossible() throws IOException
len = _arrayBuffer.length;
if (len == 0)
{
_closedAfterWrite.succeeded(out.isClosed());
async.complete();
break;
}
Expand All @@ -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
Expand All @@ -1204,7 +1205,7 @@ public void onError(Throwable t)
else
out.write(_arrayBuffer, 0, len);
}
_closedAfterWrite = out.isClosed();
_closedAfterWrite.succeeded(out.isClosed());
return;
}

Expand All @@ -1228,6 +1229,7 @@ public void onWritePossible() throws IOException
assertTrue(out.isReady());
if (BufferUtil.isEmpty(_content))
{
_closedAfterWrite.succeeded(out.isClosed());
async.complete();
break;
}
Expand All @@ -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;
}
}
Expand All @@ -1259,7 +1260,7 @@ public void onError(Throwable t)
BufferUtil.flipToFlush(_byteBuffer, 0);
out.write(_byteBuffer);
}
_closedAfterWrite = out.isClosed();
_closedAfterWrite.succeeded(out.isClosed());
return;
}

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

0 comments on commit 6d2c57c

Please sign in to comment.