From 5439f17ff6dc161bb92aae6071580b569b8f4383 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 2 Sep 2024 19:34:05 +0200 Subject: [PATCH 1/2] Rework leak tracking assertions to use a common awaitility pattern (#12226) Rework leak tracking assertions to use a common awaitility pattern Signed-off-by: Ludovic Orban --- .../server/AbstractHttpClientServerTest.java | 22 +++++-------------- .../jetty/server/AbstractHttpTest.java | 4 +++- .../jetty/server/HttpServerTestFixture.java | 12 ++++------ .../jetty/server/ThreadStarvationTest.java | 4 ++-- .../server/handler/DebugHandlerTest.java | 7 +++--- .../ssl/ServerConnectorSslServerTest.java | 4 +++- .../test/client/transport/AbstractTest.java | 7 +++--- .../ee10/test/HttpInputIntegrationTest.java | 4 ++-- .../test/HttpInputTransientErrorTest.java | 3 ++- .../ee9/test/HttpInputIntegrationTest.java | 4 ++-- .../ee9/test/HttpInputTransientErrorTest.java | 3 ++- 11 files changed, 33 insertions(+), 41 deletions(-) diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/test/java/org/eclipse/jetty/fcgi/server/AbstractHttpClientServerTest.java b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/test/java/org/eclipse/jetty/fcgi/server/AbstractHttpClientServerTest.java index b9869c2b6c9..b0001266fc2 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/test/java/org/eclipse/jetty/fcgi/server/AbstractHttpClientServerTest.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/test/java/org/eclipse/jetty/fcgi/server/AbstractHttpClientServerTest.java @@ -15,7 +15,6 @@ package org.eclipse.jetty.fcgi.server; import java.util.concurrent.TimeUnit; -import org.awaitility.Awaitility; import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.HttpClientTransport; import org.eclipse.jetty.fcgi.client.transport.HttpClientTransportOverFCGI; @@ -29,10 +28,11 @@ import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.util.ProcessorUtils; import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.util.thread.QueuedThreadPool; -import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; -import static org.junit.jupiter.api.Assertions.fail; +import static org.awaitility.Awaitility.await; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; public abstract class AbstractHttpClientServerTest { @@ -75,9 +75,9 @@ public abstract class AbstractHttpClientServerTest try { if (serverBufferPool != null) - assertNoLeaks(serverBufferPool, "\n---\nServer Leaks: " + serverBufferPool.dumpLeaks() + "---\n"); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Server Leaks: " + serverBufferPool.dumpLeaks(), serverBufferPool.getLeaks().size(), is(0))); if (clientBufferPool != null) - assertNoLeaks(clientBufferPool, "\n---\nClient Leaks: " + clientBufferPool.dumpLeaks() + "---\n"); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Client Leaks: " + clientBufferPool.dumpLeaks(), clientBufferPool.getLeaks().size(), is(0))); } finally { @@ -85,16 +85,4 @@ public abstract class AbstractHttpClientServerTest LifeCycle.stop(server); } } - - private void assertNoLeaks(ArrayByteBufferPool.Tracking bufferPool, String msg) - { - try - { - Awaitility.await().atMost(3, TimeUnit.SECONDS).until(() -> bufferPool.getLeaks().size(), Matchers.is(0)); - } - catch (Exception e) - { - fail(e.getMessage() + msg); - } - } } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/AbstractHttpTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/AbstractHttpTest.java index fb0f077c18b..0f002eb7d92 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/AbstractHttpTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/AbstractHttpTest.java @@ -21,6 +21,7 @@ import java.net.URISyntaxException; import java.util.Arrays; import java.util.HashSet; import java.util.Set; +import java.util.concurrent.TimeUnit; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.HttpVersion; @@ -33,6 +34,7 @@ import org.eclipse.jetty.util.component.LifeCycle; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import static org.awaitility.Awaitility.await; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -63,7 +65,7 @@ public abstract class AbstractHttpTest { try { - assertThat("Server leaks: " + bufferPool.dumpLeaks(), bufferPool.getLeaks().size(), is(0)); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Server leaks: " + bufferPool.dumpLeaks(), bufferPool.getLeaks().size(), is(0))); } finally { diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java index 8c263eb3567..082bde1a00f 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java @@ -20,7 +20,6 @@ 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; @@ -31,11 +30,12 @@ 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; +import static org.awaitility.Awaitility.await; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; // @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck public class HttpServerTestFixture @@ -81,11 +81,7 @@ public class HttpServerTestFixture { 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"); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Server leaks: " + _bufferPool.dumpLeaks(), _bufferPool.getLeaks().size(), is(0))); } finally { diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java index 109fd1c74bd..db42bfea6a4 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java @@ -38,13 +38,13 @@ import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.eclipse.jetty.util.thread.QueuedThreadPool; -import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import static org.awaitility.Awaitility.await; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; @@ -153,7 +153,7 @@ public class ThreadStarvationTest ArrayByteBufferPool.Tracking byteBufferPool = (ArrayByteBufferPool.Tracking)_server.getConnectors()[0].getByteBufferPool(); try { - assertThat("Server Leaks: " + byteBufferPool.dumpLeaks(), byteBufferPool.getLeaks().size(), Matchers.is(0)); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Server leaks: " + byteBufferPool.dumpLeaks(), byteBufferPool.getLeaks().size(), is(0))); } finally { diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DebugHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DebugHandlerTest.java index afb4663a9b1..cacb7ac44f3 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DebugHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DebugHandlerTest.java @@ -21,6 +21,7 @@ import java.net.URI; import java.nio.charset.StandardCharsets; import java.nio.file.Path; import java.security.KeyStore; +import java.util.concurrent.TimeUnit; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.HttpsURLConnection; import javax.net.ssl.SSLContext; @@ -35,12 +36,12 @@ import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.util.ssl.SslContextFactory; -import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import static org.awaitility.Awaitility.await; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; @@ -135,8 +136,8 @@ public class DebugHandlerTest { try { - assertThat("Server HTTP Leaks: " + httpTrackingBufferPool.dumpLeaks(), httpTrackingBufferPool.getLeaks().size(), Matchers.is(0)); - assertThat("Server SSL Leaks: " + sslTrackingBufferPool.dumpLeaks(), sslTrackingBufferPool.getLeaks().size(), Matchers.is(0)); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Server HTTP Leaks: " + httpTrackingBufferPool.dumpLeaks(), httpTrackingBufferPool.getLeaks().size(), is(0))); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Server SSL Leaks: " + sslTrackingBufferPool.dumpLeaks(), sslTrackingBufferPool.getLeaks().size(), is(0))); } finally { diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/ServerConnectorSslServerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/ServerConnectorSslServerTest.java index 1114f93d786..290da4239af 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/ServerConnectorSslServerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/ServerConnectorSslServerTest.java @@ -20,6 +20,7 @@ import java.net.URI; import java.nio.charset.StandardCharsets; import java.security.KeyStore; import java.util.Arrays; +import java.util.concurrent.TimeUnit; import java.util.regex.Matcher; import java.util.regex.Pattern; import javax.net.ssl.HttpsURLConnection; @@ -50,6 +51,7 @@ import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import static org.awaitility.Awaitility.await; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; @@ -115,7 +117,7 @@ public class ServerConnectorSslServerTest extends HttpServerTestBase @AfterEach public void dispose() throws Exception { - assertThat("Server Leaks: " + _trackingBufferPool.dumpLeaks(), _trackingBufferPool.getLeaks().size(), Matchers.is(0)); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Server leaks: " + _trackingBufferPool.dumpLeaks(), _trackingBufferPool.getLeaks().size(), is(0))); } @Override diff --git a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/AbstractTest.java b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/AbstractTest.java index 8f4932f39fa..3567787976c 100644 --- a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/AbstractTest.java +++ b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/AbstractTest.java @@ -26,7 +26,6 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Stream; import javax.management.MBeanServer; -import org.awaitility.Awaitility; import org.eclipse.jetty.alpn.server.ALPNServerConnectionFactory; import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.HttpClientTransport; @@ -66,7 +65,6 @@ import org.eclipse.jetty.util.SocketAddressResolver; import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.eclipse.jetty.util.thread.QueuedThreadPool; -import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Tags; @@ -75,6 +73,9 @@ import org.junit.jupiter.api.extension.BeforeTestExecutionCallback; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.extension.RegisterExtension; +import static org.awaitility.Awaitility.await; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.fail; @ExtendWith(WorkDirExtension.class) @@ -149,7 +150,7 @@ public class AbstractTest { try { - Awaitility.await().atMost(3, TimeUnit.SECONDS).until(() -> bufferPool.getLeaks().size(), Matchers.is(0)); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Leaks: " + bufferPool.dumpLeaks(), bufferPool.getLeaks().size(), is(0))); } catch (Exception e) { diff --git a/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/HttpInputIntegrationTest.java b/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/HttpInputIntegrationTest.java index ffb025e2e2f..a26a99ffac7 100644 --- a/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/HttpInputIntegrationTest.java +++ b/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/HttpInputIntegrationTest.java @@ -56,7 +56,6 @@ import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.util.ssl.SslContextFactory; -import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Tag; @@ -66,6 +65,7 @@ import org.junit.jupiter.params.provider.MethodSource; import static org.awaitility.Awaitility.await; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -141,7 +141,7 @@ public class HttpInputIntegrationTest { try { - assertThat("Server leaks: " + __bufferPool.dumpLeaks(), __bufferPool.getLeaks().size(), Matchers.is(0)); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Server leaks: " + __bufferPool.dumpLeaks(), __bufferPool.getLeaks().size(), is(0))); } finally { diff --git a/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/HttpInputTransientErrorTest.java b/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/HttpInputTransientErrorTest.java index ca7658ce2d5..a767e2edea3 100644 --- a/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/HttpInputTransientErrorTest.java +++ b/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/HttpInputTransientErrorTest.java @@ -42,6 +42,7 @@ import org.eclipse.jetty.util.component.LifeCycle; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import static org.awaitility.Awaitility.await; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; @@ -64,7 +65,7 @@ public class HttpInputTransientErrorTest try { if (bufferPool != null) - assertThat("Server leaks: " + bufferPool.dumpLeaks(), bufferPool.getLeaks().size(), is(0)); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Server leaks: " + bufferPool.dumpLeaks(), bufferPool.getLeaks().size(), is(0))); } finally { diff --git a/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/HttpInputIntegrationTest.java b/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/HttpInputIntegrationTest.java index 759af89dd66..c16a1f611d7 100644 --- a/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/HttpInputIntegrationTest.java +++ b/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/HttpInputIntegrationTest.java @@ -55,7 +55,6 @@ import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.util.ssl.SslContextFactory; -import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Tag; @@ -65,6 +64,7 @@ import org.junit.jupiter.params.provider.MethodSource; import static org.awaitility.Awaitility.await; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -139,7 +139,7 @@ public class HttpInputIntegrationTest { try { - assertThat("Server leaks: " + __bufferPool.dumpLeaks(), __bufferPool.getLeaks().size(), Matchers.is(0)); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Server leaks: " + __bufferPool.dumpLeaks(), __bufferPool.getLeaks().size(), is(0))); } finally { diff --git a/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/HttpInputTransientErrorTest.java b/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/HttpInputTransientErrorTest.java index 950eb29e31f..2d040c460bf 100644 --- a/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/HttpInputTransientErrorTest.java +++ b/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/HttpInputTransientErrorTest.java @@ -38,6 +38,7 @@ import org.eclipse.jetty.util.component.LifeCycle; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import static org.awaitility.Awaitility.await; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.sameInstance; @@ -58,7 +59,7 @@ public class HttpInputTransientErrorTest try { if (bufferPool != null) - assertThat("Server leaks: " + bufferPool.dumpLeaks(), bufferPool.getLeaks().size(), is(0)); + await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> assertThat("Server leaks: " + bufferPool.dumpLeaks(), bufferPool.getLeaks().size(), is(0))); } finally { From 551710e9bbb1a917dea38c4bcfde3ab08455a1e6 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 3 Sep 2024 07:58:16 +1000 Subject: [PATCH 2/2] Improve shutdown of non-persistent HTTP/1 connections #12212 (#12216) * Improve shutdown of non-persistent HTTP/1 connections + shutdown in SendCallback Signed-off-by: Simone Bordet --------- Signed-off-by: Simone Bordet Co-authored-by: Simone Bordet --- .../jetty/server/internal/HttpConnection.java | 40 +++++----- .../jetty/server/HttpServerTestBase.java | 73 +++++++++++++++++-- 2 files changed, 90 insertions(+), 23 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index d3d59ca1802..4306c161a61 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -318,15 +318,16 @@ public class HttpConnection extends AbstractMetaDataConnection implements Runnab @Override public ByteBuffer onUpgradeFrom() { - if (!isRequestBufferEmpty()) + if (isRequestBufferEmpty()) { - ByteBuffer unconsumed = ByteBuffer.allocateDirect(_retainableByteBuffer.remaining()); - unconsumed.put(_retainableByteBuffer.getByteBuffer()); - unconsumed.flip(); releaseRequestBuffer(); - return unconsumed; + return null; } - return null; + ByteBuffer unconsumed = ByteBuffer.allocateDirect(_retainableByteBuffer.remaining()); + unconsumed.put(_retainableByteBuffer.getByteBuffer()); + unconsumed.flip(); + releaseRequestBuffer(); + return unconsumed; } @Override @@ -341,10 +342,10 @@ public class HttpConnection extends AbstractMetaDataConnection implements Runnab { if (LOG.isDebugEnabled()) LOG.debug("releaseRequestBuffer {}", this); - if (_retainableByteBuffer.release()) - _retainableByteBuffer = null; - else - throw new IllegalStateException("unreleased buffer " + _retainableByteBuffer); + RetainableByteBuffer buffer = _retainableByteBuffer; + _retainableByteBuffer = null; + if (!buffer.release()) + throw new IllegalStateException("unreleased buffer " + buffer); } } @@ -369,7 +370,9 @@ public class HttpConnection extends AbstractMetaDataConnection implements Runnab HttpConnection last = setCurrentConnection(this); try { - while (getEndPoint().isOpen()) + // We must loop until we fill -1 or there is an async pause in handling. + // Note that the endpoint might already be closed in some special circumstances. + while (true) { // Fill the request buffer (if needed). int filled = fillRequestBuffer(); @@ -906,6 +909,13 @@ public class HttpConnection extends AbstractMetaDataConnection implements Runnab @Override protected void onCompleteSuccess() { + // If we are a non-persistent connection and have succeeded the last write... + if (_shutdownOut && !(_httpChannel.getRequest().getAttribute(HttpStream.UPGRADE_CONNECTION_ATTRIBUTE) instanceof Connection)) + { + // then we shutdown the output here so that the client sees the body termination ASAP and + // cannot be delayed by any further server handling before the stream callback is completed. + getEndPoint().shutdownOutput(); + } release().succeeded(); } @@ -1513,8 +1523,7 @@ public class HttpConnection extends AbstractMetaDataConnection implements Runnab return; } - Connection upgradeConnection = (Connection)_httpChannel.getRequest().getAttribute(HttpStream.UPGRADE_CONNECTION_ATTRIBUTE); - if (upgradeConnection != null) + if (_httpChannel.getRequest().getAttribute(HttpStream.UPGRADE_CONNECTION_ATTRIBUTE) instanceof Connection upgradeConnection) { getEndPoint().upgrade(upgradeConnection); _httpChannel.recycle(); @@ -1523,13 +1532,8 @@ public class HttpConnection extends AbstractMetaDataConnection implements Runnab return; } - // As this is not an upgrade, we can shutdown the output if we know we are not persistent - if (_sendCallback._shutdownOut) - getEndPoint().shutdownOutput(); - _httpChannel.recycle(); - // If a 100 Continue is still expected to be sent, but no content was read, then // close the parser so that seeks EOF below, not the next request. if (_expects100Continue) diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java index dad6703e354..f86408c292a 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java @@ -52,11 +52,15 @@ import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.Blocker; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.FutureCallback; import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.NanoTime; import org.eclipse.jetty.util.StringUtil; import org.hamcrest.Matchers; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -68,6 +72,7 @@ import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.lessThan; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -1085,6 +1090,56 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture } } + @Test + public void testCloseWhileCompletePending() throws Exception + { + String content = "The End!\r\n"; + CountDownLatch handleComplete = new CountDownLatch(1); + startServer(new Handler.Abstract() + { + @Override + public boolean handle(Request request, Response response, Callback callback) throws Exception + { + FutureCallback writeComplete = new FutureCallback(); + Content.Sink.write(response, true, content, writeComplete); + // Wait until the write is complete + writeComplete.get(30, TimeUnit.SECONDS); + + // Wait until test lets the handling complete + assertTrue(handleComplete.await(30, TimeUnit.SECONDS)); + + callback.succeeded(); + return true; + } + }); + + try (Socket client = newSocket(_serverURI.getHost(), _serverURI.getPort())) + { + OutputStream output = client.getOutputStream(); + output.write(""" + GET / HTTP/1.1\r + Host: localhost:%d\r + Connection: close\r + \r + """.formatted(_serverURI.getPort()) + .getBytes()); + output.flush(); + + client.setSoTimeout(5000); + long start = NanoTime.now(); + HttpTester.Input input = HttpTester.from(client.getInputStream()); + HttpTester.Response response = HttpTester.parseResponse(input); + assertEquals(HttpStatus.OK_200, response.getStatus()); + assertEquals(content, response.getContent()); + assertFalse(input.isEOF()); + assertEquals(-1, input.fillBuffer()); + assertTrue(input.isEOF()); + assertThat(NanoTime.secondsSince(start), lessThan(5L)); + + } + handleComplete.countDown(); + } + @Test public void testBigBlocks() throws Exception { @@ -1813,8 +1868,9 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture } } - @Test - public void testHoldContent() throws Exception + @ParameterizedTest + @ValueSource(booleans = {false /* TODO, true */}) + public void testHoldContent(boolean close) throws Exception { Queue contents = new ConcurrentLinkedQueue<>(); final int bufferSize = 1024; @@ -1857,6 +1913,10 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture } response.setStatus(200); + + if (close) + request.getConnectionMetaData().getConnection().getEndPoint().close(); + callback.succeeded(); return true; } @@ -1897,9 +1957,12 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture out.flush(); // check the response - HttpTester.Response response = HttpTester.parseResponse(client.getInputStream()); - assertNotNull(response); - assertThat(response.getStatus(), is(200)); + if (!close) + { + HttpTester.Response response = HttpTester.parseResponse(client.getInputStream()); + assertNotNull(response); + assertThat(response.getStatus(), is(200)); + } } assertTrue(closed.await(10, TimeUnit.SECONDS));