From 3431adc2378e855ee11d97ba404b68b6c5a12ac7 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Thu, 21 Sep 2023 12:01:24 +0200 Subject: [PATCH 01/11] #10543 record failure as chunk to avoid consumeAvailable to stack overflow Signed-off-by: Ludovic Orban --- .../server/internal/HttpStreamOverHTTP3.java | 6 ++ .../transport/HttpClientStreamTest.java | 80 +++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java index f05904f85f4a..5c987f1ba306 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java @@ -528,6 +528,12 @@ public void onIdleTimeout(TimeoutException failure, BiConsumer errorHolder = new AtomicReference<>(); + long timeoutMs = switch (transport) + { + case H2, H2C -> 100; + default -> 1000; + }; + new CompletableResponseListener(client.newRequest(newURI(transport)).body(content).timeout(timeoutMs, TimeUnit.MILLISECONDS)) + .send() + .whenComplete((r, t) -> + { + errorHolder.set(t); + latch.countDown(); + }); + + assertTrue(latch.await(5, TimeUnit.SECONDS)); + assertInstanceOf(TimeoutException.class, errorHolder.get()); + } + private record HandlerContext(Request request, org.eclipse.jetty.server.Response response, Callback callback) { } From f65aecd111bf6a9bee92818397ee8a7bf7ebbf94 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 22 Sep 2023 10:43:19 +0200 Subject: [PATCH 02/11] #10543 handle review comments Signed-off-by: Ludovic Orban --- .../jetty/test/client/transport/HttpClientStreamTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java index dfe03967d51a..cb7906508a69 100644 --- a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java +++ b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java @@ -1200,13 +1200,14 @@ public boolean handle(Request request, org.eclipse.jetty.server.Response respons @ParameterizedTest @MethodSource("transports") @Tag("DisableLeakTracking:server") - public void testUploadWithRetainedData(Transport transport) throws Exception + public void testHttpStreamConsumeAvailableUponClientTimeout(Transport transport) throws Exception { start(transport, new Handler.Abstract() { @Override public boolean handle(Request request, org.eclipse.jetty.server.Response response, Callback callback) { + // Consume the uploaded data very slowly to make the client timeout. new Runnable() { @Override @@ -1250,6 +1251,8 @@ public void run() } }); + // Upload a large amount of data to the server with a timeout small enough + // that the client will timeout during the transfer. byte[] data = new byte[16 * 1024 * 1024]; new Random().nextBytes(data); ByteBufferRequestContent content = new ByteBufferRequestContent(ByteBuffer.wrap(data)); From ce0c1a8d6e3e018c67571976bb2d848f85327a5b Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 22 Sep 2023 16:01:12 +0200 Subject: [PATCH 03/11] #10543 handle review comments Signed-off-by: Ludovic Orban --- .../test/client/transport/AbstractTest.java | 8 +- .../transport/HttpClientStreamTest.java | 78 ++++++++++--------- 2 files changed, 46 insertions(+), 40 deletions(-) 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 0a5d8bd9cdf7..d381e7a36ee7 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 @@ -162,7 +162,7 @@ private static boolean isLeakTrackingDisabled(TestInfo testInfo, String tagSubVa isAnnotatedWithTagValue(testInfo.getTestClass().orElseThrow(), disableLeakTrackingTagValue); if (disabled) { - System.err.println("Not tracking leaks"); + System.err.println("Not tracking " + tagSubValue + " leaks"); return true; } @@ -172,7 +172,7 @@ private static boolean isLeakTrackingDisabled(TestInfo testInfo, String tagSubVa isAnnotatedWithTagValue(testInfo.getTestClass().orElseThrow(), disableLeakTrackingTagValue + ":" + transportName); if (disabled) { - System.err.println("Not tracking leaks for transport " + transportName); + System.err.println("Not tracking " + tagSubValue + " leaks for transport " + transportName); return true; } } @@ -181,7 +181,7 @@ private static boolean isLeakTrackingDisabled(TestInfo testInfo, String tagSubVa isAnnotatedWithTagValue(testInfo.getTestClass().orElseThrow(), disableLeakTrackingTagValue + ":" + tagSubValue); if (disabled) { - System.err.println("Not tracking leaks for " + tagSubValue); + System.err.println("Not tracking " + tagSubValue + " leaks"); return true; } @@ -191,7 +191,7 @@ private static boolean isLeakTrackingDisabled(TestInfo testInfo, String tagSubVa isAnnotatedWithTagValue(testInfo.getTestClass().orElseThrow(), disableLeakTrackingTagValue + ":" + tagSubValue + ":" + transportName); if (disabled) { - System.err.println("Not tracking leaks for " + tagSubValue + " using transport " + transportName); + System.err.println("Not tracking " + tagSubValue + " leaks for transport " + transportName); return true; } } diff --git a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java index cb7906508a69..2d7cb2d674c5 100644 --- a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java +++ b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java @@ -32,7 +32,6 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; @@ -59,6 +58,7 @@ import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.CompletableTask; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.NanoTime; import org.eclipse.jetty.util.component.LifeCycle; @@ -1199,81 +1199,87 @@ public boolean handle(Request request, org.eclipse.jetty.server.Response respons @ParameterizedTest @MethodSource("transports") - @Tag("DisableLeakTracking:server") + @Tag("DisableLeakTracking") public void testHttpStreamConsumeAvailableUponClientTimeout(Transport transport) throws Exception { + AtomicReference clientRequestRef = new AtomicReference<>(); + start(transport, new Handler.Abstract() { @Override public boolean handle(Request request, org.eclipse.jetty.server.Response response, Callback callback) { - // Consume the uploaded data very slowly to make the client timeout. - new Runnable() + new CompletableTask<>() { @Override public void run() { while (true) { - try - { - Thread.sleep(100); - } - catch (InterruptedException e) - { - // ignore - } - Content.Chunk chunk = request.read(); + + Content.Chunk chunk = request.read(); if (chunk == null) { request.demand(this); return; } - if (Content.Chunk.isFailure(chunk)) { - callback.failed(chunk.getFailure()); + completeExceptionally(chunk.getFailure()); return; } - chunk.release(); - if (chunk.isLast()) { - callback.succeeded(); + complete(null); return; } + + org.eclipse.jetty.client.Request r = clientRequestRef.getAndSet(null); + if (r != null) + { + // Abort the client request then give some time for the client's + // abort notification (e.g.: reset frame) to reach the server. + r.abort(new IllegalCallerException()); + try + { + Thread.sleep(100); + } + catch (InterruptedException e) + { + completeExceptionally(e); + return; + } + } } } - }.run(); + } + .start() + .whenComplete((result, failure) -> + { + if (failure == null) + callback.succeeded(); + else + callback.failed(failure); + }); return true; } }); - // Upload a large amount of data to the server with a timeout small enough - // that the client will timeout during the transfer. byte[] data = new byte[16 * 1024 * 1024]; new Random().nextBytes(data); ByteBufferRequestContent content = new ByteBufferRequestContent(ByteBuffer.wrap(data)); - CountDownLatch latch = new CountDownLatch(1); - AtomicReference errorHolder = new AtomicReference<>(); - long timeoutMs = switch (transport) - { - case H2, H2C -> 100; - default -> 1000; - }; - new CompletableResponseListener(client.newRequest(newURI(transport)).body(content).timeout(timeoutMs, TimeUnit.MILLISECONDS)) + org.eclipse.jetty.client.Request request = client.newRequest(newURI(transport)) + .body(content); + clientRequestRef.set(request); + Throwable throwable = new CompletableResponseListener(request) .send() - .whenComplete((r, t) -> - { - errorHolder.set(t); - latch.countDown(); - }); + .handle((r, t) -> t) + .get(5, TimeUnit.SECONDS); - assertTrue(latch.await(5, TimeUnit.SECONDS)); - assertInstanceOf(TimeoutException.class, errorHolder.get()); + assertInstanceOf(IllegalCallerException.class, throwable); } private record HandlerContext(Request request, org.eclipse.jetty.server.Response response, Callback callback) From c5bf9cc1e4ceeeeb4012363388717549435e5252 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 25 Sep 2023 11:37:12 +0200 Subject: [PATCH 04/11] #10543 make sure drainClearBytesForStream() returns -1 instead of throwing when the stream is finished to avoid a stack overflow Signed-off-by: Ludovic Orban --- .../http3/server/internal/HttpStreamOverHTTP3.java | 6 ------ .../incubator/ForeignIncubatorQuicheConnection.java | 13 +++++++++---- .../jetty/quic/quiche/jna/JnaQuicheConnection.java | 13 +++++++++---- .../test/client/transport/HttpClientStreamTest.java | 6 +++--- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java index 5c987f1ba306..f05904f85f4a 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java @@ -528,12 +528,6 @@ public void onIdleTimeout(TimeoutException failure, BiConsumer clientRequestRef = new AtomicReference<>(); @@ -1216,8 +1218,6 @@ public void run() { while (true) { - - Content.Chunk chunk = request.read(); if (chunk == null) { From 3672d7c837426d5786fd5aed13768db4f0dff6e7 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 25 Sep 2023 11:39:34 +0200 Subject: [PATCH 05/11] #10543 release buffer when fill returns -1 Signed-off-by: Ludovic Orban --- .../jetty/server/internal/HttpConnection.java | 16 +++++----------- 1 file changed, 5 insertions(+), 11 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 4746b4255347..b16da8dc2953 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 @@ -587,9 +587,14 @@ private int fillRequestBuffer() filled = getEndPoint().fill(requestBuffer); if (filled > 0) + { bytesIn.add(filled); + } else if (filled < 0) + { + releaseRequestBuffer(); _parser.atEOF(); + } if (LOG.isDebugEnabled()) LOG.debug("{} filled {} {}", this, filled, _retainableByteBuffer); @@ -1145,18 +1150,7 @@ 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; } From eb0338e5233f9cf4263683d570c24c38e8abcd58 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 25 Sep 2023 17:59:27 +0200 Subject: [PATCH 06/11] #10543 re-instate H3 fix as temporary Signed-off-by: Ludovic Orban --- .../http3/server/internal/HttpStreamOverHTTP3.java | 6 ++++++ .../incubator/ForeignIncubatorQuicheConnection.java | 13 ++++--------- .../jetty/quic/quiche/jna/JnaQuicheConnection.java | 13 ++++--------- .../test/client/transport/HttpClientStreamTest.java | 3 ++- 4 files changed, 16 insertions(+), 19 deletions(-) diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java index f05904f85f4a..5c987f1ba306 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java @@ -528,6 +528,12 @@ public void onIdleTimeout(TimeoutException failure, BiConsumer clientRequestRef = new AtomicReference<>(); From a3c982532275bdc739a69be0bf3ae030689fd350 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 26 Sep 2023 12:44:57 +0200 Subject: [PATCH 07/11] #10543 make sure HttpConnection always releases its retainable buffer when needed Signed-off-by: Ludovic Orban --- .../jetty/server/internal/HttpConnection.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 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 b16da8dc2953..8a890e71fd95 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 @@ -586,19 +586,20 @@ private int fillRequestBuffer() if (filled == 0) // Do a retry on fill 0 (optimization for SSL connections) filled = getEndPoint().fill(requestBuffer); + if (LOG.isDebugEnabled()) + LOG.debug("{} filled {} {}", this, filled, _retainableByteBuffer); + if (filled > 0) { bytesIn.add(filled); } - else if (filled < 0) + else { + if (filled < 0) + _parser.atEOF(); releaseRequestBuffer(); - _parser.atEOF(); } - if (LOG.isDebugEnabled()) - LOG.debug("{} filled {} {}", this, filled, _retainableByteBuffer); - return filled; } catch (IOException e) @@ -606,6 +607,11 @@ else if (filled < 0) if (LOG.isDebugEnabled()) LOG.debug("Unable to fill from endpoint {}", getEndPoint(), e); _parser.atEOF(); + if (_retainableByteBuffer != null) + { + _retainableByteBuffer.clear(); + releaseRequestBuffer(); + } return -1; } } From a835a17cd32b1c4bb0e6f3a5457d4ed7396ce838 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 26 Sep 2023 12:45:28 +0200 Subject: [PATCH 08/11] #10543 use var instead of fully qualified class name Signed-off-by: Ludovic Orban --- .../jetty/test/client/transport/HttpClientStreamTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java index 6525360fad29..b8a02684eb8a 100644 --- a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java +++ b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java @@ -1237,7 +1237,7 @@ public void run() return; } - org.eclipse.jetty.client.Request r = clientRequestRef.getAndSet(null); + var r = clientRequestRef.getAndSet(null); if (r != null) { // Abort the client request then give some time for the client's @@ -1272,7 +1272,7 @@ public void run() new Random().nextBytes(data); ByteBufferRequestContent content = new ByteBufferRequestContent(ByteBuffer.wrap(data)); - org.eclipse.jetty.client.Request request = client.newRequest(newURI(transport)) + var request = client.newRequest(newURI(transport)) .body(content); clientRequestRef.set(request); Throwable throwable = new CompletableResponseListener(request) From 2a4e5ca6802818b79e5774f9e62c7c871e53d0d5 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 26 Sep 2023 13:02:56 +0200 Subject: [PATCH 09/11] #10543 align buffer releasing in all streams' consumeAvailable() implementations Signed-off-by: Ludovic Orban --- .../fcgi/server/internal/HttpStreamOverFCGI.java | 15 ++++++++++++++- .../server/internal/HttpStreamOverHTTP2.java | 16 +++++++++++++++- .../server/internal/HttpStreamOverHTTP3.java | 15 ++++++++++++++- .../jetty/server/internal/HttpConnection.java | 12 ++++++++++++ 4 files changed, 55 insertions(+), 3 deletions(-) diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java index baedcf9da712..fa23f70bf32b 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java @@ -314,7 +314,20 @@ public boolean isCommitted() @Override public Throwable consumeAvailable() { - return HttpStream.consumeAvailable(this, _httpChannel.getConnectionMetaData().getHttpConfiguration()); + Throwable result = HttpStream.consumeAvailable(this, _httpChannel.getConnectionMetaData().getHttpConfiguration()); + if (result != null) + { + // 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 (_chunk != null) + { + _chunk.release(); + _chunk = Content.Chunk.next(_chunk); + } + if (_chunk == null) + _chunk = Content.Chunk.from(result, true); + } + return result; } @Override diff --git a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java index 9617ca8673c3..bedb43e4f22a 100644 --- a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java @@ -571,7 +571,21 @@ public Throwable consumeAvailable() { if (tunnelSupport != null) return null; - return HttpStream.consumeAvailable(this, _httpChannel.getConnectionMetaData().getHttpConfiguration()); + Throwable result = HttpStream.consumeAvailable(this, _httpChannel.getConnectionMetaData().getHttpConfiguration()); + if (result != null) + { + _trailer = null; + // 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 (_chunk != null) + { + _chunk.release(); + _chunk = Content.Chunk.next(_chunk); + } + if (_chunk == null) + _chunk = Content.Chunk.from(result, true); + } + return result; } @Override diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java index 5c987f1ba306..d7d3fff388e8 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java @@ -487,7 +487,20 @@ public Throwable consumeAvailable() { if (getTunnelSupport() != null) return null; - return HttpStream.consumeAvailable(this, httpChannel.getConnectionMetaData().getHttpConfiguration()); + Throwable result = HttpStream.consumeAvailable(this, httpChannel.getConnectionMetaData().getHttpConfiguration()); + if (result != null) + { + // 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 (chunk != null) + { + chunk.release(); + chunk = Content.Chunk.next(chunk); + } + if (chunk == null) + chunk = Content.Chunk.from(result, true); + } + return result; } public boolean isIdle() 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 8a890e71fd95..ef013c36241b 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 @@ -1156,7 +1156,19 @@ public Throwable consumeAvailable() { Throwable result = HttpStream.consumeAvailable(this, getHttpConfiguration()); if (result != null) + { _generator.setPersistent(false); + _retainableByteBuffer = null; + // 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 (_chunk != null) + { + _chunk.release(); + _chunk = Content.Chunk.next(_chunk); + } + if (_chunk == null) + _chunk = Content.Chunk.from(result, true); + } return result; } From 02c917334a2228eadcf83721e32e2bfb9f7b8dc4 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 26 Sep 2023 15:29:13 +0200 Subject: [PATCH 10/11] #10543 align buffer releasing in all streams' consumeAvailable() implementations Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/server/internal/HttpConnection.java | 1 - 1 file changed, 1 deletion(-) 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 ef013c36241b..cb1213a2e588 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 @@ -1158,7 +1158,6 @@ public Throwable consumeAvailable() if (result != null) { _generator.setPersistent(false); - _retainableByteBuffer = null; // 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 (_chunk != null) From e74cec3f073cefdd7bb88cf854664f19a0b0b5a8 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 26 Sep 2023 18:14:34 +0200 Subject: [PATCH 11/11] #10543 handle review comments Signed-off-by: Ludovic Orban --- .../server/internal/HttpStreamOverFCGI.java | 8 +- .../server/internal/HttpStreamOverHTTP2.java | 8 +- .../server/internal/HttpStreamOverHTTP3.java | 8 +- .../jetty/server/internal/HttpConnection.java | 79 +++++++++---------- 4 files changed, 39 insertions(+), 64 deletions(-) diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java index fa23f70bf32b..680531847e58 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java @@ -317,15 +317,9 @@ public Throwable consumeAvailable() Throwable result = HttpStream.consumeAvailable(this, _httpChannel.getConnectionMetaData().getHttpConfiguration()); if (result != null) { - // 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 (_chunk != null) - { _chunk.release(); - _chunk = Content.Chunk.next(_chunk); - } - if (_chunk == null) - _chunk = Content.Chunk.from(result, true); + _chunk = Content.Chunk.from(result, true); } return result; } diff --git a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java index bedb43e4f22a..bce32e75fbb5 100644 --- a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java @@ -575,15 +575,9 @@ public Throwable consumeAvailable() if (result != null) { _trailer = null; - // 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 (_chunk != null) - { _chunk.release(); - _chunk = Content.Chunk.next(_chunk); - } - if (_chunk == null) - _chunk = Content.Chunk.from(result, true); + _chunk = Content.Chunk.from(result, true); } return result; } diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java index d7d3fff388e8..00a07053ca21 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java @@ -490,15 +490,9 @@ public Throwable consumeAvailable() Throwable result = HttpStream.consumeAvailable(this, httpChannel.getConnectionMetaData().getHttpConfiguration()); if (result != null) { - // 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 (chunk != null) - { chunk.release(); - chunk = Content.Chunk.next(chunk); - } - if (chunk == null) - chunk = Content.Chunk.from(result, true); + chunk = Content.Chunk.from(result, true); } return result; } 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 cb1213a2e588..2262e94ec7be 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 @@ -572,50 +572,49 @@ private int fillRequestBuffer() _retainableByteBuffer = newBuffer; } - if (isRequestBufferEmpty()) - { - // Get a buffer - // We are not in a race here for the request buffer as we have not yet received a request, - // so there are not an possible legal threads calling #parseContent or #completed. - ByteBuffer requestBuffer = getRequestBuffer(); + if (!isRequestBufferEmpty()) + return _retainableByteBuffer.remaining(); - // fill - try - { - int filled = getEndPoint().fill(requestBuffer); - if (filled == 0) // Do a retry on fill 0 (optimization for SSL connections) - filled = getEndPoint().fill(requestBuffer); + // Get a buffer + // We are not in a race here for the request buffer as we have not yet received a request, + // so there are not any possible legal threads calling #parseContent or #completed. + ByteBuffer requestBuffer = getRequestBuffer(); - if (LOG.isDebugEnabled()) - LOG.debug("{} filled {} {}", this, filled, _retainableByteBuffer); + // fill + try + { + int filled = getEndPoint().fill(requestBuffer); + if (filled == 0) // Do a retry on fill 0 (optimization for SSL connections) + filled = getEndPoint().fill(requestBuffer); - if (filled > 0) - { - bytesIn.add(filled); - } - else - { - if (filled < 0) - _parser.atEOF(); - releaseRequestBuffer(); - } + if (LOG.isDebugEnabled()) + LOG.debug("{} filled {} {}", this, filled, _retainableByteBuffer); - return filled; + if (filled > 0) + { + bytesIn.add(filled); } - catch (IOException e) + else { - if (LOG.isDebugEnabled()) - LOG.debug("Unable to fill from endpoint {}", getEndPoint(), e); - _parser.atEOF(); - if (_retainableByteBuffer != null) - { - _retainableByteBuffer.clear(); - releaseRequestBuffer(); - } - return -1; + if (filled < 0) + _parser.atEOF(); + releaseRequestBuffer(); + } + + return filled; + } + catch (Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("Unable to fill from endpoint {}", getEndPoint(), x); + _parser.atEOF(); + if (_retainableByteBuffer != null) + { + _retainableByteBuffer.clear(); + releaseRequestBuffer(); } + return -1; } - return 0; } private boolean parseRequestBuffer() @@ -1158,15 +1157,9 @@ public Throwable consumeAvailable() 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 (_chunk != null) - { _chunk.release(); - _chunk = Content.Chunk.next(_chunk); - } - if (_chunk == null) - _chunk = Content.Chunk.from(result, true); + _chunk = Content.Chunk.from(result, true); } return result; }