From f3393be42b99264b7bc260b2354e3ec6858038db Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 20 Aug 2024 13:25:58 +0200 Subject: [PATCH] Fixes #9121 - Flaky BlockedWritesWithSmallThreadPoolTest.testServerThreadsBlockedInWrites(). The test uncovered a larger problem detailed in the issue: the Handler Callback should be non-blocking. Since all implementations of HttpStream are non-blocking, overridden HttpStream.getInvocationType() to return NON_BLOCKING. This guarantees that even in case of all server threads blocked, blocked/pending writes can be completed. Signed-off-by: Simone Bordet --- .../BlockedWritesWithSmallThreadPoolTest.java | 15 +++++++-------- .../java/org/eclipse/jetty/server/HttpStream.java | 6 ++++++ .../jetty/server/internal/HttpChannelState.java | 1 - .../jetty/server/internal/HttpConnection.java | 6 ------ 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/BlockedWritesWithSmallThreadPoolTest.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/BlockedWritesWithSmallThreadPoolTest.java index 64d170e79956..48fe11afed1b 100644 --- a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/BlockedWritesWithSmallThreadPoolTest.java +++ b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/BlockedWritesWithSmallThreadPoolTest.java @@ -33,6 +33,7 @@ import org.eclipse.jetty.http2.server.HTTP2CServerConnectionFactory; import org.eclipse.jetty.http2.server.RawHTTP2ServerConnectionFactory; import org.eclipse.jetty.io.AbstractEndPoint; +import org.eclipse.jetty.io.Content; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.Request; @@ -45,7 +46,6 @@ import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import static java.util.concurrent.TimeUnit.SECONDS; @@ -100,7 +100,6 @@ public void dispose() } @Test - @Tag("flaky") public void testServerThreadsBlockedInWrites() throws Exception { int contentLength = 16 * 1024 * 1024; @@ -108,11 +107,12 @@ public void testServerThreadsBlockedInWrites() throws Exception start(new Handler.Abstract() { @Override - public boolean handle(Request request, Response response, Callback callback) + public boolean handle(Request request, Response response, Callback callback) throws Exception { serverEndPointRef.compareAndSet(null, (AbstractEndPoint)request.getConnectionMetaData().getConnection().getEndPoint()); - // Write a large content to cause TCP congestion. - response.write(true, ByteBuffer.wrap(new byte[contentLength]), callback); + // Blocking write a large content to cause TCP congestion. + Content.Sink.write(response, true, ByteBuffer.wrap(new byte[contentLength])); + callback.succeeded(); return true; } }); @@ -138,21 +138,20 @@ public boolean handle(Request request, Response response, Callback callback) @Override public void onDataAvailable(Stream stream) { - Stream.Data data = stream.readData(); try { // Block here to stop reading from the network // to cause the server to TCP congest. clientBlockLatch.await(5, SECONDS); + Stream.Data data = stream.readData(); data.release(); if (data.frame().isEndStream()) clientDataLatch.countDown(); else stream.demand(); } - catch (InterruptedException x) + catch (InterruptedException ignored) { - data.release(); } } }); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java index 93b86096ffb7..c6b488107a78 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java @@ -135,6 +135,12 @@ static Throwable consumeAvailable(HttpStream stream, HttpConfiguration httpConfi return CONTENT_NOT_CONSUMED; } + @Override + default InvocationType getInvocationType() + { + return InvocationType.NON_BLOCKING; + } + class Wrapper implements HttpStream { private final HttpStream _wrapped; diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index 5aa3287355e6..af8ca07824ed 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -1636,7 +1636,6 @@ private boolean lockedCompleteCallback() @Override public InvocationType getInvocationType() { - // TODO review this as it is probably not correct return _request.getHttpStream().getInvocationType(); } } 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 3d479670005f..b61924ca4547 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 @@ -1610,12 +1610,6 @@ private void abort(Throwable failure) { getEndPoint().close(failure); } - - @Override - public InvocationType getInvocationType() - { - return HttpStream.super.getInvocationType(); - } } private class TunnelSupportOverHTTP1 implements TunnelSupport