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.

Sign up for GitHub

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

Fix server HTTP buffer leak in consumeAvailable() #10428

Merged
merged 4 commits into from
Sep 11, 2023
Merged

Fix server HTTP buffer leak in consumeAvailable() #10428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eeace14
#10226 fix HTTP consumeAvailable() buffer leak and re-enable tests
lorban Aug 29, 2023
87430f9
#10226 only release the buffer when it could not be propagated to a C…
lorban Aug 29, 2023
58a5499
#10226 fix buffer leak in consumeAvailable() when HttpStream.consumeA…
lorban Aug 30, 2023
a643194
#10226 handle review comments
lorban Sep 8, 2023
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
11 changes: 11 additions & 0 deletions jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1145,7 +1145,18 @@ public Throwable consumeAvailable()
{
Throwable result = HttpStream.consumeAvailable(this, getHttpConfiguration());
if (result != null)
{
_generator.setPersistent(false);
// If HttpStream.consumeAvailable() returns an error, there may be unconsumed content left,
// so we must make sure the buffer is released and that the next chunk indicates the end of the stream.
if (_retainableByteBuffer != null)
{
_retainableByteBuffer.release();
_retainableByteBuffer = null;
}
if (_chunk == null)
_chunk = Content.Chunk.from(result, true);
}
return result;
}

Expand Down
26 changes: 22 additions & 4 deletions jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,25 @@
import java.nio.ByteBuffer;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.concurrent.TimeUnit;

import org.awaitility.Awaitility;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.io.ArrayByteBufferPool;
import org.eclipse.jetty.io.Content;
import org.eclipse.jetty.util.Blocker;
import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.util.Callback;
import org.eclipse.jetty.util.Fields;
import org.eclipse.jetty.util.Promise;
import org.eclipse.jetty.util.thread.QueuedThreadPool;
import org.eclipse.jetty.util.thread.ScheduledExecutorScheduler;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;

import static org.junit.jupiter.api.Assertions.fail;

// @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck
public class HttpServerTestFixture
{
Expand All @@ -38,6 +45,7 @@ public class HttpServerTestFixture

protected QueuedThreadPool _threadPool;
protected Server _server;
protected ArrayByteBufferPool.Tracking _bufferPool;
protected URI _serverURI;
protected HttpConfiguration _httpConfiguration;
protected ServerConnector _connector;
Expand All @@ -55,7 +63,8 @@ protected Socket newSocket(String host, int port) throws Exception
public void before()
{
_threadPool = new QueuedThreadPool();
_server = new Server(_threadPool);
_bufferPool = new ArrayByteBufferPool.Tracking();
_server = new Server(_threadPool, new ScheduledExecutorScheduler(), _bufferPool);
}

protected void initServer(ServerConnector connector) throws Exception
Expand All @@ -70,9 +79,18 @@ protected void initServer(ServerConnector connector) throws Exception
@AfterEach
public void stopServer() throws Exception
{
_server.stop();
_server.join();
_server.setConnectors(new Connector[]{});
try
{
Awaitility.await().atMost(5, TimeUnit.SECONDS).until(() -> _bufferPool.getLeaks().size(), Matchers.is(0));
}
catch (Exception e)
{
fail(e.getMessage() + "\n---\nServer Leaks: " + _bufferPool.dumpLeaks() + "---\n");
}
finally
{
_server.stop();
}
}

protected void startServer(Handler handler) throws Exception
Expand Down
4 changes: 0 additions & 4 deletions ...-transports/src/test/java/org/eclipse/jetty/test/client/transport/ServerTimeoutsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.Response;
import org.eclipse.jetty.util.Callback;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
Expand Down Expand Up @@ -95,9 +94,6 @@ public boolean handle(Request request, Response response, Callback callback)

@ParameterizedTest
@MethodSource("transportsAndTrueIdleTimeoutListeners")
@Tag("DisableLeakTracking:server:HTTP")
@Tag("DisableLeakTracking:server:HTTPS")
@Tag("DisableLeakTracking:server:UNIX_DOMAIN")
public void testIdleTimeoutWithDemand(Transport transport, boolean listener) throws Exception
{
AtomicBoolean listenerCalled = new AtomicBoolean();
Expand Down