From 6858307760e40d087fcef1b661748d385a0f67d6 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 15 Jul 2024 18:18:09 +1000 Subject: [PATCH 01/22] Experiment with IteratingCallback The previous semantic of `onCompleteFailure` has been renamed to `onFailure(Throwable)`, which is called immediately (but serialized) on either an abort or a failure. A new `onCompleteFailure(Throwable)` method has been added that is called only after a `failed(throwable)` or a `abort(Throwable)` followed by `succeeded()` or `failed(Throwable)`` No usage has yet been made of the new `onCompleteFailure`, but the ICB implementation has been completely replaced by the one developed in #11876 --- .../jetty/docs/programming/ContentDocs.java | 2 +- .../docs/programming/SelectorManagerDocs.java | 2 +- .../jetty/docs/programming/WebSocketDocs.java | 2 +- .../docs/programming/server/ServerDocs.java | 2 +- .../jetty/client/transport/HttpSender.java | 2 +- .../internal/HttpSenderOverHTTP.java | 6 +- .../eclipse/jetty/fcgi/generator/Flusher.java | 2 +- .../jetty/http2/internal/HTTP2Flusher.java | 2 +- .../jetty/http2/tests/RawHTTP2ProxyTest.java | 4 +- .../eclipse/jetty/http3/ControlFlusher.java | 2 +- .../jetty/http3/InstructionFlusher.java | 2 +- .../eclipse/jetty/http3/MessageFlusher.java | 2 +- .../org/eclipse/jetty/io/IOResources.java | 4 +- .../jetty/io/internal/ContentCopier.java | 4 +- .../jetty/quic/common/QuicConnection.java | 2 +- .../jetty/quic/common/QuicSession.java | 2 +- .../jetty/server/handler/ConnectHandler.java | 2 +- .../handler/gzip/GzipResponseAndCallback.java | 4 +- .../jetty/server/internal/HttpConnection.java | 2 +- .../client/transport/CustomTransportTest.java | 4 +- .../eclipse/jetty/util/IteratingCallback.java | 611 +++++++++++----- .../jetty/util/IteratingNestedCallback.java | 2 +- .../jetty/util/IteratingCallbackTest.java | 665 ++++++++++++++++-- .../websocket/core/WebSocketConnection.java | 4 +- .../websocket/core/internal/FrameFlusher.java | 2 +- .../internal/PerMessageDeflateExtension.java | 4 +- .../websocket/core/util/DemandingFlusher.java | 2 +- .../core/util/TransformingFlusher.java | 4 +- .../core/internal/FrameFlusherTest.java | 4 +- .../ee10/proxy/AsyncMiddleManServlet.java | 2 +- .../jetty/ee10/proxy/AsyncProxyServlet.java | 2 +- .../jetty/ee10/servlet/HttpOutput.java | 14 +- .../ee11/proxy/AsyncMiddleManServlet.java | 2 +- .../jetty/ee11/servlet/HttpOutput.java | 14 +- .../nested/FileBufferedResponseHandler.java | 2 +- .../eclipse/jetty/ee9/nested/HttpOutput.java | 14 +- .../ee9/proxy/AsyncMiddleManServlet.java | 2 +- .../jetty/ee9/proxy/AsyncProxyServlet.java | 2 +- 38 files changed, 1089 insertions(+), 317 deletions(-) diff --git a/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java b/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java index d406ee1d3bd9..4b932825b6d7 100644 --- a/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java +++ b/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java @@ -361,7 +361,7 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable failure) + protected void onFailure(Throwable failure) { // In case of a failure, either on the // read or on the write, release the chunk. diff --git a/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java b/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java index cc60b78e7f92..4f31cc613301 100644 --- a/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java +++ b/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java @@ -298,7 +298,7 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { // The iteration completed with a failure. getEndPoint().close(cause); diff --git a/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java b/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java index b0a65e64e336..84939d8957d2 100644 --- a/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java +++ b/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java @@ -541,7 +541,7 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable x) + protected void onFailure(Throwable x) { x.printStackTrace(); } diff --git a/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java b/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java index a109be5ed6d3..deb03d16822c 100644 --- a/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java +++ b/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java @@ -241,7 +241,7 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { getEndPoint().close(cause); } diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java index 551a210ae0e7..3b505886ebe1 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java @@ -591,7 +591,7 @@ else if (expect100) } @Override - protected void onCompleteFailure(Throwable x) + protected void onFailure(Throwable x) { if (chunk != null) { diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java index 1f6bc4a3f756..cdc0b150df65 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java @@ -248,9 +248,9 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { - super.onCompleteFailure(cause); + super.onFailure(cause); release(); callback.failed(cause); } @@ -335,7 +335,7 @@ protected Action process() throws Exception } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { release(); callback.failed(cause); diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java index 9c64ffb77943..f5431cf9061d 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java @@ -109,7 +109,7 @@ protected void onSuccess() } @Override - public void onCompleteFailure(Throwable x) + public void onFailure(Throwable x) { if (active != null) active.failed(x); diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java index 1d3849f066f5..68f9521459ec 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java @@ -344,7 +344,7 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable x) + protected void onFailure(Throwable x) { release(); diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java index 59827ebe1958..77a4a1d0b722 100644 --- a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java +++ b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/RawHTTP2ProxyTest.java @@ -519,7 +519,7 @@ protected void onSuccess() } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { frameInfo.callback.failed(cause); } @@ -673,7 +673,7 @@ protected void onSuccess() } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { frameInfo.callback.failed(cause); } diff --git a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/ControlFlusher.java b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/ControlFlusher.java index deee66c03b7f..7f4cb53c7db2 100644 --- a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/ControlFlusher.java +++ b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/ControlFlusher.java @@ -122,7 +122,7 @@ protected void onSuccess() } @Override - protected void onCompleteFailure(Throwable failure) + protected void onFailure(Throwable failure) { if (LOG.isDebugEnabled()) LOG.debug("failed to write {} on {}", entries, this, failure); diff --git a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java index 7a41ba177c6b..7b73c228179a 100644 --- a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java +++ b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java @@ -118,7 +118,7 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable failure) + protected void onFailure(Throwable failure) { if (LOG.isDebugEnabled()) LOG.debug("failed to write buffers on {}", this, failure); diff --git a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java index f450bfaf0ec2..a0073fa2c317 100644 --- a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java +++ b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java @@ -118,7 +118,7 @@ protected void onSuccess() } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { if (LOG.isDebugEnabled()) LOG.debug("failed to write {} on {}", entry, this, cause); diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/IOResources.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/IOResources.java index 41a59c0330a7..0e2fc7f481e3 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/IOResources.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/IOResources.java @@ -413,12 +413,12 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable x) + protected void onFailure(Throwable x) { if (retainableByteBuffer != null) retainableByteBuffer.release(); IO.close(channel); - super.onCompleteFailure(x); + super.onFailure(x); } } } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java index b4b59f851d4b..bff4b267dea2 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java @@ -77,13 +77,13 @@ protected Action process() throws Throwable } @Override - protected void onCompleteFailure(Throwable x) + protected void onFailure(Throwable x) { if (current != null) { current.release(); current = Content.Chunk.next(current); } - ExceptionUtil.callAndThen(x, source::fail, super::onCompleteFailure); + ExceptionUtil.callAndThen(x, source::fail, super::onFailure); } } diff --git a/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicConnection.java b/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicConnection.java index e5c1d340b659..3dba959a0d6d 100644 --- a/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicConnection.java +++ b/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicConnection.java @@ -377,7 +377,7 @@ public InvocationType getInvocationType() } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { entry.callback.failed(cause); QuicConnection.this.close(); diff --git a/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java b/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java index 75ee111e659a..aeb09e59ff09 100644 --- a/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java +++ b/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java @@ -543,7 +543,7 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable failure) + protected void onFailure(Throwable failure) { if (LOG.isDebugEnabled()) LOG.debug("failed to write cipher bytes, closing session on {}", QuicSession.this, failure); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java index 51fdb7e10117..9a7a6183e3da 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java @@ -768,7 +768,7 @@ protected void onSuccess() } @Override - protected void onCompleteFailure(Throwable x) + protected void onFailure(Throwable x) { if (LOG.isDebugEnabled()) LOG.debug("Failed to write {} bytes {}", filled, TunnelConnection.this, x); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipResponseAndCallback.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipResponseAndCallback.java index d14d2c68d015..702f765f082d 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipResponseAndCallback.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipResponseAndCallback.java @@ -322,14 +322,14 @@ public GzipBufferCB(boolean complete, Callback callback, ByteBuffer content) } @Override - protected void onCompleteFailure(Throwable x) + protected void onFailure(Throwable x) { if (_deflaterEntry != null) { _deflaterEntry.release(); _deflaterEntry = null; } - super.onCompleteFailure(x); + super.onFailure(x); } @Override 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 2027b6542fc0..57814be4f816 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 @@ -910,7 +910,7 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(final Throwable x) + public void onFailure(final Throwable x) { failedCallback(release(), x); } diff --git a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/CustomTransportTest.java b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/CustomTransportTest.java index a3a7ecf9a464..7b535efcbfe9 100644 --- a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/CustomTransportTest.java +++ b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/CustomTransportTest.java @@ -335,7 +335,7 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { // There was a write error, close the Gateway Channel. channel.close(cause); @@ -378,7 +378,7 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { // There was a write error, close the Gateway Channel. channel.close(cause); diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java index 155577f58e24..5f4169f4b720 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java @@ -14,8 +14,11 @@ package org.eclipse.jetty.util; import java.io.IOException; +import java.util.Objects; import org.eclipse.jetty.util.thread.AutoLock; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * This specialized callback implements a pattern that allows @@ -41,20 +44,27 @@ *

* Subclasses must implement method {@link #process()} where the * asynchronous sub-task is initiated and a suitable {@link Action} - * is returned to this callback to indicate the overall progress of + * is returned to this callback to indicate the overall progress ofk * the large asynchronous task. * This callback is passed to the asynchronous sub-task, and a call * to {@link #succeeded()} on this callback represents the successful * completion of the asynchronous sub-task, while a call to * {@link #failed(Throwable)} on this callback represents the * completion with a failure of the large asynchronous task. + *

+ * For most purposes, the {@link #succeeded()} and {@link #failed(Throwable)} + * methods of this class should be considered final, and only overridden in + * extraordinary circumstances. Any action taken in such extensions are not + * serialized. */ public abstract class IteratingCallback implements Callback { + private static final Logger LOG = LoggerFactory.getLogger(IteratingCallback.class); + /** * The internal states of this callback. */ - private enum State + enum State { /** * This callback is idle, ready to iterate. @@ -64,48 +74,35 @@ private enum State /** * This callback is just about to call {@link #process()}, * or within it, or just exited from it, either normally - * or by throwing. + * or by throwing. Further actions are waiting for the + * {@link #process()} method to return. */ PROCESSING, /** - * Method {@link #process()} returned {@link Action#SCHEDULED} - * and this callback is waiting for the asynchronous sub-task - * to complete. - */ - PENDING, - - /** - * The asynchronous sub-task was completed successfully - * via a call to {@link #succeeded()} while in - * {@link #PROCESSING} state. - */ - CALLED, - - /** - * The iteration terminated successfully as indicated by - * {@link Action#SUCCEEDED} returned from - * {@link IteratingCallback#process()}. + * The asynchronous sub-task was completed either with + * a call to {@link #succeeded()} or {@link #failed(Throwable)}, whilst in + * {@link #PROCESSING} state. Further actions are waiting for the + * {@link #process()} method to return. */ - SUCCEEDED, + PROCESSING_CALLED, /** - * The iteration terminated with a failure via a call - * to {@link IteratingCallback#failed(Throwable)}. + * Method {@link #process()} returned {@link Action#SCHEDULED} + * and this callback is waiting for the asynchronous sub-task + * to complete via a callback to {@link #succeeded()} or {@link #failed(Throwable)} */ - FAILED, + PENDING, /** - * This callback has been {@link #close() closed} and - * cannot be {@link #reset() reset}. + * This callback is complete. */ - CLOSED, + COMPLETE, /** - * This callback has been {@link #abort(Throwable) aborted}, - * and cannot be {@link #reset() reset}. + * Complete and can't be reset. */ - ABORTED + CLOSED } /** @@ -120,6 +117,7 @@ protected enum Action * for additional events to trigger more work. */ IDLE, + /** * Indicates that {@link #process()} has initiated an asynchronous * sub-task, where the execution has started but the callback @@ -127,6 +125,7 @@ protected enum Action * may have not yet been invoked. */ SCHEDULED, + /** * Indicates that {@link #process()} has completed the whole * iteration successfully. @@ -137,7 +136,8 @@ protected enum Action private final AutoLock _lock = new AutoLock(); private State _state; private Throwable _failure; - private boolean _iterate; + private boolean _reprocess; + private boolean _aborted; protected IteratingCallback() { @@ -146,7 +146,7 @@ protected IteratingCallback() protected IteratingCallback(boolean needReset) { - _state = needReset ? State.SUCCEEDED : State.IDLE; + _state = needReset ? State.COMPLETE : State.IDLE; } /** @@ -179,8 +179,24 @@ protected void onSuccess() { } + /** + * Invoked when the overall task has been {@link #abort(Throwable) aborted} or {@link #failed(Throwable) failed}. + *

+ * Calls to this method are serialized with respect to {@link #onAborted(Throwable)}, {@link #process()}, + * {@link #onCompleteFailure(Throwable)} and {@link #onCompleted(Throwable)}. + * + * @param cause The cause of the failure or abort + */ + protected void onFailure(Throwable cause) + { + } + /** * Invoked when the overall task has completed successfully. + *

+ * Calls to this method are serialized with respect to {@link #process()}, {@link #onAborted(Throwable)} + * and {@link #onCompleted(Throwable)}. + * If this method is called, then {@link #onCompleteFailure(Throwable)} ()} will never be called. * * @see #onCompleteFailure(Throwable) */ @@ -190,6 +206,10 @@ protected void onCompleteSuccess() /** * Invoked when the overall task has completed with a failure. + *

+ * Calls to this method are serialized with respect to {@link #process()}, {@link #onAborted(Throwable)} + * and {@link #onCompleted(Throwable)}. + * If this method is called, then {@link #onCompleteSuccess()} will never be called. * * @param cause the throwable to indicate cause of failure * @see #onCompleteSuccess() @@ -198,6 +218,50 @@ protected void onCompleteFailure(Throwable cause) { } + /** + * Invoked when the overall task has been aborted. + *

+ * Calls to this method are serialized with respect to {@link #process()}, {@link #onCompleteFailure(Throwable)} + * and {@link #onCompleted(Throwable)}. + * If this method is called, then {@link #onCompleteSuccess()} will never be called. + *

+ * The default implementation of this method calls {@link #failed(Throwable)}. Overridden implementations of + * this method SHOULD NOT call {@code super.onAborted(Throwable)}. + * + * @param cause The cause of the abort + */ + protected void onAborted(Throwable cause) + { + } + + /** + * Invoked when the overall task has completed. + *

+ * Calls to this method are serialized with respect to {@link #process()} and {@link #onAborted(Throwable)}. + * The default implementation of this method will call either {@link #onCompleteSuccess()} or {@link #onCompleteFailure(Throwable)} + * thus implementations of this method should always call {@code super.onCompleted(Throwable)}. + * + * @param causeOrNull the cause of any {@link #abort(Throwable) abort} or {@link #failed(Throwable) failure}, + * else {@code null} for {@link #succeeded() success}. + */ + protected void onCompleted(Throwable causeOrNull) + { + if (causeOrNull == null) + onCompleteSuccess(); + else + onCompleteFailure(causeOrNull); + } + + private void doCompleteSuccess() + { + onCompleted(null); + } + + private void doCompleteFailure(Throwable cause) + { + onCompleted(cause); + } + /** * This method must be invoked by applications to start the processing * of asynchronous sub-tasks. @@ -215,28 +279,18 @@ public void iterate() { switch (_state) { - case PENDING: - case CALLED: - // process will be called when callback is handled - break; - case IDLE: _state = State.PROCESSING; process = true; break; case PROCESSING: - _iterate = true; - break; - - case FAILED: - case SUCCEEDED: + case PROCESSING_CALLED: + _reprocess = true; break; - case CLOSED: - case ABORTED: default: - throw new IllegalStateException(toString()); + break; } } if (process) @@ -248,21 +302,24 @@ private void processing() // This should only ever be called when in processing state, however a failed or close call // may happen concurrently, so state is not assumed. - boolean notifyCompleteSuccess = false; - Throwable notifyCompleteFailure = null; + boolean completeSuccess = false; + Throwable abortDoCompleteFailure = null; + Throwable completeFailure = null; + Throwable onAbort = null; // While we are processing processing: while (true) { // Call process to get the action that we have to take. - Action action = null; + Action action; try { action = process(); } catch (Throwable x) { + action = null; failed(x); // Fall through to possibly invoke onCompleteFailure(). } @@ -271,72 +328,104 @@ private void processing() // acted on the action we have just received try (AutoLock ignored = _lock.lock()) { + if (LOG.isDebugEnabled()) + LOG.debug("processing {} {}", action, this); + switch (_state) { case PROCESSING: { - if (action != null) + if (action == null) + break processing; + switch (action) { - switch (action) + case IDLE: { - case IDLE: + if (_aborted) { - // Has iterate been called while we were processing? - if (_iterate) - { - // yes, so skip idle and keep processing - _iterate = false; - continue; - } - - // No, so we can go idle - _state = State.IDLE; + _state = _failure instanceof ClosedException ? State.CLOSED : State.COMPLETE; + abortDoCompleteFailure = _failure; break processing; } - case SCHEDULED: + + // Has iterate been called while we were processing? + if (_reprocess) { - // we won the race against the callback, so the callback has to process and we can break processing - _state = State.PENDING; - break processing; + // yes, so skip idle and keep processing + _reprocess = false; + continue; + } + + // No, so we can go idle + _state = State.IDLE; + break processing; + } + case SCHEDULED: + { + // we won the race against the callback, so the callback has to process and we can break processing + _state = State.PENDING; + if (_aborted) + { + onAbort = _failure; + _failure = new AbortingException(onAbort); } - case SUCCEEDED: + break processing; + } + case SUCCEEDED: + { + // we lost the race against the callback, + _reprocess = false; + if (_aborted) { - // we lost the race against the callback, - _iterate = false; - _state = State.SUCCEEDED; - notifyCompleteSuccess = true; - break processing; + _state = _failure instanceof ClosedException ? State.CLOSED : State.COMPLETE; + abortDoCompleteFailure = _failure; } - default: + else { - break; + _state = State.COMPLETE; + completeSuccess = true; } + break processing; + } + default: + { + break; } } throw new IllegalStateException(String.format("%s[action=%s]", this, action)); } - case CALLED: + case PROCESSING_CALLED: { + if (action != Action.SCHEDULED && action != null) + { + _state = State.CLOSED; + abortDoCompleteFailure = new IllegalStateException("Action not scheduled"); + if (_failure == null) + { + _failure = abortDoCompleteFailure; + } + else + { + ExceptionUtil.addSuppressedIfNotAssociated(_failure, onAbort); + abortDoCompleteFailure = _failure; + } + break processing; + } + if (_failure != null) + { + if (_aborted) + abortDoCompleteFailure = _failure; + else + completeFailure = _failure; + _state = _failure instanceof ClosedException ? State.CLOSED : State.COMPLETE; + break processing; + } callOnSuccess = true; - if (action != Action.SCHEDULED) - throw new IllegalStateException(String.format("%s[action=%s]", this, action)); - // we lost the race, so we have to keep processing _state = State.PROCESSING; - continue; + break; } - case FAILED: - case CLOSED: - case ABORTED: - notifyCompleteFailure = _failure; - break processing; - - case SUCCEEDED: - break processing; - - case IDLE: - case PENDING: default: throw new IllegalStateException(String.format("%s[action=%s]", this, action)); } @@ -347,47 +436,74 @@ private void processing() onSuccess(); } } - - if (notifyCompleteSuccess) - onCompleteSuccess(); - else if (notifyCompleteFailure != null) - onCompleteFailure(notifyCompleteFailure); + if (abortDoCompleteFailure != null) + ExceptionUtil.callAndThen(abortDoCompleteFailure, this::doOnAbortedOnFailure, this::doCompleteFailure); + else if (completeSuccess) + doCompleteSuccess(); + else if (completeFailure != null) + ExceptionUtil.callAndThen(completeFailure, this::onFailure, this::doCompleteFailure); + else if (onAbort != null) + ExceptionUtil.callAndThen(onAbort, this::doOnAbortedOnFailure, this::doAbortPendingCompletion); } /** * Method to invoke when the asynchronous sub-task succeeds. *

- * This method should be considered final for all practical purposes. - *

+ * For most purposes, this method should be considered {@code final} and should only be + * overridden in extraordinary circumstances. + * Subclasses that override this method must always call {@code super.succeeded()}. + * Such overridden methods are not serialized with respect to {@link #process()}, {@link #onCompleteSuccess()}, + * {@link #onCompleteFailure(Throwable)}, nor {@link #onAborted(Throwable)}. They should not act on nor change any + * fields that may be used by those methods. * Eventually, {@link #onSuccess()} is * called, either by the caller thread or by the processing * thread. */ @Override - public void succeeded() + public final void succeeded() { boolean process = false; + Throwable completeFailure = null; try (AutoLock ignored = _lock.lock()) { + if (LOG.isDebugEnabled()) + LOG.debug("succeeded {}", this); switch (_state) { case PROCESSING: { - _state = State.CALLED; + // Another thread is processing, so we just tell it the state + _state = State.PROCESSING_CALLED; break; } case PENDING: { - _state = State.PROCESSING; - process = true; + if (_aborted) + { + if (_failure instanceof AbortingException) + { + // Another thread is still calling onAborted, so we will let it do the completion + _state = _failure.getCause() instanceof ClosedException ? State.CLOSED : State.COMPLETE; + } + else + { + // The onAborted call is complete, so we must do the completion + _state = _failure instanceof ClosedException ? State.CLOSED : State.COMPLETE; + completeFailure = _failure; + } + } + else + { + // No other thread is processing, so we will do the processing + _state = State.PROCESSING; + process = true; + } break; } - case FAILED: - case CLOSED: - case ABORTED: + case COMPLETE, CLOSED: { - // Too late! - break; + // Too late + return; } default: { @@ -397,8 +513,11 @@ public void succeeded() } if (process) { - onSuccess(); - processing(); + ExceptionUtil.callAndThen(this::onSuccess, this::processing); + } + else if (completeFailure != null) + { + doCompleteFailure(completeFailure); } } @@ -407,47 +526,84 @@ public void succeeded() * or to fail the overall asynchronous task and therefore * terminate the iteration. *

- * This method should be considered final for all practical purposes. - *

* Eventually, {@link #onCompleteFailure(Throwable)} is * called, either by the caller thread or by the processing * thread. - * + *

+ * For most purposes, this method should be considered {@code final} and should only be + * overridden in extraordinary circumstances. + * Subclasses that override this method must always call {@code super.succeeded()}. + * Such overridden methods are not serialized with respect to {@link #process()}, {@link #onCompleteSuccess()}, + * {@link #onCompleteFailure(Throwable)}, nor {@link #onAborted(Throwable)}. They should not act on nor change any + * fields that may be used by those methods. * @see #isFailed() */ @Override - public void failed(Throwable x) + public final void failed(Throwable cause) { - boolean failure = false; + cause = Objects.requireNonNullElseGet(cause, IOException::new); + + Throwable completeFailure = null; + Throwable abortCompletion = null; try (AutoLock ignored = _lock.lock()) { + if (LOG.isDebugEnabled()) + LOG.debug("failed {}", this, cause); switch (_state) { - case CALLED: - case SUCCEEDED: - case FAILED: - case CLOSED: - case ABORTED: - // Too late! + case PROCESSING: + { + // Another thread is processing, so we just tell it the state + _state = State.PROCESSING_CALLED; + if (_failure == null) + _failure = cause; + else + ExceptionUtil.addSuppressedIfNotAssociated(_failure, cause); break; + } case PENDING: { - _state = State.FAILED; - failure = true; + if (_aborted) + { + if (_failure instanceof AbortingException) + { + // Another thread is still calling onAborted, so we will let it do the completion + ExceptionUtil.addSuppressedIfNotAssociated(_failure.getCause(), cause); + _state = _failure.getCause() instanceof ClosedException ? State.CLOSED : State.COMPLETE; + } + else + { + // The onAborted call is complete, so we must do the completion + ExceptionUtil.addSuppressedIfNotAssociated(_failure, cause); + _state = _failure instanceof ClosedException ? State.CLOSED : State.COMPLETE; + abortCompletion = _failure; + } + } + else + { + // No other thread is processing, so we will do the processing + _state = State.COMPLETE; + _failure = cause; + completeFailure = _failure; + } break; } - case PROCESSING: + case COMPLETE, CLOSED: { - _state = State.FAILED; - _failure = x; - break; + // Too late + ExceptionUtil.addSuppressedIfNotAssociated(_failure, cause); + return; } default: + { throw new IllegalStateException(toString()); + } } } - if (failure) - onCompleteFailure(x); + if (completeFailure != null) + ExceptionUtil.callAndThen(completeFailure, this::onFailure, this::doCompleteFailure); + else if (abortCompletion != null) + doCompleteFailure(abortCompletion); } /** @@ -459,37 +615,63 @@ public void failed(Throwable x) * * @see #isClosed() */ - public void close() + public final void close() { - String failure = null; + Throwable onAbort = null; + Throwable onAbortDoCompleteFailure = null; + try (AutoLock ignored = _lock.lock()) { + if (LOG.isDebugEnabled()) + LOG.debug("close {}", this); switch (_state) { - case IDLE: - case SUCCEEDED: - case FAILED: - _state = State.CLOSED; - break; - - case PROCESSING: - _failure = new IOException(String.format("Close %s in state %s", this, _state)); + case IDLE -> + { + // Nothing happening so we can abort and complete _state = State.CLOSED; - break; + _failure = new ClosedException(); + onAbortDoCompleteFailure = _failure; + } + case PROCESSING, PROCESSING_CALLED -> + { + // Another thread is processing, so we just tell it the state and let it handle it + if (_aborted) + { + ExceptionUtil.addSuppressedIfNotAssociated(_failure, new ClosedException()); + } + else + { + _aborted = true; + _failure = new ClosedException(); + } + } - case CLOSED: - case ABORTED: - break; + case PENDING -> + { + // We are waiting for the callback, so we can only call onAbort and then keep waiting + onAbort = new ClosedException(); + _failure = new AbortingException(onAbort); + _aborted = true; + } - default: - failure = String.format("Close %s in state %s", this, _state); + case COMPLETE -> + { _state = State.CLOSED; - break; + } + + case CLOSED -> + { + // too late + return; + } } } - if (failure != null) - onCompleteFailure(new IOException(failure)); + if (onAbort != null) + ExceptionUtil.callAndThen(onAbort, this::doOnAbortedOnFailure, this::doAbortPendingCompletion); + else if (onAbortDoCompleteFailure != null) + ExceptionUtil.callAndThen(onAbortDoCompleteFailure, this::doOnAbortedOnFailure, this::doCompleteFailure); } /** @@ -498,49 +680,106 @@ public void close() * ultimately be invoked, either during this call or later after * any call to {@link #process()} has returned.

* - * @param failure the cause of the abort + * @param cause the cause of the abort + * @return {@code true} if abort was called before the callback was complete. * @see #isAborted() */ - public void abort(Throwable failure) + public final boolean abort(Throwable cause) { - boolean abort = false; + cause = Objects.requireNonNullElseGet(cause, Throwable::new); + + boolean onAbort = false; + boolean onAbortDoCompleteFailure = false; try (AutoLock ignored = _lock.lock()) { + if (LOG.isDebugEnabled()) + LOG.debug("abort {}", this, cause); + + // Are we already aborted? + if (_aborted) + { + ExceptionUtil.addSuppressedIfNotAssociated(_failure, cause); + return false; + } + switch (_state) { - case SUCCEEDED: - case FAILED: - case CLOSED: - case ABORTED: + case IDLE: { - // Too late. + // Nothing happening so we can abort and complete + _state = State.COMPLETE; + _failure = cause; + _aborted = true; + onAbortDoCompleteFailure = true; break; } - case IDLE: - case PENDING: + case PROCESSING: { - _failure = failure; - _state = State.ABORTED; - abort = true; + // Another thread is processing, so we just tell it the state and let it handle everything + _failure = cause; + _aborted = true; break; } - case PROCESSING: - case CALLED: + case PROCESSING_CALLED: { - _failure = failure; - _state = State.ABORTED; + // Another thread is processing, but we have already succeeded or failed. + if (_failure == null) + _failure = cause; + else + ExceptionUtil.addSuppressedIfNotAssociated(_failure, cause); + _aborted = true; break; } - default: - throw new IllegalStateException(toString()); + case PENDING: + { + // We are waiting for the callback, so we can only call onAbort and then keep waiting + onAbort = true; + _failure = new AbortingException(cause); + _aborted = true; + break; + } + + case COMPLETE, CLOSED: + { + // too late + ExceptionUtil.addSuppressedIfNotAssociated(_failure, cause); + return false; + } } } - if (abort) - onCompleteFailure(failure); + if (onAbortDoCompleteFailure) + ExceptionUtil.callAndThen(cause, this::doOnAbortedOnFailure, this::doCompleteFailure); + else if (onAbort) + ExceptionUtil.callAndThen(cause, this::doOnAbortedOnFailure, this::doAbortPendingCompletion); + + return true; + } + + private void doOnAbortedOnFailure(Throwable cause) + { + ExceptionUtil.callAndThen(cause, this::onAborted, this::onFailure); + } + + private void doAbortPendingCompletion() + { + Throwable doCompleteFailure = null; + try (AutoLock ignored = _lock.lock()) + { + _failure = _failure.getCause(); + + if (Objects.requireNonNull(_state) != State.PENDING) + { + // the callback completed, one way or another, so it is up to use to do the completion + doCompleteFailure = _failure; + } + } + + if (doCompleteFailure != null) + ExceptionUtil.call(doCompleteFailure, this::doCompleteFailure); } /** @@ -561,7 +800,7 @@ public boolean isClosed() { try (AutoLock ignored = _lock.lock()) { - return _state == State.CLOSED; + return _state == State.CLOSED || _failure instanceof ClosedException; } } @@ -572,7 +811,7 @@ public boolean isFailed() { try (AutoLock ignored = _lock.lock()) { - return _state == State.FAILED; + return _failure != null; } } @@ -585,7 +824,7 @@ public boolean isSucceeded() { try (AutoLock ignored = _lock.lock()) { - return _state == State.SUCCEEDED; + return _state == State.COMPLETE && _failure == null; } } @@ -596,7 +835,7 @@ public boolean isAborted() { try (AutoLock ignored = _lock.lock()) { - return _state == State.ABORTED; + return _aborted; } } @@ -618,11 +857,10 @@ public boolean reset() case IDLE: return true; - case SUCCEEDED: - case FAILED: + case COMPLETE: _state = State.IDLE; _failure = null; - _iterate = false; + _reprocess = false; return true; default: @@ -634,6 +872,31 @@ public boolean reset() @Override public String toString() { - return String.format("%s@%x[%s]", getClass().getSimpleName(), hashCode(), _state); + try (AutoLock ignored = _lock.lock()) + { + return String.format("%s@%x[%s, %b, %s]", getClass().getSimpleName(), hashCode(), _state, _aborted, _failure); + } + } + + private static class ClosedException extends Exception + { + ClosedException() + { + super("Closed"); + } + + ClosedException(Throwable suppressed) + { + this(); + ExceptionUtil.addSuppressedIfNotAssociated(this, suppressed); + } + } + + private static class AbortingException extends Exception + { + AbortingException(Throwable cause) + { + super(cause.getMessage(), cause); + } } } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingNestedCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingNestedCallback.java index 745e8b800d7d..1d409cd859ed 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingNestedCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingNestedCallback.java @@ -53,7 +53,7 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable x) + protected void onFailure(Throwable x) { _callback.failed(x); } diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java index 00f6b007b7dd..cd04a884f46f 100644 --- a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java @@ -13,19 +13,37 @@ package org.eclipse.jetty.util; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicMarkableReference; +import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Stream; +import org.awaitility.Awaitility; import org.eclipse.jetty.util.thread.ScheduledExecutorScheduler; import org.eclipse.jetty.util.thread.Scheduler; +import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; - +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.sameInstance; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; public class IteratingCallbackTest @@ -202,46 +220,31 @@ protected Action process() { processed++; - switch (i--) + return switch (i--) { - case 5: + case 5, 2 -> + { succeeded(); - return Action.SCHEDULED; - - case 4: + yield Action.SCHEDULED; + } + case 4, 1 -> + { scheduler.schedule(successTask, 5, TimeUnit.MILLISECONDS); - return Action.SCHEDULED; - - case 3: - scheduler.schedule(new Runnable() - { - @Override - public void run() - { - idle.countDown(); - } - }, 5, TimeUnit.MILLISECONDS); - return Action.IDLE; - - case 2: - succeeded(); - return Action.SCHEDULED; - - case 1: - scheduler.schedule(successTask, 5, TimeUnit.MILLISECONDS); - return Action.SCHEDULED; - - case 0: - return Action.SUCCEEDED; - - default: - throw new IllegalStateException(); - } + yield Action.SCHEDULED; + } + case 3 -> + { + scheduler.schedule(idle::countDown, 5, TimeUnit.MILLISECONDS); + yield Action.IDLE; + } + case 0 -> Action.SUCCEEDED; + default -> throw new IllegalStateException(); + }; } }; cb.iterate(); - idle.await(10, TimeUnit.SECONDS); + assertTrue(idle.await(10, TimeUnit.SECONDS)); assertTrue(cb.isIdle()); cb.iterate(); @@ -252,25 +255,21 @@ public void run() @Test public void testCloseDuringProcessingReturningScheduled() throws Exception { - testCloseDuringProcessing(IteratingCallback.Action.SCHEDULED); - } - - @Test - public void testCloseDuringProcessingReturningSucceeded() throws Exception - { - testCloseDuringProcessing(IteratingCallback.Action.SUCCEEDED); - } - - private void testCloseDuringProcessing(final IteratingCallback.Action action) throws Exception - { + final CountDownLatch abortLatch = new CountDownLatch(1); final CountDownLatch failureLatch = new CountDownLatch(1); IteratingCallback callback = new IteratingCallback() { @Override - protected Action process() throws Exception + protected Action process() { close(); - return action; + return Action.SCHEDULED; + } + + @Override + protected void onAborted(Throwable cause) + { + abortLatch.countDown(); } @Override @@ -282,27 +281,45 @@ protected void onCompleteFailure(Throwable cause) callback.iterate(); - assertTrue(failureLatch.await(5, TimeUnit.SECONDS)); + assertFalse(failureLatch.await(100, TimeUnit.MILLISECONDS)); + assertTrue(abortLatch.await(1000000000, TimeUnit.SECONDS)); + assertTrue(callback.isClosed()); + + callback.succeeded(); + assertTrue(failureLatch.await(1, TimeUnit.SECONDS)); + assertTrue(callback.isFailed()); + assertTrue(callback.isClosed()); } - private abstract static class TestCB extends IteratingCallback + @Test + public void testCloseDuringProcessingReturningSucceeded() throws Exception { - protected Runnable successTask = new Runnable() + final CountDownLatch failureLatch = new CountDownLatch(1); + IteratingCallback callback = new IteratingCallback() { @Override - public void run() + protected Action process() { - succeeded(); + close(); + return Action.SUCCEEDED; } - }; - protected Runnable failTask = new Runnable() - { + @Override - public void run() + protected void onCompleteFailure(Throwable cause) { - failed(new Exception("testing failure")); + failureLatch.countDown(); } }; + + callback.iterate(); + + assertTrue(failureLatch.await(5, TimeUnit.SECONDS)); + } + + private abstract static class TestCB extends IteratingCallback + { + protected Runnable successTask = this::succeeded; + protected Runnable failTask = () -> failed(new Exception("testing failure")); protected CountDownLatch completed = new CountDownLatch(1); protected int processed = 0; @@ -320,8 +337,7 @@ public void onCompleteFailure(Throwable x) boolean waitForComplete() throws InterruptedException { - completed.await(10, TimeUnit.SECONDS); - return isSucceeded(); + return completed.await(10, TimeUnit.SECONDS) && isSucceeded(); } } @@ -390,57 +406,541 @@ protected void onCompleteFailure(Throwable cause) assertEquals(1, count.get()); - // Aborting should not iterate. icb.abort(new Exception()); assertTrue(ocfLatch.await(5, TimeUnit.SECONDS)); + assertTrue(icb.isFailed()); assertTrue(icb.isAborted()); assertEquals(1, count.get()); } @Test - public void testWhenProcessingAbortSerializesOnCompleteFailure() throws Exception + public void testWhenPendingAbortSerializesOnCompleteFailure() throws Exception { - AtomicInteger count = new AtomicInteger(); - CountDownLatch ocfLatch = new CountDownLatch(1); + AtomicReference aborted = new AtomicReference<>(); + CountDownLatch abortLatch = new CountDownLatch(1); + AtomicReference failure = new AtomicReference<>(); + AtomicMarkableReference completed = new AtomicMarkableReference<>(null, false); + IteratingCallback icb = new IteratingCallback() { @Override protected Action process() throws Throwable { - count.incrementAndGet(); - abort(new Exception()); - - // After calling abort, onCompleteFailure() must not be called yet. - assertFalse(ocfLatch.await(1, TimeUnit.SECONDS)); - return Action.SCHEDULED; } + @Override + protected void onAborted(Throwable cause) + { + aborted.set(cause); + ExceptionUtil.call(abortLatch::await, Throwable::printStackTrace); + } + @Override protected void onCompleteFailure(Throwable cause) { - ocfLatch.countDown(); + failure.set(cause); + } + + @Override + protected void onCompleted(Throwable causeOrNull) + { + completed.set(causeOrNull, true); + super.onCompleted(causeOrNull); } }; icb.iterate(); - assertEquals(1, count.get()); + assertThat(icb.toString(), containsString("[PENDING, false,")); - assertTrue(ocfLatch.await(5, TimeUnit.SECONDS)); - assertTrue(icb.isAborted()); + Throwable cause = new Throwable("test abort"); + new Thread(() -> icb.abort(cause)).start(); + + Awaitility.waitAtMost(5, TimeUnit.SECONDS).until(() -> icb.toString().contains("[PENDING, true,")); + Awaitility.waitAtMost(5, TimeUnit.SECONDS).until(() -> aborted.get() != null); - // Calling succeeded() won't cause further iterations. icb.succeeded(); - assertEquals(1, count.get()); + // We are now complete, but callbacks have not yet been done + assertThat(icb.toString(), containsString("[COMPLETE, true,")); + assertThat(failure.get(), nullValue()); + assertFalse(completed.isMarked()); + + abortLatch.countDown(); + Awaitility.waitAtMost(5, TimeUnit.SECONDS).until(completed::isMarked); + assertThat(failure.get(), sameInstance(cause)); + assertThat(completed.getReference(), sameInstance(cause)); + } + + public enum Event + { + PROCESSED, + ABORTED, + SUCCEEDED, + FAILED + } + + public static Stream> serializedEvents() + { + return Stream.of( + List.of(Event.PROCESSED, Event.ABORTED, Event.SUCCEEDED), + List.of(Event.PROCESSED, Event.SUCCEEDED, Event.ABORTED), + + List.of(Event.SUCCEEDED, Event.PROCESSED, Event.ABORTED), + List.of(Event.SUCCEEDED, Event.ABORTED, Event.PROCESSED), + + List.of(Event.ABORTED, Event.SUCCEEDED, Event.PROCESSED), + List.of(Event.ABORTED, Event.PROCESSED, Event.SUCCEEDED), + + List.of(Event.PROCESSED, Event.ABORTED, Event.FAILED), + List.of(Event.PROCESSED, Event.FAILED, Event.ABORTED), + + List.of(Event.FAILED, Event.PROCESSED, Event.ABORTED), + List.of(Event.FAILED, Event.ABORTED, Event.PROCESSED), + + List.of(Event.ABORTED, Event.FAILED, Event.PROCESSED), + List.of(Event.ABORTED, Event.PROCESSED, Event.FAILED) + ); + } + + @ParameterizedTest + @MethodSource("serializedEvents") + public void testSerializesProcessAbortCompletion(List events) throws Exception + { + AtomicReference aborted = new AtomicReference<>(); + CountDownLatch processingLatch = new CountDownLatch(1); + CountDownLatch abortLatch = new CountDownLatch(1); + AtomicReference failure = new AtomicReference<>(); + AtomicMarkableReference completed = new AtomicMarkableReference<>(null, false); + + + Throwable cause = new Throwable("test abort"); + + IteratingCallback icb = new IteratingCallback() + { + @Override + protected Action process() throws Throwable + { + abort(cause); + ExceptionUtil.call(processingLatch::await, Throwable::printStackTrace); + return Action.SCHEDULED; + } + + @Override + protected void onAborted(Throwable cause) + { + aborted.set(cause); + ExceptionUtil.call(abortLatch::await, Throwable::printStackTrace); + } + + @Override + protected void onCompleteFailure(Throwable cause) + { + failure.set(cause); + } + + @Override + protected void onCompleted(Throwable causeOrNull) + { + completed.set(causeOrNull, true); + super.onCompleted(causeOrNull); + } + }; + + new Thread(icb::iterate).start(); + + Awaitility.waitAtMost(5, TimeUnit.SECONDS).until(() -> icb.toString().contains("[PROCESSING, true,")); + + // we have aborted, but onAborted not yet called + assertThat(aborted.get(), nullValue()); + + int count = 0; + for (Event event : events) + { + switch (event) + { + case PROCESSED -> + { + processingLatch.countDown(); + // We can call aborted + Awaitility.waitAtMost(5, TimeUnit.SECONDS).pollInterval(10, TimeUnit.MILLISECONDS).until(() -> aborted.get() != null); + } + case ABORTED -> + { + abortLatch.countDown(); + Awaitility.waitAtMost(5, TimeUnit.SECONDS).pollInterval(10, TimeUnit.MILLISECONDS).until(() -> !icb.toString().contains("AbortingException")); + } + case SUCCEEDED -> icb.succeeded(); + + case FAILED -> icb.failed(new Throwable("failure")); + } + + if (++count < 3) + { + // Not complete yet + assertThat(failure.get(), nullValue()); + assertFalse(completed.isMarked()); + } + + // Extra aborts ignored + assertFalse(icb.abort(new Throwable("ignored"))); + } + + // When the callback is succeeded, the completion events can be called + Awaitility.waitAtMost(5, TimeUnit.SECONDS).pollInterval(10, TimeUnit.MILLISECONDS).until(completed::isMarked); + assertThat(failure.get(), sameInstance(cause)); + assertThat(completed.getReference(), sameInstance(cause)); + } + + @Test + public void testICBSuccess() throws Exception + { + TestIteratingCB callback = new TestIteratingCB(); + callback.iterate(); + callback.succeeded(); + assertTrue(callback._completed.await(1, TimeUnit.SECONDS)); + assertThat(callback._onFailure.get(), nullValue()); + assertThat(callback._completion.getReference(), Matchers.nullValue()); + assertTrue(callback._completion.isMarked()); + + // Everything now a noop + assertFalse(callback.abort(new Throwable())); + callback.failed(new Throwable()); + assertThat(callback._completion.getReference(), Matchers.nullValue()); + assertThat(callback._completed.getCount(), is(0L)); + + callback.checkNoBadCalls(); + } + + @Test + public void testICBFailure() throws Exception + { + Throwable failure = new Throwable(); + TestIteratingCB callback = new TestIteratingCB(); + callback.iterate(); + callback.failed(failure); + assertTrue(callback._completed.await(1, TimeUnit.SECONDS)); + assertThat(callback._onFailure.get(), sameInstance(failure)); + assertThat(callback._completion.getReference(), Matchers.sameInstance(failure)); + assertTrue(callback._completion.isMarked()); + + // Everything now a noop, other than suppression + callback.succeeded(); + Throwable late = new Throwable(); + assertFalse(callback.abort(late)); + assertFalse(ExceptionUtil.areNotAssociated(failure, late)); + assertThat(callback._completion.getReference(), Matchers.sameInstance(failure)); + assertThat(callback._completed.getCount(), is(0L)); + + callback.checkNoBadCalls(); + } + + @Test + public void testICBAbortSuccess() throws Exception + { + TestIteratingCB callback = new TestIteratingCB(); + callback.iterate(); + + Throwable abort = new Throwable(); + callback.abort(abort); + assertFalse(callback._completed.await(100, TimeUnit.MILLISECONDS)); + assertThat(callback._onFailure.get(), sameInstance(abort)); + assertThat(callback._completion.getReference(), Matchers.sameInstance(abort)); + assertFalse(callback._completion.isMarked()); + + callback.succeeded(); + assertThat(callback._completion.getReference(), Matchers.sameInstance(abort)); + assertThat(callback._completed.getCount(), is(0L)); + + Throwable late = new Throwable(); + callback.failed(late); + assertFalse(callback.abort(late)); + assertTrue(ExceptionUtil.areAssociated(abort, late)); + assertTrue(ExceptionUtil.areAssociated(callback._onFailure.get(), late)); + assertThat(callback._completion.getReference(), Matchers.sameInstance(abort)); + assertThat(callback._completed.getCount(), is(0L)); + + callback.checkNoBadCalls(); + } + + public static Stream abortTests() + { + List tests = new ArrayList<>(); + + for (IteratingCallback.State state : IteratingCallback.State.values()) + { + String name = state.name(); + + if (name.contains("PROCESSING")) + { + for (IteratingCallback.Action action : IteratingCallback.Action.values()) + { + if (name.contains("CALLED")) + { + if (action == IteratingCallback.Action.SCHEDULED) + { + tests.add(Arguments.of(name, action.toString(), Boolean.TRUE)); + tests.add(Arguments.of(name, action.toString(), Boolean.FALSE)); + } + } + else if (action == IteratingCallback.Action.SCHEDULED) + { + tests.add(Arguments.of(name, action.toString(), Boolean.TRUE)); + tests.add(Arguments.of(name, action.toString(), Boolean.FALSE)); + } + else + { + tests.add(Arguments.of(name, action.toString(), null)); + } + } + } + else if (name.equals("COMPLETE") || name.contains("PENDING")) + { + tests.add(Arguments.of(name, null, Boolean.TRUE)); + tests.add(Arguments.of(name, null, Boolean.FALSE)); + } + else + { + tests.add(Arguments.of(name, null, null)); + } + } + + return tests.stream(); + } + + @ParameterizedTest + @MethodSource("abortTests") + public void testAbortInEveryState(String state, String action, Boolean success) throws Exception + { + CountDownLatch processLatch = new CountDownLatch(1); + + AtomicReference onAbort = new AtomicReference<>(); + AtomicReference onFailure = new AtomicReference<>(null); + AtomicMarkableReference onCompleted = new AtomicMarkableReference<>(null, false); + + Throwable cause = new Throwable("abort"); + Throwable failure = new Throwable("failure"); + AtomicInteger badCalls = new AtomicInteger(0); + + IteratingCallback callback = new IteratingCallback() + { + @Override + protected Action process() throws Throwable + { + if (state.contains("CALLED")) + { + if (success) + succeeded(); + else + failed(failure); + } + + if (state.contains("PENDING")) + return Action.SCHEDULED; + + if (state.equals("COMPLETE")) + { + if (success) + return Action.SUCCEEDED; + failed(new Throwable("Complete Failure")); + return Action.SCHEDULED; + } + + if (state.equals("CLOSED")) + { + close(); + return Action.SUCCEEDED; + } + + processLatch.await(); + return IteratingCallback.Action.valueOf(action); + } + + @Override + protected void onFailure(Throwable cause) + { + if (!onFailure.compareAndSet(null, cause)) + badCalls.incrementAndGet(); + } + + @Override + protected void onAborted(Throwable cause) + { + if (!onAbort.compareAndSet(null, cause)) + badCalls.incrementAndGet(); + } + + @Override + protected void onCompleted(Throwable causeOrNull) + { + onCompleted.set(causeOrNull, true); + super.onCompleted(causeOrNull); + } + }; + + if (!state.equals("IDLE")) + { + new Thread(callback::iterate).start(); + } + + Awaitility.waitAtMost(5, TimeUnit.SECONDS).pollInterval(10, TimeUnit.MILLISECONDS).until(() -> callback.toString().contains(state)); + assertThat(callback.toString(), containsString("[" + state + ",")); + onAbort.set(null); + + if (success == Boolean.FALSE && (state.equals("COMPLETE") || state.equals("CLOSED"))) + { + // We must be failed already + assertThat(onFailure.get(), notNullValue()); + } + + boolean aborted = callback.abort(cause); + + // Check abort in completed state + if (state.equals("COMPLETE") || state.equals("CLOSED")) + { + assertThat(aborted, is(false)); + assertThat(onAbort.get(), nullValue()); + assertTrue(onCompleted.isMarked()); + if (success == Boolean.TRUE) + assertThat(onCompleted.getReference(), nullValue()); + else + assertThat(onCompleted.getReference(), notNullValue()); + return; + } + + // Check abort in non completed state + assertThat(aborted, is(true)); + + if (state.contains("PROCESSING")) + { + processLatch.countDown(); + + Awaitility.waitAtMost(5, TimeUnit.SECONDS).pollInterval(10, TimeUnit.MILLISECONDS).until(() -> !callback.toString().contains("PROCESSING")); + + if (action.equals("SCHEDULED")) + { + if (success) + { + callback.succeeded(); + } + else + { + Throwable failureAfterAbort = new Throwable("failure after abort"); + callback.failed(failureAfterAbort); + assertThat(onFailure.get(), not(sameInstance(failureAfterAbort))); + assertTrue(ExceptionUtil.areAssociated(onFailure.get(), failureAfterAbort)); + } + } + } + else if (state.contains("PENDING")) + { + if (success) + callback.succeeded(); + else + callback.failed(new Throwable("failure after abort")); + } + + assertTrue(onCompleted.isMarked()); + + if (state.contains("CALLED") && !success) + { + assertThat(onCompleted.getReference(), sameInstance(failure)); + assertThat(onAbort.get(), sameInstance(failure)); + } + else + { + assertThat(onCompleted.getReference(), sameInstance(cause)); + assertThat(onAbort.get(), sameInstance(cause)); + } + + assertThat(badCalls.get(), is(0)); + } + + private static class TestIteratingCB extends IteratingCallback + { + final AtomicInteger _count; + final AtomicInteger _badCalls = new AtomicInteger(0); + final AtomicBoolean _onSuccess = new AtomicBoolean(); + final AtomicReference _onFailure = new AtomicReference<>(); + final AtomicMarkableReference _completion = new AtomicMarkableReference<>(null, false); + final CountDownLatch _completed = new CountDownLatch(2); + + private TestIteratingCB() + { + this(1); + } + + private TestIteratingCB(int count) + { + _count = new AtomicInteger(count); + } + + @Override + protected Action process() + { + return _count.getAndDecrement() == 0 ? Action.SUCCEEDED : Action.SCHEDULED; + } + + @Override + protected void onAborted(Throwable cause) + { + _completion.compareAndSet(null, cause, false, false); + } + + @Override + protected void onSuccess() + { + if (!_onSuccess.compareAndSet(false, true)) + _badCalls.incrementAndGet(); + } + + @Override + protected void onFailure(Throwable cause) + { + if (!_onFailure.compareAndSet(null, cause)) + _badCalls.incrementAndGet(); + } + + @Override + protected void onCompleteFailure(Throwable cause) + { + if (_completion.compareAndSet(null, cause, false, true)) + _completed.countDown(); + + Throwable failure = _completion.getReference(); + if (failure != null && _completion.compareAndSet(failure, failure, false, true)) + _completed.countDown(); + } + + @Override + protected void onCompleteSuccess() + { + if (_completion.compareAndSet(null, null, false, true)) + _completed.countDown(); + } + + @Override + protected void onCompleted(Throwable causeOrNull) + { + if (_completion.isMarked()) + _badCalls.incrementAndGet(); + super.onCompleted(causeOrNull); + _completed.countDown(); + } + + public void checkNoBadCalls() + { + assertThat(_badCalls.get(), is(0)); + } } @Test public void testOnSuccessCalledDespiteISE() throws Exception { CountDownLatch latch = new CountDownLatch(1); + AtomicReference aborted = new AtomicReference<>(); IteratingCallback icb = new IteratingCallback() { @Override @@ -451,13 +951,22 @@ protected Action process() } @Override - protected void onSuccess() + protected void onAborted(Throwable cause) + { + aborted.set(cause); + super.onAborted(cause); + } + + @Override + protected void onCompleted(Throwable causeOrNull) { + super.onCompleted(causeOrNull); latch.countDown(); } }; - assertThrows(IllegalStateException.class, icb::iterate); + icb.iterate(); assertTrue(latch.await(5, TimeUnit.SECONDS)); + assertThat(aborted.get(), instanceOf(IllegalStateException.class)); } } diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/WebSocketConnection.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/WebSocketConnection.java index 698507ece9db..095c01071a47 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/WebSocketConnection.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/WebSocketConnection.java @@ -635,10 +635,10 @@ private Flusher(Scheduler scheduler, int bufferSize, Generator generator, EndPoi } @Override - public void onCompleteFailure(Throwable x) + public void onFailure(Throwable x) { coreSession.processConnectionError(x, NOOP); - super.onCompleteFailure(x); + super.onFailure(x); } } } diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java index 3aee03b73b0c..876ec8ebb1bd 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java @@ -405,7 +405,7 @@ public void timeoutExpired() } @Override - public void onCompleteFailure(Throwable failure) + public void onFailure(Throwable failure) { if (batchBuffer != null) batchBuffer.clear(); diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java index 5e8d65573f41..54b7ab3d1393 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java @@ -494,10 +494,10 @@ private boolean inflate(Frame frame, Callback callback, boolean first) throws Da } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { releasePayload(_payloadRef); - super.onCompleteFailure(cause); + super.onFailure(cause); } private void releasePayload(AtomicReference reference) diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/DemandingFlusher.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/DemandingFlusher.java index 41e123b0f165..9a8a2afff7d0 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/DemandingFlusher.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/DemandingFlusher.java @@ -178,7 +178,7 @@ protected Action process() throws Throwable } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { Throwable suppressed = _failure.getAndSet(cause); if (suppressed != null && suppressed != cause) diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java index edd5ee805ecd..51cc47495461 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java @@ -170,7 +170,7 @@ protected Action process() throws Throwable } @Override - protected void onCompleteFailure(Throwable t) + protected void onFailure(Throwable t) { if (log.isDebugEnabled()) log.debug("onCompleteFailure {}", t.toString()); @@ -180,7 +180,7 @@ protected void onCompleteFailure(Throwable t) notifyCallbackFailure(current.callback, t); current = null; } - onFailure(t); + this.onFailure(t); } } diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/internal/FrameFlusherTest.java b/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/internal/FrameFlusherTest.java index 2ae247437e02..7ba03da9e066 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/internal/FrameFlusherTest.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-tests/src/test/java/org/eclipse/jetty/websocket/core/internal/FrameFlusherTest.java @@ -166,11 +166,11 @@ public void testWriteTimeout() throws Exception FrameFlusher frameFlusher = new FrameFlusher(bufferPool, scheduler, generator, endPoint, bufferSize, maxGather) { @Override - public void onCompleteFailure(Throwable failure) + public void onFailure(Throwable failure) { error.set(failure); flusherFailure.countDown(); - super.onCompleteFailure(failure); + super.onFailure(failure); } }; diff --git a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java index b464b5a325d0..a4a2f459ebda 100644 --- a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java +++ b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java @@ -400,7 +400,7 @@ private void process(ByteBuffer content, Callback callback, boolean finished) th } @Override - protected void onCompleteFailure(Throwable x) + protected void onFailure(Throwable x) { onError(x); } diff --git a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncProxyServlet.java b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncProxyServlet.java index 11e048ac8236..df91f4f27305 100644 --- a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncProxyServlet.java +++ b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncProxyServlet.java @@ -192,7 +192,7 @@ protected void onRequestContent(HttpServletRequest request, Request proxyRequest } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { onError(cause); } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java index 498079c1c042..ae49f9343d5a 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java @@ -1425,7 +1425,7 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable e) + public void onFailure(Throwable e) { onWriteComplete(_last, e); } @@ -1461,11 +1461,11 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable e) + public void onFailure(Throwable e) { try { - super.onCompleteFailure(e); + super.onFailure(e); } catch (Throwable t) { @@ -1666,11 +1666,11 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable x) + public void onFailure(Throwable x) { _buffer.release(); IO.close(_in); - super.onCompleteFailure(x); + super.onFailure(x); } } @@ -1739,11 +1739,11 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable x) + public void onFailure(Throwable x) { _buffer.release(); IO.close(_in); - super.onCompleteFailure(x); + super.onFailure(x); } } diff --git a/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncMiddleManServlet.java b/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncMiddleManServlet.java index bb902313989e..5110d88b3f01 100644 --- a/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncMiddleManServlet.java +++ b/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncMiddleManServlet.java @@ -400,7 +400,7 @@ private void process(ByteBuffer content, Callback callback, boolean finished) th } @Override - protected void onCompleteFailure(Throwable x) + protected void onFailure(Throwable x) { onError(x); } diff --git a/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java b/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java index 557b17939fe3..d28444d8c7c4 100644 --- a/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java +++ b/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java @@ -1427,7 +1427,7 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable e) + public void onFailure(Throwable e) { onWriteComplete(_last, e); } @@ -1463,11 +1463,11 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable e) + public void onFailure(Throwable e) { try { - super.onCompleteFailure(e); + super.onFailure(e); } catch (Throwable t) { @@ -1668,11 +1668,11 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable x) + public void onFailure(Throwable x) { _buffer.release(); IO.close(_in); - super.onCompleteFailure(x); + super.onFailure(x); } } @@ -1741,11 +1741,11 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable x) + public void onFailure(Throwable x) { _buffer.release(); IO.close(_in); - super.onCompleteFailure(x); + super.onFailure(x); } } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandler.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandler.java index 0eb943a384b8..efec2644bdf7 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandler.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/FileBufferedResponseHandler.java @@ -221,7 +221,7 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { dispose(); callback.failed(cause); diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java index f85fba691612..02902d780da1 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java @@ -1612,7 +1612,7 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable e) + public void onFailure(Throwable e) { onWriteComplete(_last, e); } @@ -1648,11 +1648,11 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable e) + public void onFailure(Throwable e) { try { - super.onCompleteFailure(e); + super.onFailure(e); } catch (Throwable t) { @@ -1852,11 +1852,11 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable x) + public void onFailure(Throwable x) { _buffer.release(); IO.close(_in); - super.onCompleteFailure(x); + super.onFailure(x); } } @@ -1923,11 +1923,11 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(Throwable x) + public void onFailure(Throwable x) { _buffer.release(); IO.close(_in); - super.onCompleteFailure(x); + super.onFailure(x); } } diff --git a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServlet.java b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServlet.java index f8d678276a62..a02c716b8623 100644 --- a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServlet.java +++ b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServlet.java @@ -400,7 +400,7 @@ private void process(ByteBuffer content, Callback callback, boolean finished) th } @Override - protected void onCompleteFailure(Throwable x) + protected void onFailure(Throwable x) { onError(x); } diff --git a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncProxyServlet.java b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncProxyServlet.java index 321da55fd150..6c90f26e7cdf 100644 --- a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncProxyServlet.java +++ b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncProxyServlet.java @@ -192,7 +192,7 @@ protected void onRequestContent(HttpServletRequest request, Request proxyRequest } @Override - protected void onCompleteFailure(Throwable cause) + protected void onFailure(Throwable cause) { onError(cause); } From 2b60a6d3af5a8a63300f651ab96e9e411eb4e6d9 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 16 Jul 2024 11:08:48 +1000 Subject: [PATCH 02/22] Experiment with IteratingCallback The previous semantic of `onCompleteFailure` has been renamed to `onFailure(Throwable)`, which is called immediately (but serialized) on either an abort or a failure. A new `onCompleteFailure(Throwable)` method has been added that is called only after a `failed(throwable)` or a `abort(Throwable)` followed by `succeeded()` or `failed(Throwable)`` No usage has yet been made of the new `onCompleteFailure`, but the ICB implementation has been completely replaced by the one developed in #11876 --- .../java/org/eclipse/jetty/util/IteratingCallback.java | 7 +------ .../org/eclipse/jetty/ee11/proxy/AsyncProxyServlet.java | 4 ++-- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java index 5f4169f4b720..91a8f2774977 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java @@ -44,18 +44,13 @@ *

* Subclasses must implement method {@link #process()} where the * asynchronous sub-task is initiated and a suitable {@link Action} - * is returned to this callback to indicate the overall progress ofk + * is returned to this callback to indicate the overall progress of * the large asynchronous task. * This callback is passed to the asynchronous sub-task, and a call * to {@link #succeeded()} on this callback represents the successful * completion of the asynchronous sub-task, while a call to * {@link #failed(Throwable)} on this callback represents the * completion with a failure of the large asynchronous task. - *

- * For most purposes, the {@link #succeeded()} and {@link #failed(Throwable)} - * methods of this class should be considered final, and only overridden in - * extraordinary circumstances. Any action taken in such extensions are not - * serialized. */ public abstract class IteratingCallback implements Callback { diff --git a/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncProxyServlet.java b/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncProxyServlet.java index 00350bc2491a..6d8508224588 100644 --- a/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncProxyServlet.java +++ b/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncProxyServlet.java @@ -192,9 +192,9 @@ protected void onRequestContent(HttpServletRequest request, Request proxyRequest } @Override - public void failed(Throwable x) + public void onFailure(Throwable x) { - super.failed(x); + super.onFailure(x); onError(x); } } From c2742ccb4a6d8e5f473f8db910d07d251dd7fb7f Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 16 Jul 2024 12:01:08 +1000 Subject: [PATCH 03/22] Experiment with IteratingCallback The previous semantic of `onCompleteFailure` has been renamed to `onFailure(Throwable)`, which is called immediately (but serialized) on either an abort or a failure. A new `onCompleteFailure(Throwable)` method has been added that is called only after a `failed(throwable)` or a `abort(Throwable)` followed by `succeeded()` or `failed(Throwable)`` No usage has yet been made of the new `onCompleteFailure`, but the ICB implementation has been completely replaced by the one developed in #11876 --- .../eclipse/jetty/util/IteratingCallback.java | 162 ++++++++++-------- 1 file changed, 91 insertions(+), 71 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java index 91a8f2774977..a08bfdb2c33d 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java @@ -247,14 +247,57 @@ protected void onCompleted(Throwable causeOrNull) onCompleteFailure(causeOrNull); } + private void doOnSuccessProcessing() + { + ExceptionUtil.callAndThen(this::onSuccess, this::processing); + } + private void doCompleteSuccess() { onCompleted(null); } - private void doCompleteFailure(Throwable cause) + private void doOnCompleted(Throwable cause) + { + ExceptionUtil.call(cause, this::onCompleted); + } + + private void doOnFailureOnCompleted(Throwable cause) + { + ExceptionUtil.callAndThen(cause, this::onFailure, this::onCompleted); + } + + private void doOnAbortedOnFailure(Throwable cause) { - onCompleted(cause); + ExceptionUtil.callAndThen(cause, this::onAborted, this::onFailure); + } + + private void doOnAbortedOnFailureOnCompleted(Throwable cause) + { + ExceptionUtil.callAndThen(cause, this::doOnAbortedOnFailure, this::onCompleted); + } + + private void doOnAbortedOnFailureIfNotPendingDoCompleted(Throwable cause) + { + ExceptionUtil.callAndThen(cause, this::doOnAbortedOnFailure, this::ifNotPendingDoCompleted); + } + + private void ifNotPendingDoCompleted() + { + Throwable completeFailure = null; + try (AutoLock ignored = _lock.lock()) + { + _failure = _failure.getCause(); + + if (Objects.requireNonNull(_state) != State.PENDING) + { + // the callback completed, one way or another, so it is up to us to do the completion + completeFailure = _failure; + } + } + + if (completeFailure != null) + doOnCompleted(completeFailure); } /** @@ -298,9 +341,9 @@ private void processing() // may happen concurrently, so state is not assumed. boolean completeSuccess = false; - Throwable abortDoCompleteFailure = null; - Throwable completeFailure = null; - Throwable onAbort = null; + Throwable onAbortedOnFailureOnCompleted = null; + Throwable onFailureOnCompleted = null; + Throwable onAbortedOnFailureIfNotPendingDoCompleted = null; // While we are processing processing: @@ -339,7 +382,7 @@ private void processing() if (_aborted) { _state = _failure instanceof ClosedException ? State.CLOSED : State.COMPLETE; - abortDoCompleteFailure = _failure; + onAbortedOnFailureOnCompleted = _failure; break processing; } @@ -361,8 +404,8 @@ private void processing() _state = State.PENDING; if (_aborted) { - onAbort = _failure; - _failure = new AbortingException(onAbort); + onAbortedOnFailureIfNotPendingDoCompleted = _failure; + _failure = new AbortingException(onAbortedOnFailureIfNotPendingDoCompleted); } break processing; } @@ -373,7 +416,7 @@ private void processing() if (_aborted) { _state = _failure instanceof ClosedException ? State.CLOSED : State.COMPLETE; - abortDoCompleteFailure = _failure; + onAbortedOnFailureOnCompleted = _failure; } else { @@ -395,24 +438,24 @@ private void processing() if (action != Action.SCHEDULED && action != null) { _state = State.CLOSED; - abortDoCompleteFailure = new IllegalStateException("Action not scheduled"); + onAbortedOnFailureOnCompleted = new IllegalStateException("Action not scheduled"); if (_failure == null) { - _failure = abortDoCompleteFailure; + _failure = onAbortedOnFailureOnCompleted; } else { - ExceptionUtil.addSuppressedIfNotAssociated(_failure, onAbort); - abortDoCompleteFailure = _failure; + ExceptionUtil.addSuppressedIfNotAssociated(_failure, onAbortedOnFailureIfNotPendingDoCompleted); + onAbortedOnFailureOnCompleted = _failure; } break processing; } if (_failure != null) { if (_aborted) - abortDoCompleteFailure = _failure; + onAbortedOnFailureOnCompleted = _failure; else - completeFailure = _failure; + onFailureOnCompleted = _failure; _state = _failure instanceof ClosedException ? State.CLOSED : State.COMPLETE; break processing; } @@ -431,14 +474,14 @@ private void processing() onSuccess(); } } - if (abortDoCompleteFailure != null) - ExceptionUtil.callAndThen(abortDoCompleteFailure, this::doOnAbortedOnFailure, this::doCompleteFailure); + if (onAbortedOnFailureOnCompleted != null) + doOnAbortedOnFailureOnCompleted(onAbortedOnFailureOnCompleted); else if (completeSuccess) doCompleteSuccess(); - else if (completeFailure != null) - ExceptionUtil.callAndThen(completeFailure, this::onFailure, this::doCompleteFailure); - else if (onAbort != null) - ExceptionUtil.callAndThen(onAbort, this::doOnAbortedOnFailure, this::doAbortPendingCompletion); + else if (onFailureOnCompleted != null) + doOnFailureOnCompleted(onFailureOnCompleted); + else if (onAbortedOnFailureIfNotPendingDoCompleted != null) + doOnAbortedOnFailureIfNotPendingDoCompleted(onAbortedOnFailureIfNotPendingDoCompleted); } /** @@ -457,8 +500,8 @@ else if (onAbort != null) @Override public final void succeeded() { - boolean process = false; - Throwable completeFailure = null; + boolean onSuccessProcessing = false; + Throwable onCompleted = null; try (AutoLock ignored = _lock.lock()) { if (LOG.isDebugEnabled()) @@ -484,14 +527,14 @@ public final void succeeded() { // The onAborted call is complete, so we must do the completion _state = _failure instanceof ClosedException ? State.CLOSED : State.COMPLETE; - completeFailure = _failure; + onCompleted = _failure; } } else { // No other thread is processing, so we will do the processing _state = State.PROCESSING; - process = true; + onSuccessProcessing = true; } break; } @@ -506,13 +549,13 @@ public final void succeeded() } } } - if (process) + if (onSuccessProcessing) { - ExceptionUtil.callAndThen(this::onSuccess, this::processing); + doOnSuccessProcessing(); } - else if (completeFailure != null) + else if (onCompleted != null) { - doCompleteFailure(completeFailure); + doOnCompleted(onCompleted); } } @@ -538,8 +581,8 @@ public final void failed(Throwable cause) { cause = Objects.requireNonNullElseGet(cause, IOException::new); - Throwable completeFailure = null; - Throwable abortCompletion = null; + Throwable onFailureOnCompleted = null; + Throwable onCompleted = null; try (AutoLock ignored = _lock.lock()) { if (LOG.isDebugEnabled()) @@ -571,7 +614,7 @@ public final void failed(Throwable cause) // The onAborted call is complete, so we must do the completion ExceptionUtil.addSuppressedIfNotAssociated(_failure, cause); _state = _failure instanceof ClosedException ? State.CLOSED : State.COMPLETE; - abortCompletion = _failure; + onCompleted = _failure; } } else @@ -579,7 +622,7 @@ public final void failed(Throwable cause) // No other thread is processing, so we will do the processing _state = State.COMPLETE; _failure = cause; - completeFailure = _failure; + onFailureOnCompleted = _failure; } break; } @@ -595,10 +638,10 @@ public final void failed(Throwable cause) } } } - if (completeFailure != null) - ExceptionUtil.callAndThen(completeFailure, this::onFailure, this::doCompleteFailure); - else if (abortCompletion != null) - doCompleteFailure(abortCompletion); + if (onFailureOnCompleted != null) + doOnFailureOnCompleted(onFailureOnCompleted); + else if (onCompleted != null) + doOnCompleted(onCompleted); } /** @@ -612,8 +655,8 @@ else if (abortCompletion != null) */ public final void close() { - Throwable onAbort = null; - Throwable onAbortDoCompleteFailure = null; + Throwable onAbortedOnFailureIfNotPendingDoCompleted = null; + Throwable onAbortOnFailureOnCompleted = null; try (AutoLock ignored = _lock.lock()) { @@ -626,7 +669,7 @@ public final void close() // Nothing happening so we can abort and complete _state = State.CLOSED; _failure = new ClosedException(); - onAbortDoCompleteFailure = _failure; + onAbortOnFailureOnCompleted = _failure; } case PROCESSING, PROCESSING_CALLED -> { @@ -645,8 +688,8 @@ public final void close() case PENDING -> { // We are waiting for the callback, so we can only call onAbort and then keep waiting - onAbort = new ClosedException(); - _failure = new AbortingException(onAbort); + onAbortedOnFailureIfNotPendingDoCompleted = new ClosedException(); + _failure = new AbortingException(onAbortedOnFailureIfNotPendingDoCompleted); _aborted = true; } @@ -663,10 +706,10 @@ public final void close() } } - if (onAbort != null) - ExceptionUtil.callAndThen(onAbort, this::doOnAbortedOnFailure, this::doAbortPendingCompletion); - else if (onAbortDoCompleteFailure != null) - ExceptionUtil.callAndThen(onAbortDoCompleteFailure, this::doOnAbortedOnFailure, this::doCompleteFailure); + if (onAbortedOnFailureIfNotPendingDoCompleted != null) + doOnAbortedOnFailureIfNotPendingDoCompleted(onAbortedOnFailureIfNotPendingDoCompleted); + else if (onAbortOnFailureOnCompleted != null) + doOnAbortedOnFailureOnCompleted(onAbortOnFailureOnCompleted); } /** @@ -747,36 +790,13 @@ public final boolean abort(Throwable cause) } if (onAbortDoCompleteFailure) - ExceptionUtil.callAndThen(cause, this::doOnAbortedOnFailure, this::doCompleteFailure); + doOnAbortedOnFailureOnCompleted(cause); else if (onAbort) - ExceptionUtil.callAndThen(cause, this::doOnAbortedOnFailure, this::doAbortPendingCompletion); + doOnAbortedOnFailureIfNotPendingDoCompleted(cause); return true; } - private void doOnAbortedOnFailure(Throwable cause) - { - ExceptionUtil.callAndThen(cause, this::onAborted, this::onFailure); - } - - private void doAbortPendingCompletion() - { - Throwable doCompleteFailure = null; - try (AutoLock ignored = _lock.lock()) - { - _failure = _failure.getCause(); - - if (Objects.requireNonNull(_state) != State.PENDING) - { - // the callback completed, one way or another, so it is up to use to do the completion - doCompleteFailure = _failure; - } - } - - if (doCompleteFailure != null) - ExceptionUtil.call(doCompleteFailure, this::doCompleteFailure); - } - /** * @return whether this callback is idle, and {@link #iterate()} needs to be called */ From 4c28464c0bba3b3b2cbe39fae0f59a074473ac09 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 17 Jul 2024 09:33:57 +1000 Subject: [PATCH 04/22] Reduce capture of this:: on hot path --- .../org/eclipse/jetty/util/IteratingCallback.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java index a08bfdb2c33d..bc0328edb1b1 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java @@ -15,6 +15,7 @@ import java.io.IOException; import java.util.Objects; +import java.util.function.Consumer; import org.eclipse.jetty.util.thread.AutoLock; import org.slf4j.Logger; @@ -129,6 +130,9 @@ protected enum Action } private final AutoLock _lock = new AutoLock(); + private final Runnable _onSuccess = this::onSuccess; + private final Runnable _processing = this::processing; + private final Consumer _onCompleted = this::onCompleted; private State _state; private Throwable _failure; private boolean _reprocess; @@ -249,7 +253,7 @@ protected void onCompleted(Throwable causeOrNull) private void doOnSuccessProcessing() { - ExceptionUtil.callAndThen(this::onSuccess, this::processing); + ExceptionUtil.callAndThen(_onSuccess, _processing); } private void doCompleteSuccess() @@ -259,12 +263,12 @@ private void doCompleteSuccess() private void doOnCompleted(Throwable cause) { - ExceptionUtil.call(cause, this::onCompleted); + ExceptionUtil.call(cause, _onCompleted); } private void doOnFailureOnCompleted(Throwable cause) { - ExceptionUtil.callAndThen(cause, this::onFailure, this::onCompleted); + ExceptionUtil.callAndThen(cause, this::onFailure, _onCompleted); } private void doOnAbortedOnFailure(Throwable cause) @@ -274,7 +278,7 @@ private void doOnAbortedOnFailure(Throwable cause) private void doOnAbortedOnFailureOnCompleted(Throwable cause) { - ExceptionUtil.callAndThen(cause, this::doOnAbortedOnFailure, this::onCompleted); + ExceptionUtil.callAndThen(cause, this::doOnAbortedOnFailure, _onCompleted); } private void doOnAbortedOnFailureIfNotPendingDoCompleted(Throwable cause) From a6fa5bfcb52e3efa220a70cb621cf3b54515efaa Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 19 Jul 2024 16:04:25 +0200 Subject: [PATCH 05/22] Split the release operations in case of failures. Signed-off-by: Simone Bordet --- .../jetty/docs/programming/ContentDocs.java | 10 +++-- .../jetty/client/transport/HttpSender.java | 13 ++++--- .../internal/HttpSenderOverHTTP.java | 15 ++++++-- .../eclipse/jetty/fcgi/generator/Flusher.java | 27 +++++++++---- .../jetty/http2/internal/HTTP2Flusher.java | 8 +++- .../eclipse/jetty/http3/ControlFlusher.java | 8 +++- .../jetty/http3/InstructionFlusher.java | 8 +++- .../eclipse/jetty/http3/MessageFlusher.java | 8 +++- .../org/eclipse/jetty/io/IOResources.java | 10 ++++- .../jetty/io/internal/ContentCopier.java | 27 ++++++++----- .../jetty/quic/common/QuicSession.java | 12 +++--- .../jetty/server/handler/ConnectHandler.java | 7 +++- .../handler/gzip/GzipResponseAndCallback.java | 22 +++++------ .../jetty/server/internal/HttpConnection.java | 21 ++++++++-- .../eclipse/jetty/util/IteratingCallback.java | 14 +++---- .../jetty/util/IteratingCallbackTest.java | 1 - .../websocket/core/internal/FrameFlusher.java | 19 +++++++--- .../internal/PerMessageDeflateExtension.java | 3 +- .../core/util/TransformingFlusher.java | 2 +- .../jetty/ee10/servlet/HttpOutput.java | 38 +++++++++++++------ .../jetty/ee11/proxy/AsyncProxyServlet.java | 1 - .../jetty/ee11/servlet/HttpOutput.java | 38 +++++++++++++------ .../eclipse/jetty/ee9/nested/HttpOutput.java | 38 +++++++++++++------ 23 files changed, 240 insertions(+), 110 deletions(-) diff --git a/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java b/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java index 4b932825b6d7..3bf3e415c556 100644 --- a/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java +++ b/documentation/jetty-documentation/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java @@ -362,13 +362,17 @@ protected void onCompleteSuccess() @Override protected void onFailure(Throwable failure) + { + // The copy is failed, fail the callback. + callback.failed(failure); + } + + @Override + protected void onCompleteFailure(Throwable cause) { // In case of a failure, either on the // read or on the write, release the chunk. chunk.release(); - - // The copy is failed, fail the callback. - callback.failed(failure); } @Override diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java index 3b505886ebe1..359c1ff3ab3c 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java @@ -593,12 +593,6 @@ else if (expect100) @Override protected void onFailure(Throwable x) { - if (chunk != null) - { - chunk.release(); - chunk = Content.Chunk.next(chunk); - } - failRequest(x); internalAbort(exchange, x); @@ -607,6 +601,13 @@ protected void onFailure(Throwable x) promise.succeeded(true); } + @Override + protected void onCompleteFailure(Throwable x) + { + if (chunk != null) + chunk.release(); + } + @Override public InvocationType getInvocationType() { diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java index cdc0b150df65..bfb5d9704421 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java @@ -250,11 +250,15 @@ protected void onCompleteSuccess() @Override protected void onFailure(Throwable cause) { - super.onFailure(cause); - release(); callback.failed(cause); } + @Override + protected void onCompleteFailure(Throwable cause) + { + release(); + } + private void release() { if (headerBuffer != null) @@ -337,10 +341,15 @@ protected Action process() throws Exception @Override protected void onFailure(Throwable cause) { - release(); callback.failed(cause); } + @Override + protected void onCompleteFailure(Throwable cause) + { + release(); + } + private void release() { if (chunkBuffer != null) diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java index f5431cf9061d..06447dcc10c0 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java @@ -15,6 +15,7 @@ import java.nio.ByteBuffer; import java.util.ArrayDeque; +import java.util.ArrayList; import java.util.List; import java.util.Queue; @@ -115,32 +116,44 @@ public void onFailure(Throwable x) active.failed(x); active = null; + List entries; + try (AutoLock ignored = lock.lock()) + { + entries = new ArrayList<>(queue); + } + entries.forEach(entry -> entry.failed(x)); + } + + @Override + protected void onCompleteFailure(Throwable cause) + { while (true) { Entry entry = poll(); if (entry == null) break; - entry.failed(x); + entry.release(); } } } - private record Entry(ByteBufferPool.Accumulator accumulator, Callback callback) implements Callback + private record Entry(ByteBufferPool.Accumulator accumulator, Callback callback) { - @Override public void succeeded() { - if (accumulator != null) - accumulator.release(); + release(); callback.succeeded(); } - @Override public void failed(Throwable x) + { + callback.failed(x); + } + + private void release() { if (accumulator != null) accumulator.release(); - callback.failed(x); } } } diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java index 68f9521459ec..8ef2d83a6cb0 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java @@ -346,8 +346,6 @@ protected void onCompleteSuccess() @Override protected void onFailure(Throwable x) { - release(); - Throwable closed; Set allEntries; try (AutoLock ignored = lock.lock()) @@ -376,6 +374,12 @@ protected void onFailure(Throwable x) session.onWriteFailure(x); } + @Override + protected void onCompleteFailure(Throwable x) + { + release(); + } + public void terminate(Throwable cause) { Throwable closed; diff --git a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/ControlFlusher.java b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/ControlFlusher.java index 7f4cb53c7db2..00c3a4ff0711 100644 --- a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/ControlFlusher.java +++ b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/ControlFlusher.java @@ -127,8 +127,6 @@ protected void onFailure(Throwable failure) if (LOG.isDebugEnabled()) LOG.debug("failed to write {} on {}", entries, this, failure); - accumulator.release(); - List allEntries = new ArrayList<>(entries); entries.clear(); try (AutoLock ignored = lock.lock()) @@ -147,6 +145,12 @@ protected void onFailure(Throwable failure) endPoint.getQuicSession().getProtocolSession().outwardClose(error, "control_stream_failure"); } + @Override + protected void onCompleteFailure(Throwable cause) + { + accumulator.release(); + } + @Override public InvocationType getInvocationType() { diff --git a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java index 7b73c228179a..33e35217ab27 100644 --- a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java +++ b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java @@ -123,8 +123,6 @@ protected void onFailure(Throwable failure) if (LOG.isDebugEnabled()) LOG.debug("failed to write buffers on {}", this, failure); - accumulator.release(); - try (AutoLock ignored = lock.lock()) { terminated = failure; @@ -138,6 +136,12 @@ protected void onFailure(Throwable failure) endPoint.getQuicSession().getProtocolSession().outwardClose(error, "instruction_stream_failure"); } + @Override + protected void onCompleteFailure(Throwable cause) + { + accumulator.release(); + } + @Override public InvocationType getInvocationType() { diff --git a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java index a0073fa2c317..865ef6237257 100644 --- a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java +++ b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java @@ -123,8 +123,6 @@ protected void onFailure(Throwable cause) if (LOG.isDebugEnabled()) LOG.debug("failed to write {} on {}", entry, this, cause); - accumulator.release(); - if (entry != null) { entry.callback.failed(cause); @@ -132,6 +130,12 @@ protected void onFailure(Throwable cause) } } + @Override + protected void onCompleteFailure(Throwable cause) + { + accumulator.release(); + } + @Override public InvocationType getInvocationType() { diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/IOResources.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/IOResources.java index 0e2fc7f481e3..68aa9c55114a 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/IOResources.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/IOResources.java @@ -415,10 +415,16 @@ protected void onCompleteSuccess() @Override protected void onFailure(Throwable x) { - if (retainableByteBuffer != null) - retainableByteBuffer.release(); IO.close(channel); super.onFailure(x); } + + @Override + protected void onCompleteFailure(Throwable cause) + { + if (retainableByteBuffer != null) + retainableByteBuffer.release(); + super.onCompleteFailure(cause); + } } } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java index bff4b267dea2..0ffba7557894 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java @@ -47,9 +47,6 @@ public InvocationType getInvocationType() @Override protected Action process() throws Throwable { - if (current != null) - current.release(); - if (terminated) return Action.SUCCEEDED; @@ -77,13 +74,25 @@ protected Action process() throws Throwable } @Override - protected void onFailure(Throwable x) + protected void onSuccess() + { + super.onSuccess(); + current.release(); + } + + @Override + protected void onFailure(Throwable cause) { - if (current != null) + ExceptionUtil.callAndThen(cause, source::fail, super::onFailure); + } + + @Override + protected void onCompleteFailure(Throwable x) + { + ExceptionUtil.callAndThen(x, ignored -> { - current.release(); - current = Content.Chunk.next(current); - } - ExceptionUtil.callAndThen(x, source::fail, super::onFailure); + if (current != null) + current.release(); + }, super::onCompleteFailure); } } diff --git a/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java b/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java index aeb09e59ff09..24ea99d9b584 100644 --- a/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java +++ b/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java @@ -539,7 +539,9 @@ protected void onCompleteSuccess() { if (LOG.isDebugEnabled()) LOG.debug("connection closed {}", QuicSession.this); - finish(new ClosedChannelException()); + cipherBuffer.release(); + finishOutwardClose(new ClosedChannelException()); + timeout.destroy(); } @Override @@ -547,14 +549,14 @@ protected void onFailure(Throwable failure) { if (LOG.isDebugEnabled()) LOG.debug("failed to write cipher bytes, closing session on {}", QuicSession.this, failure); - finish(failure); + finishOutwardClose(failure); + timeout.destroy(); } - private void finish(Throwable failure) + @Override + protected void onCompleteFailure(Throwable cause) { cipherBuffer.release(); - finishOutwardClose(failure); - timeout.destroy(); } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java index 9a7a6183e3da..1d22e479a720 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java @@ -772,10 +772,15 @@ protected void onFailure(Throwable x) { if (LOG.isDebugEnabled()) LOG.debug("Failed to write {} bytes {}", filled, TunnelConnection.this, x); - buffer.release(); disconnect(x); } + @Override + protected void onCompleteFailure(Throwable cause) + { + buffer.release(); + } + private void disconnect(Throwable x) { TunnelConnection.this.close(x); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipResponseAndCallback.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipResponseAndCallback.java index 702f765f082d..35b63b869b4e 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipResponseAndCallback.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipResponseAndCallback.java @@ -321,17 +321,6 @@ public GzipBufferCB(boolean complete, Callback callback, ByteBuffer content) LOG.debug("GzipBufferCB(complete={}, callback={}, content={})", complete, callback, BufferUtil.toDetailString(content)); } - @Override - protected void onFailure(Throwable x) - { - if (_deflaterEntry != null) - { - _deflaterEntry.release(); - _deflaterEntry = null; - } - super.onFailure(x); - } - @Override protected Action process() throws Exception { @@ -377,6 +366,17 @@ protected Action process() throws Exception }; } + @Override + protected void onCompleteFailure(Throwable x) + { + if (_deflaterEntry != null) + { + _deflaterEntry.release(); + _deflaterEntry = null; + } + super.onCompleteFailure(x); + } + private void cleanup() { if (_deflaterEntry != null) 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 57814be4f816..ca6825d9d87c 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 @@ -878,15 +878,19 @@ public Action process() throws Exception } } - private Callback release() + private Callback resetCallback() { Callback complete = _callback; _callback = null; _info = null; _content = null; + return complete; + } + + private void release() + { releaseHeader(); releaseChunk(); - return complete; } private void releaseHeader() @@ -906,13 +910,22 @@ private void releaseChunk() @Override protected void onCompleteSuccess() { - release().succeeded(); + Callback callback = resetCallback(); + release(); + callback.succeeded(); } @Override public void onFailure(final Throwable x) { - failedCallback(release(), x); + Callback callback = resetCallback(); + failedCallback(callback, x); + } + + @Override + protected void onCompleteFailure(Throwable cause) + { + release(); } @Override diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java index bc0328edb1b1..0fa609949fff 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java @@ -660,7 +660,7 @@ else if (onCompleted != null) public final void close() { Throwable onAbortedOnFailureIfNotPendingDoCompleted = null; - Throwable onAbortOnFailureOnCompleted = null; + Throwable onAbortedOnFailureOnCompleted = null; try (AutoLock ignored = _lock.lock()) { @@ -673,7 +673,7 @@ public final void close() // Nothing happening so we can abort and complete _state = State.CLOSED; _failure = new ClosedException(); - onAbortOnFailureOnCompleted = _failure; + onAbortedOnFailureOnCompleted = _failure; } case PROCESSING, PROCESSING_CALLED -> { @@ -712,8 +712,8 @@ public final void close() if (onAbortedOnFailureIfNotPendingDoCompleted != null) doOnAbortedOnFailureIfNotPendingDoCompleted(onAbortedOnFailureIfNotPendingDoCompleted); - else if (onAbortOnFailureOnCompleted != null) - doOnAbortedOnFailureOnCompleted(onAbortOnFailureOnCompleted); + else if (onAbortedOnFailureOnCompleted != null) + doOnAbortedOnFailureOnCompleted(onAbortedOnFailureOnCompleted); } /** @@ -731,7 +731,7 @@ public final boolean abort(Throwable cause) cause = Objects.requireNonNullElseGet(cause, Throwable::new); boolean onAbort = false; - boolean onAbortDoCompleteFailure = false; + boolean onAbortedOnFailureOnCompleted = false; try (AutoLock ignored = _lock.lock()) { if (LOG.isDebugEnabled()) @@ -752,7 +752,7 @@ public final boolean abort(Throwable cause) _state = State.COMPLETE; _failure = cause; _aborted = true; - onAbortDoCompleteFailure = true; + onAbortedOnFailureOnCompleted = true; break; } @@ -793,7 +793,7 @@ public final boolean abort(Throwable cause) } } - if (onAbortDoCompleteFailure) + if (onAbortedOnFailureOnCompleted) doOnAbortedOnFailureOnCompleted(cause); else if (onAbort) doOnAbortedOnFailureIfNotPendingDoCompleted(cause); diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java index cd04a884f46f..32d90a4f7c97 100644 --- a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java @@ -358,7 +358,6 @@ protected Action process() throws Throwable @Override protected void onCompleteFailure(Throwable cause) { - super.onCompleteFailure(cause); failure.incrementAndGet(); } }; diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java index 876ec8ebb1bd..719ec026ab1d 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java @@ -407,9 +407,6 @@ public void timeoutExpired() @Override public void onFailure(Throwable failure) { - if (batchBuffer != null) - batchBuffer.clear(); - releaseAggregate(); try (AutoLock l = lock.lock()) { failedEntries.addAll(queue); @@ -418,9 +415,6 @@ public void onFailure(Throwable failure) failedEntries.addAll(entries); entries.clear(); - releasableBuffers.forEach(RetainableByteBuffer::release); - releasableBuffers.clear(); - if (closedCause == null) closedCause = failure; else if (closedCause != failure) @@ -436,6 +430,19 @@ else if (closedCause != failure) endPoint.close(closedCause); } + @Override + protected void onCompleteFailure(Throwable cause) + { + if (batchBuffer != null) + batchBuffer.clear(); + releaseAggregate(); + try (AutoLock l = lock.lock()) + { + releasableBuffers.forEach(RetainableByteBuffer::release); + releasableBuffers.clear(); + } + } + private void releaseAggregate() { if (batchBuffer != null && batchBuffer.isEmpty()) diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java index 54b7ab3d1393..c0a9a22e2a3b 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java @@ -494,10 +494,9 @@ private boolean inflate(Frame frame, Callback callback, boolean first) throws Da } @Override - protected void onFailure(Throwable cause) + protected void onCompleteFailure(Throwable cause) { releasePayload(_payloadRef); - super.onFailure(cause); } private void releasePayload(AtomicReference reference) diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java index 51cc47495461..542992d94cd6 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java @@ -180,7 +180,7 @@ protected void onFailure(Throwable t) notifyCallbackFailure(current.callback, t); current = null; } - this.onFailure(t); + TransformingFlusher.this.onFailure(t); } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java index ae49f9343d5a..bf7095d8402a 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java @@ -223,7 +223,8 @@ private void onWriteComplete(boolean last, Throwable failure) _state = State.CLOSED; closedCallback = _closedCallback; _closedCallback = null; - lockedReleaseBuffer(failure != null); + if (failure == null) + lockedReleaseBuffer(false); wake = updateApiState(failure); } else if (_state == State.CLOSE) @@ -1425,10 +1426,19 @@ protected void onCompleteSuccess() } @Override - public void onFailure(Throwable e) + protected void onFailure(Throwable e) { onWriteComplete(_last, e); } + + @Override + protected void onCompleteFailure(Throwable cause) + { + try (AutoLock ignored = _channelState.lock()) + { + lockedReleaseBuffer(true); + } + } } private abstract class NestedChannelWriteCB extends ChannelWriteCB @@ -1461,7 +1471,7 @@ protected void onCompleteSuccess() } @Override - public void onFailure(Throwable e) + protected void onFailure(Throwable e) { try { @@ -1662,15 +1672,18 @@ protected void onCompleteSuccess() { _buffer.release(); IO.close(_in); - super.onCompleteSuccess(); } @Override - public void onFailure(Throwable x) + protected void onFailure(Throwable cause) { - _buffer.release(); IO.close(_in); - super.onFailure(x); + } + + @Override + public void onCompleteFailure(Throwable x) + { + _buffer.release(); } } @@ -1735,15 +1748,18 @@ protected void onCompleteSuccess() { _buffer.release(); IO.close(_in); - super.onCompleteSuccess(); } @Override - public void onFailure(Throwable x) + protected void onFailure(Throwable cause) { - _buffer.release(); IO.close(_in); - super.onFailure(x); + } + + @Override + public void onCompleteFailure(Throwable x) + { + _buffer.release(); } } diff --git a/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncProxyServlet.java b/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncProxyServlet.java index 6d8508224588..ce317f0f0b4d 100644 --- a/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncProxyServlet.java +++ b/jetty-ee11/jetty-ee11-proxy/src/main/java/org/eclipse/jetty/ee11/proxy/AsyncProxyServlet.java @@ -194,7 +194,6 @@ protected void onRequestContent(HttpServletRequest request, Request proxyRequest @Override public void onFailure(Throwable x) { - super.onFailure(x); onError(x); } } diff --git a/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java b/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java index d28444d8c7c4..2969de680688 100644 --- a/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java +++ b/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java @@ -222,7 +222,8 @@ private void onWriteComplete(boolean last, Throwable failure) _state = State.CLOSED; closedCallback = _closedCallback; _closedCallback = null; - releaseBuffer(); + if (failure == null) + releaseBuffer(); wake = updateApiState(failure); } else if (_state == State.CLOSE) @@ -1427,10 +1428,19 @@ protected void onCompleteSuccess() } @Override - public void onFailure(Throwable e) + protected void onFailure(Throwable e) { onWriteComplete(_last, e); } + + @Override + protected void onCompleteFailure(Throwable cause) + { + try (AutoLock ignored = _channelState.lock()) + { + releaseBuffer(); + } + } } private abstract class NestedChannelWriteCB extends ChannelWriteCB @@ -1463,7 +1473,7 @@ protected void onCompleteSuccess() } @Override - public void onFailure(Throwable e) + protected void onFailure(Throwable e) { try { @@ -1664,15 +1674,18 @@ protected void onCompleteSuccess() { _buffer.release(); IO.close(_in); - super.onCompleteSuccess(); } @Override - public void onFailure(Throwable x) + protected void onFailure(Throwable cause) { - _buffer.release(); IO.close(_in); - super.onFailure(x); + } + + @Override + public void onCompleteFailure(Throwable x) + { + _buffer.release(); } } @@ -1737,15 +1750,18 @@ protected void onCompleteSuccess() { _buffer.release(); IO.close(_in); - super.onCompleteSuccess(); } @Override - public void onFailure(Throwable x) + protected void onFailure(Throwable cause) { - _buffer.release(); IO.close(_in); - super.onFailure(x); + } + + @Override + public void onCompleteFailure(Throwable x) + { + _buffer.release(); } } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java index 02902d780da1..ef6fd6b1f0c0 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java @@ -301,7 +301,8 @@ private void onWriteComplete(boolean last, Throwable failure) _state = State.CLOSED; closedCallback = _closedCallback; _closedCallback = null; - lockedReleaseBuffer(failure != null); + if (failure == null) + lockedReleaseBuffer(false); wake = updateApiState(failure); } else if (_state == State.CLOSE) @@ -1612,10 +1613,19 @@ protected void onCompleteSuccess() } @Override - public void onFailure(Throwable e) + protected void onFailure(Throwable e) { onWriteComplete(_last, e); } + + @Override + protected void onCompleteFailure(Throwable cause) + { + try (AutoLock l = _channelState.lock()) + { + lockedReleaseBuffer(true); + } + } } private abstract class NestedChannelWriteCB extends ChannelWriteCB @@ -1648,7 +1658,7 @@ protected void onCompleteSuccess() } @Override - public void onFailure(Throwable e) + protected void onFailure(Throwable e) { try { @@ -1848,15 +1858,18 @@ protected void onCompleteSuccess() { _buffer.release(); IO.close(_in); - super.onCompleteSuccess(); } @Override - public void onFailure(Throwable x) + protected void onFailure(Throwable cause) { - _buffer.release(); IO.close(_in); - super.onFailure(x); + } + + @Override + public void onCompleteFailure(Throwable x) + { + _buffer.release(); } } @@ -1919,15 +1932,18 @@ protected void onCompleteSuccess() { _buffer.release(); IO.close(_in); - super.onCompleteSuccess(); } @Override - public void onFailure(Throwable x) + protected void onFailure(Throwable cause) { - _buffer.release(); IO.close(_in); - super.onFailure(x); + } + + @Override + public void onCompleteFailure(Throwable x) + { + _buffer.release(); } } From b56de27b84e4ba3d45fbbb7af9230a0032fe23a7 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 19 Jul 2024 16:19:19 +0200 Subject: [PATCH 06/22] Made onCompleted() private to address PR review. This is necessary to avoid confusion, for example class A_ICB extending ICB and overriding onCompleted() without calling super, and class B_ICB extending A_ICB and overriding onCompleteSuccess() which would be never called. Signed-off-by: Simone Bordet --- .../eclipse/jetty/util/IteratingCallback.java | 2 +- .../jetty/util/IteratingCallbackTest.java | 68 +++++++++++-------- 2 files changed, 39 insertions(+), 31 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java index bc0328edb1b1..071ca8ed1bc2 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java @@ -243,7 +243,7 @@ protected void onAborted(Throwable cause) * @param causeOrNull the cause of any {@link #abort(Throwable) abort} or {@link #failed(Throwable) failure}, * else {@code null} for {@link #succeeded() success}. */ - protected void onCompleted(Throwable causeOrNull) + private void onCompleted(Throwable causeOrNull) { if (causeOrNull == null) onCompleteSuccess(); diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java index cd04a884f46f..6ccfdab5cefa 100644 --- a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java @@ -438,16 +438,16 @@ protected void onAborted(Throwable cause) } @Override - protected void onCompleteFailure(Throwable cause) + protected void onCompleteSuccess() { - failure.set(cause); + completed.set(null, true); } @Override - protected void onCompleted(Throwable causeOrNull) + protected void onCompleteFailure(Throwable cause) { - completed.set(causeOrNull, true); - super.onCompleted(causeOrNull); + completed.set(cause, true); + failure.set(cause); } }; @@ -536,16 +536,16 @@ protected void onAborted(Throwable cause) } @Override - protected void onCompleteFailure(Throwable cause) + protected void onCompleteSuccess() { - failure.set(cause); + completed.set(null, true); } @Override - protected void onCompleted(Throwable causeOrNull) + protected void onCompleteFailure(Throwable cause) { - completed.set(causeOrNull, true); - super.onCompleted(causeOrNull); + completed.set(cause, true); + failure.set(cause); } }; @@ -773,10 +773,15 @@ protected void onAborted(Throwable cause) } @Override - protected void onCompleted(Throwable causeOrNull) + protected void onCompleteSuccess() { - onCompleted.set(causeOrNull, true); - super.onCompleted(causeOrNull); + onCompleted.set(null, true); + } + + @Override + protected void onCompleteFailure(Throwable cause) + { + onCompleted.set(cause, true); } }; @@ -865,7 +870,7 @@ private static class TestIteratingCB extends IteratingCallback final AtomicBoolean _onSuccess = new AtomicBoolean(); final AtomicReference _onFailure = new AtomicReference<>(); final AtomicMarkableReference _completion = new AtomicMarkableReference<>(null, false); - final CountDownLatch _completed = new CountDownLatch(2); + final CountDownLatch _completed = new CountDownLatch(1); private TestIteratingCB() { @@ -903,31 +908,29 @@ protected void onFailure(Throwable cause) _badCalls.incrementAndGet(); } - @Override - protected void onCompleteFailure(Throwable cause) - { - if (_completion.compareAndSet(null, cause, false, true)) - _completed.countDown(); - - Throwable failure = _completion.getReference(); - if (failure != null && _completion.compareAndSet(failure, failure, false, true)) - _completed.countDown(); - } - @Override protected void onCompleteSuccess() { + if (_completion.isMarked()) + _badCalls.incrementAndGet(); + if (_completion.compareAndSet(null, null, false, true)) _completed.countDown(); } @Override - protected void onCompleted(Throwable causeOrNull) + protected void onCompleteFailure(Throwable cause) { if (_completion.isMarked()) _badCalls.incrementAndGet(); - super.onCompleted(causeOrNull); - _completed.countDown(); + + if (_completion.compareAndSet(null, cause, false, true)) + _completed.countDown(); + + // Try again the CAS if there was a call to onAborted(). + Throwable failure = _completion.getReference(); + if (failure != null && _completion.compareAndSet(failure, failure, false, true)) + _completed.countDown(); } public void checkNoBadCalls() @@ -958,9 +961,14 @@ protected void onAborted(Throwable cause) } @Override - protected void onCompleted(Throwable causeOrNull) + protected void onCompleteSuccess() + { + latch.countDown(); + } + + @Override + protected void onCompleteFailure(Throwable cause) { - super.onCompleted(causeOrNull); latch.countDown(); } }; From c50b4da97e667aa3a14846442777dddfa121e9a5 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 24 Jul 2024 18:15:37 +1000 Subject: [PATCH 07/22] Fix --- .../eclipse/jetty/websocket/core/util/TransformingFlusher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java index 51cc47495461..542992d94cd6 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/TransformingFlusher.java @@ -180,7 +180,7 @@ protected void onFailure(Throwable t) notifyCallbackFailure(current.callback, t); current = null; } - this.onFailure(t); + TransformingFlusher.this.onFailure(t); } } From 14d3b08e95ffa1bae817b967bdce3b767a4bb20e Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 25 Jul 2024 11:56:55 +1000 Subject: [PATCH 08/22] reverted import changes from bad merge --- .../main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java | 1 + .../main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java | 2 ++ 2 files changed, 3 insertions(+) diff --git a/jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java b/jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java index 8df588c64dff..1fd30b5e234e 100644 --- a/jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java +++ b/jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java @@ -52,6 +52,7 @@ import org.eclipse.jetty.server.Deployable; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.handler.ContextHandler; +import org.eclipse.jetty.util.ClassMatcher; import org.eclipse.jetty.util.ExceptionUtil; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.StringUtil; diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java index 89a9c87a0fe3..8e557e0d47fc 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java @@ -71,6 +71,8 @@ import org.eclipse.jetty.server.AllowedResourceAliasChecker; import org.eclipse.jetty.server.Context; import org.eclipse.jetty.server.FormFields; +import org.eclipse.jetty.server.Handler; +import org.eclipse.jetty.server.Response; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.Session; import org.eclipse.jetty.server.handler.ContextHandler.ScopedContext; From 5854e11f229084f006d5c3ff7ddeacec4c4fd10c Mon Sep 17 00:00:00 2001 From: gregw Date: Sat, 3 Aug 2024 10:30:26 +1000 Subject: [PATCH 09/22] releaseForRemoval --- .../eclipse/jetty/io/ArrayByteBufferPool.java | 4 +- .../org/eclipse/jetty/io/ByteBufferPool.java | 12 +++-- .../jetty/io/RetainableByteBuffer.java | 17 +++++++ .../jetty/io/ArrayByteBufferPoolTest.java | 8 +-- .../jetty/server/handler/ErrorHandler.java | 12 +++-- .../jetty/ee10/servlet/HttpOutput.java | 15 ++---- .../jetty/ee11/servlet/HttpOutput.java | 51 ++++++++----------- .../ee11/servlet/ServletChannelState.java | 5 ++ .../eclipse/jetty/ee9/nested/HttpOutput.java | 8 +-- 9 files changed, 75 insertions(+), 57 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java index 01cbe9a3515d..c1f93021864a 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java @@ -224,7 +224,7 @@ public RetainableByteBuffer.Mutable acquire(int size, boolean direct) } @Override - public boolean removeAndRelease(RetainableByteBuffer buffer) + public boolean releaseForRemoval(RetainableByteBuffer buffer) { RetainableByteBuffer actual = buffer; while (actual instanceof RetainableByteBuffer.Wrapper wrapper) @@ -244,7 +244,7 @@ public boolean removeAndRelease(RetainableByteBuffer buffer) return buffer.release(); } - return ByteBufferPool.super.removeAndRelease(buffer); + return ByteBufferPool.super.releaseForRemoval(buffer); } private void reserve(RetainedBucket bucket, ByteBuffer byteBuffer) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java index 77caa43226c1..5be18db049fe 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java @@ -63,11 +63,9 @@ public interface ByteBufferPool * If the buffer is not in a pool, calling this method is equivalent to calling {@link RetainableByteBuffer#release()}. * Calling this method satisfies any contract that requires a call to {@link RetainableByteBuffer#release()}. * @return {@code true} if a call to {@link RetainableByteBuffer#release()} would have returned {@code true}. - * @see RetainableByteBuffer#release() - * @deprecated This API is experimental and may be removed in future releases + * @see RetainableByteBuffer#releaseForRemoval() */ - @Deprecated - default boolean removeAndRelease(RetainableByteBuffer buffer) + default boolean releaseForRemoval(RetainableByteBuffer buffer) { return buffer != null && buffer.release(); } @@ -95,6 +93,12 @@ public ByteBufferPool getWrapped() return wrapped; } + @Override + public boolean releaseForRemoval(RetainableByteBuffer buffer) + { + return getWrapped().releaseForRemoval(buffer); + } + @Override public RetainableByteBuffer.Mutable acquire(int size, boolean direct) { diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index f570d2dd3f24..8586ed8b6191 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -163,6 +163,11 @@ default Mutable asMutable() throws ReadOnlyBufferException throw new ReadOnlyBufferException(); } + default boolean releaseForRemoval() + { + return release(); + } + /** * Appends and consumes the contents of this buffer to the passed buffer, limited by the capacity of the target buffer. * @param buffer The buffer to append bytes to, whose limit will be updated. @@ -656,6 +661,12 @@ public RetainableByteBuffer getWrapped() return (RetainableByteBuffer)super.getWrapped(); } + @Override + public boolean releaseForRemoval() + { + return getWrapped().releaseForRemoval(); + } + @Override public boolean isRetained() { @@ -1300,6 +1311,12 @@ protected Pooled(ByteBufferPool pool, ByteBuffer byteBuffer, Retainable retainab _pool = pool; } + @Override + public boolean releaseForRemoval() + { + return _pool.releaseForRemoval(this); + } + @Override public RetainableByteBuffer slice(long length) { diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java index 25e424f47bc4..26c8e2c9f5fb 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java @@ -448,7 +448,7 @@ public void testReleaseExcessMemory() } @Test - public void testRemoveAndRelease() + public void testReleaseForRemoval() { ArrayByteBufferPool pool = new ArrayByteBufferPool(); @@ -471,9 +471,9 @@ public void testRemoveAndRelease() retained1 = pool.acquire(1024, false); retained1.retain(); - assertTrue(pool.removeAndRelease(reserved1)); - assertTrue(pool.removeAndRelease(acquired1)); - assertFalse(pool.removeAndRelease(retained1)); + assertTrue(reserved1.releaseForRemoval()); + assertTrue(acquired1.releaseForRemoval()); + assertFalse(retained1.releaseForRemoval()); assertTrue(retained1.release()); assertThat(pool.getHeapByteBufferCount(), is(2L)); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java index d32e4255c39a..f24abc85701a 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java @@ -261,7 +261,7 @@ else if (charsets.contains(StandardCharsets.ISO_8859_1)) catch (Throwable x) { if (buffer != null) - byteBufferPool.removeAndRelease(buffer); + buffer.releaseForRemoval(); throw x; } } @@ -600,7 +600,9 @@ public WriteErrorCallback(Callback callback, ByteBufferPool pool, RetainableByte public void succeeded() { Callback callback = _callback.getAndSet(null); - if (callback != null) + if (callback == null) + _buffer.release(); + else ExceptionUtil.callAndThen(_buffer::release, callback::succeeded); } @@ -608,8 +610,10 @@ public void succeeded() public void failed(Throwable x) { Callback callback = _callback.getAndSet(null); - if (callback != null) - ExceptionUtil.callAndThen(x, t -> _pool.removeAndRelease(_buffer), callback::failed); + if (callback == null) + _buffer.releaseForRemoval(); + else + ExceptionUtil.callAndThen(x, t -> _buffer.releaseForRemoval(), callback::failed); } } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java index b247b9062aba..e34b4eea5e50 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java @@ -132,9 +132,7 @@ enum ApiState private State _state = State.OPEN; private boolean _softClose = false; private long _written; - private long _flushed; private long _firstByteNanoTime = -1; - private ByteBufferPool _pool; private RetainableByteBuffer _aggregate; private int _bufferSize; private int _commitSize; @@ -588,8 +586,7 @@ private RetainableByteBuffer lockedAcquireBuffer() if (_aggregate == null) { - _pool = _servletChannel.getRequest().getComponents().getByteBufferPool(); - _aggregate = _pool.acquire(getBufferSize(), useOutputDirectByteBuffers); + _aggregate = _servletChannel.getRequest().getComponents().getByteBufferPool().acquire(getBufferSize(), useOutputDirectByteBuffers); } return _aggregate; } @@ -600,12 +597,11 @@ private void lockedReleaseBuffer(boolean failure) if (_aggregate != null) { - if (failure && _pool != null) - _pool.removeAndRelease(_aggregate); + if (failure) + _aggregate.releaseForRemoval(); else _aggregate.release(); _aggregate = null; - _pool = null; } } @@ -1260,7 +1256,6 @@ public void recycle() _writeListener = null; _onError = null; _firstByteNanoTime = -1; - _flushed = 0; _closedCallback = null; } } @@ -1643,7 +1638,7 @@ protected void onCompleteSuccess() @Override public void onFailure(Throwable x) { - _buffer.release(); + _buffer.releaseForRemoval(); IO.close(_in); super.onFailure(x); } @@ -1716,7 +1711,7 @@ protected void onCompleteSuccess() @Override public void onFailure(Throwable x) { - _buffer.release(); + _buffer.releaseForRemoval(); IO.close(_in); super.onFailure(x); } diff --git a/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java b/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java index 0e3b8936295e..9ca23d523e82 100644 --- a/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java +++ b/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/HttpOutput.java @@ -132,7 +132,6 @@ enum ApiState private State _state = State.OPEN; private boolean _softClose = false; private long _written; - private long _flushed; private long _firstByteNanoTime = -1; private RetainableByteBuffer _aggregate; private int _bufferSize; @@ -221,7 +220,7 @@ private void onWriteComplete(boolean last, Throwable failure) _state = State.CLOSED; closedCallback = _closedCallback; _closedCallback = null; - releaseBuffer(); + lockedReleaseBuffer(failure != null); wake = updateApiState(failure); } else if (_state == State.CLOSE) @@ -318,20 +317,6 @@ public void softClose() } } - public ByteBuffer takeContentAndClose() - { - try (AutoLock l = _channelState.lock()) - { - if (_state != State.OPEN) - throw new IllegalStateException(stateString()); - // TODO avoid this copy. - ByteBuffer content = _aggregate != null && _aggregate.hasRemaining() ? BufferUtil.copy(_aggregate.getByteBuffer()) : BufferUtil.EMPTY_BUFFER; - _state = State.CLOSED; - releaseBuffer(); - return content; - } - } - /** * This method is invoked for the COMPLETE action handling in * HttpChannel.handle. The callback passed typically will call completed @@ -457,7 +442,7 @@ public void completed(Throwable failure) try (AutoLock ignored = _channelState.lock()) { _state = State.CLOSED; - releaseBuffer(); + lockedReleaseBuffer(failure != null); } } @@ -589,24 +574,33 @@ public ByteBuffer getByteBuffer() { try (AutoLock ignored = _channelState.lock()) { - return acquireBuffer().getByteBuffer(); + return lockedAcquireBuffer().getByteBuffer(); } } - private RetainableByteBuffer acquireBuffer() + private RetainableByteBuffer lockedAcquireBuffer() { + assert _channelState.isLockHeldByCurrentThread(); + boolean useOutputDirectByteBuffers = _servletChannel.getConnectionMetaData().getHttpConfiguration().isUseOutputDirectByteBuffers(); - ByteBufferPool pool = _servletChannel.getRequest().getComponents().getByteBufferPool(); + if (_aggregate == null) - _aggregate = pool.acquire(getBufferSize(), useOutputDirectByteBuffers); + { + _aggregate = _servletChannel.getRequest().getComponents().getByteBufferPool().acquire(getBufferSize(), useOutputDirectByteBuffers); + } return _aggregate; } - private void releaseBuffer() + private void lockedReleaseBuffer(boolean failure) { + assert _channelState.isLockHeldByCurrentThread(); + if (_aggregate != null) { - _aggregate.release(); + if (failure) + _aggregate.releaseForRemoval(); + else + _aggregate.release(); _aggregate = null; } } @@ -765,7 +759,7 @@ public void write(byte[] b, int off, int len) throws IOException // Should we aggregate? if (aggregate) { - acquireBuffer(); + lockedAcquireBuffer(); int filled = BufferUtil.fill(_aggregate.getByteBuffer(), b, off, len); // return if we are not complete, not full and filled all the content @@ -970,7 +964,7 @@ public void write(int b) throws IOException } _written = written; - acquireBuffer(); + lockedAcquireBuffer(); BufferUtil.append(_aggregate.getByteBuffer(), (byte)b); } @@ -1249,6 +1243,7 @@ public void recycle() { try (AutoLock ignored = _channelState.lock()) { + lockedReleaseBuffer(_state != State.CLOSED); _state = State.OPEN; _apiState = ApiState.BLOCKING; _softClose = true; // Stay closed until next request @@ -1257,12 +1252,10 @@ public void recycle() _commitSize = config.getOutputAggregationSize(); if (_commitSize > _bufferSize) _commitSize = _bufferSize; - releaseBuffer(); _written = 0; _writeListener = null; _onError = null; _firstByteNanoTime = -1; - _flushed = 0; _closedCallback = null; } } @@ -1645,7 +1638,7 @@ protected void onCompleteSuccess() @Override public void onFailure(Throwable x) { - _buffer.release(); + _buffer.releaseForRemoval(); IO.close(_in); super.onFailure(x); } @@ -1718,7 +1711,7 @@ protected void onCompleteSuccess() @Override public void onFailure(Throwable x) { - _buffer.release(); + _buffer.releaseForRemoval(); IO.close(_in); super.onFailure(x); } diff --git a/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletChannelState.java b/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletChannelState.java index e9b7f0aa9b02..57791ea11f15 100644 --- a/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletChannelState.java +++ b/jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletChannelState.java @@ -224,6 +224,11 @@ AutoLock lock() return _lock.lock(); } + boolean isLockHeldByCurrentThread() + { + return _lock.isHeldByCurrentThread(); + } + public State getState() { try (AutoLock ignored = lock()) diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java index 083e0b8df63f..216bf23d22a2 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java @@ -672,8 +672,8 @@ private void lockedReleaseBuffer(boolean failure) if (_aggregate != null) { - if (failure && _pool != null) - _pool.removeAndRelease(_aggregate); + if (failure) + _aggregate.releaseForRemoval(); else _aggregate.release(); _aggregate = null; @@ -1852,7 +1852,7 @@ protected void onCompleteSuccess() @Override public void onFailure(Throwable x) { - _buffer.release(); + _buffer.releaseForRemoval(); IO.close(_in); super.onFailure(x); } @@ -1923,7 +1923,7 @@ protected void onCompleteSuccess() @Override public void onFailure(Throwable x) { - _buffer.release(); + _buffer.releaseForRemoval(); IO.close(_in); super.onFailure(x); } From 92b37b8a6427a83617cd246eb4c0b57d66a73c45 Mon Sep 17 00:00:00 2001 From: gregw Date: Sat, 3 Aug 2024 10:45:53 +1000 Subject: [PATCH 10/22] improved javadoc --- .../eclipse/jetty/util/IteratingCallback.java | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java index 071ca8ed1bc2..8716a41a2532 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java @@ -183,15 +183,24 @@ protected void onSuccess() *

* Calls to this method are serialized with respect to {@link #onAborted(Throwable)}, {@link #process()}, * {@link #onCompleteFailure(Throwable)} and {@link #onCompleted(Throwable)}. - * + *

+ * Because {@code onFailure} can be called due to an {@link #abort(Throwable)} or {@link #close()} operation, it is + * possible that any resources passed to a {@link Action#SCHEDULED} operation may still be in use, and thus should not + * be recycled by this call. For example any buffers passed to a write operation should not be returned to a buffer + * pool by implementations of {@code onFailure}. Such resources may be discarded here, or safely recycled in a + * subsequent call to {@link #onCompleted(Throwable)} or {@link #onCompleteFailure(Throwable)}, when + * the {@link Action#SCHEDULED} operation has completed. * @param cause The cause of the failure or abort + * @see #onCompleted(Throwable) + * @see #onCompleteFailure(Throwable) */ protected void onFailure(Throwable cause) { } /** - * Invoked when the overall task has completed successfully. + * Invoked when the overall task has completed successfully, specifically after any {@link Action#SCHEDULED} operations + * have {@link Callback#succeeded()} and {@link #process()} has returned {@link Action#SUCCEEDED}. *

* Calls to this method are serialized with respect to {@link #process()}, {@link #onAborted(Throwable)} * and {@link #onCompleted(Throwable)}. @@ -226,8 +235,16 @@ protected void onCompleteFailure(Throwable cause) *

* The default implementation of this method calls {@link #failed(Throwable)}. Overridden implementations of * this method SHOULD NOT call {@code super.onAborted(Throwable)}. - * + *

+ * Because {@code onAborted} can be called due to an {@link #abort(Throwable)} or {@link #close()} operation, it is + * possible that any resources passed to a {@link Action#SCHEDULED} operation may still be in use, and thus should not + * be recycled by this call. For example any buffers passed to a write operation should not be returned to a buffer + * pool by implementations of {@code onFailure}. Such resources may be discarded here, or safely recycled in a + * subsequent call to {@link #onCompleted(Throwable)} or {@link #onCompleteFailure(Throwable)}, when + * the {@link Action#SCHEDULED} operation has completed. * @param cause The cause of the abort + * @see #onCompleted(Throwable) + * @see #onCompleteFailure(Throwable) */ protected void onAborted(Throwable cause) { From c00f6f075029565067d7b6060a3377d3d909cc64 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 5 Aug 2024 11:45:12 +1000 Subject: [PATCH 11/22] WIP --- .../jetty/docs/programming/ContentDocs.java | 24 ++++++++++++------- .../jetty/docs/programming/WebSocketDocs.java | 2 ++ .../docs/programming/server/ServerDocs.java | 6 ++--- .../jetty/http2/internal/HTTP2Flusher.java | 8 +++++-- .../jetty/http3/InstructionFlusher.java | 8 +++++-- .../eclipse/jetty/http3/MessageFlusher.java | 8 +++++-- .../eclipse/jetty/util/IteratingCallback.java | 2 +- .../websocket/core/util/DemandingFlusher.java | 4 +--- 8 files changed, 41 insertions(+), 21 deletions(-) diff --git a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java index d406ee1d3bd9..a3b7242aebf8 100644 --- a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java +++ b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java @@ -332,8 +332,8 @@ protected Action process() throws Throwable // No chunk, demand to be called back when there will be more chunks. if (chunk == null) { - source.demand(this::iterate); - return Action.IDLE; + source.demand(this::succeeded); + return Action.SCHEDULED; } // The read failed, re-throw the failure @@ -351,6 +351,9 @@ protected void onSuccess() { // After every successful write, release the chunk. chunk.release(); + + // Reset the chunk, preserving errors and EOF + chunk = Content.Chunk.next(chunk); } @Override @@ -361,14 +364,19 @@ protected void onCompleteSuccess() } @Override - protected void onCompleteFailure(Throwable failure) + protected void onFailure(Throwable cause) { - // In case of a failure, either on the - // read or on the write, release the chunk. - chunk.release(); - // The copy is failed, fail the callback. - callback.failed(failure); + // This may occur before a write has completed (due to abort or close), so we cannot release the chunk here. + callback.failed(cause); + } + + @Override + protected void onCompleteFailure(Throwable failure) + { + // In case of a failure, we wait until the write has completed before releasing any chunk. + if (chunk != null) + chunk.release(); } @Override diff --git a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java index b0a65e64e336..d89fcf6a487e 100644 --- a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java +++ b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java @@ -523,6 +523,7 @@ protected Action process() throws Throwable // <2> @Override public void succeed() { + // Map the o.e.j.websocket.api.Callback to o.e.jetty.util.Callback API // When the send succeeds, succeed this IteratingCallback. succeeded(); } @@ -530,6 +531,7 @@ public void succeed() @Override public void fail(Throwable x) { + // Map the o.e.j.websocket.api.Callback to o.e.jetty.util.Callback API // When the send fails, fail this IteratingCallback. failed(x); } diff --git a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java index a109be5ed6d3..de8a00eb5c7a 100644 --- a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java +++ b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java @@ -176,7 +176,7 @@ public void onOpen() @Override public void onFillable() { - callback.iterate(); + callback.succeeded(); } private class JSONHTTPIteratingCallback extends IteratingCallback @@ -222,8 +222,8 @@ else if (filled == 0) // again interest for fill events. fillInterested(); - // Signal that the iteration is now IDLE. - return Action.IDLE; + // Signal that the iteration is now SCHEDULED for a callback to onFillable. + return Action.SCHEDULED; } else { diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java index 68f9521459ec..6c3e8aa9d779 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java @@ -346,8 +346,6 @@ protected void onCompleteSuccess() @Override protected void onFailure(Throwable x) { - release(); - Throwable closed; Set allEntries; try (AutoLock ignored = lock.lock()) @@ -376,6 +374,12 @@ protected void onFailure(Throwable x) session.onWriteFailure(x); } + @Override + protected void onCompleteFailure(Throwable cause) + { + release(); + } + public void terminate(Throwable cause) { Throwable closed; diff --git a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java index 7b73c228179a..33e35217ab27 100644 --- a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java +++ b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/InstructionFlusher.java @@ -123,8 +123,6 @@ protected void onFailure(Throwable failure) if (LOG.isDebugEnabled()) LOG.debug("failed to write buffers on {}", this, failure); - accumulator.release(); - try (AutoLock ignored = lock.lock()) { terminated = failure; @@ -138,6 +136,12 @@ protected void onFailure(Throwable failure) endPoint.getQuicSession().getProtocolSession().outwardClose(error, "instruction_stream_failure"); } + @Override + protected void onCompleteFailure(Throwable cause) + { + accumulator.release(); + } + @Override public InvocationType getInvocationType() { diff --git a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java index a0073fa2c317..865ef6237257 100644 --- a/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java +++ b/jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/MessageFlusher.java @@ -123,8 +123,6 @@ protected void onFailure(Throwable cause) if (LOG.isDebugEnabled()) LOG.debug("failed to write {} on {}", entry, this, cause); - accumulator.release(); - if (entry != null) { entry.callback.failed(cause); @@ -132,6 +130,12 @@ protected void onFailure(Throwable cause) } } + @Override + protected void onCompleteFailure(Throwable cause) + { + accumulator.release(); + } + @Override public InvocationType getInvocationType() { diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java index 8716a41a2532..cee96d357131 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java @@ -260,7 +260,7 @@ protected void onAborted(Throwable cause) * @param causeOrNull the cause of any {@link #abort(Throwable) abort} or {@link #failed(Throwable) failure}, * else {@code null} for {@link #succeeded() success}. */ - private void onCompleted(Throwable causeOrNull) + protected void onCompleted(Throwable causeOrNull) { if (causeOrNull == null) onCompleteSuccess(); diff --git a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/DemandingFlusher.java b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/DemandingFlusher.java index 9a8a2afff7d0..4465873e3f39 100644 --- a/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/DemandingFlusher.java +++ b/jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/DemandingFlusher.java @@ -153,7 +153,7 @@ protected Action process() throws Throwable throw failure; if (!_demand.get()) - break; + return Action.IDLE; if (_needContent) { @@ -173,8 +173,6 @@ protected Action process() throws Throwable _callback = null; } } - - return Action.IDLE; } @Override From d3157bf087d8fa8624ea44e2fb31197a73cc9582 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 5 Aug 2024 16:48:07 +1000 Subject: [PATCH 12/22] WIP --- .../jetty/http2/internal/HTTP2Flusher.java | 16 ++++--- .../jetty/io/RetainableByteBuffer.java | 42 +++++++++++++++++-- 2 files changed, 45 insertions(+), 13 deletions(-) diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java index 6c3e8aa9d779..201511fb8c5e 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java @@ -308,7 +308,7 @@ protected void onSuccess() private void finish() { - release(); + release(false); processedEntries.forEach(HTTP2Session.Entry::succeeded); processedEntries.clear(); invocationType = InvocationType.NON_BLOCKING; @@ -328,12 +328,15 @@ private void finish() } } - private void release() + private void release(boolean failure) { if (!released) { released = true; - accumulator.release(); + if (failure) + accumulator.releaseForRemoval(); + else + accumulator.release(); } } @@ -346,6 +349,7 @@ protected void onCompleteSuccess() @Override protected void onFailure(Throwable x) { + release(true); Throwable closed; Set allEntries; try (AutoLock ignored = lock.lock()) @@ -374,12 +378,6 @@ protected void onFailure(Throwable x) session.onWriteFailure(x); } - @Override - protected void onCompleteFailure(Throwable cause) - { - release(); - } - public void terminate(Throwable cause) { Throwable closed; diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 8586ed8b6191..18e7a5ffda89 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -1938,6 +1938,22 @@ public boolean release() return false; } + @Override + public boolean releaseForRemoval() + { + if (LOG.isDebugEnabled()) + LOG.debug("release {}", this); + if (super.release()) + { + for (RetainableByteBuffer buffer : _buffers) + buffer.releaseForRemoval(); + _buffers.clear(); + _aggregate = null; + return true; + } + return false; + } + @Override public void clear() { @@ -2331,10 +2347,6 @@ public void writeTo(Content.Sink sink, boolean last, Callback callback) @Override protected Action process() { - // release the last buffer written - if (_buffer != null) - _buffer.release(); - // write next buffer if (_index < _buffers.size()) { @@ -2355,6 +2367,28 @@ protected Action process() _buffers.clear(); return Action.SUCCEEDED; } + + @Override + protected void onSuccess() + { + // release the last buffer written + if (_buffer != null) + { + _buffer.release(); + _buffer = null; + } + } + + @Override + protected void onCompleteFailure(Throwable x) + { + // release the last buffer written + if (_buffer != null) + { + _buffer.releaseForRemoval(); + _buffer = null; + } + } }.iterate(); } } From d65bc94fd828898541efd0dfb9bd850e3485348a Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 5 Aug 2024 17:03:31 +1000 Subject: [PATCH 13/22] documentation --- .../docs/programming/SelectorManagerDocs.java | 2 +- .../modules/programming-guide/pages/arch/io.adoc | 11 ++++++----- .../org/eclipse/jetty/io/AbstractConnection.java | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java index cc60b78e7f92..360ab386e86a 100644 --- a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java +++ b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java @@ -277,7 +277,7 @@ else if (filled == 0) // No more bytes to read, declare // again interest for fill events. - fillInterested(); + fillInterested(this); // Signal that the iteration is now IDLE. return Action.IDLE; diff --git a/documentation/jetty/modules/programming-guide/pages/arch/io.adoc b/documentation/jetty/modules/programming-guide/pages/arch/io.adoc index 9de123c71f41..ff746a899af7 100644 --- a/documentation/jetty/modules/programming-guide/pages/arch/io.adoc +++ b/documentation/jetty/modules/programming-guide/pages/arch/io.adoc @@ -187,7 +187,7 @@ In turn, this calls `IteratingCallback.process()`, an abstract method that must Method `process()` must return: * `Action.SCHEDULED`, to indicate whether the loop has performed a non-blocking, possibly asynchronous, operation -* `Action.IDLE`, to indicate that the loop should temporarily be suspended to be resumed later +* `Action.IDLE`, to indicate that the loop should temporarily be suspended to be resumed later with another call to iterate * `Action.SUCCEEDED` to indicate that the loop exited successfully Any exception thrown within `process()` exits the loops with a failure. @@ -209,13 +209,14 @@ If this was the only active network connection, the system would now be idle, wi Eventually, the Jetty I/O system will notify that the `write()` completed; this notifies the `IteratingCallback` that can now resume the loop and call `process()` again. -When `process()` is called, it is possible that zero bytes are read from the network; in this case, you want to deallocate the buffer since the other peer may never send more bytes for the `Connection` to read, or it may send them after a long pause -- in both cases we do not want to retain the memory allocated by the buffer; next, you want to call `fillInterested()` to declare again interest for read events, and return `Action.IDLE` since there is nothing to write back and therefore the loop may be suspended. -When more bytes are again available to be read from the network, `onFillable()` will be called again and that will start the iteration again. +When `process()` is called, it is possible that zero bytes are read from the network; in this case, you want to deallocate the buffer since the other peer may never send more bytes for the `Connection` to read, or it may send them after a long pause -- in both cases we do not want to retain the memory allocated by the buffer; next, you want to call `fillInterested(this)` to declare again interest for read events, and return `Action.SCHEDULED` since a callback is scheduled to occur once filling is possible. Another possibility is that during `process()` the read returns `-1` indicating that the other peer has closed the connection; this means that there will not be more bytes to read and the loop can be exited, so you return `Action.SUCCEEDED`; `IteratingCallback` will then call `onCompleteSuccess()` where you can close the `EndPoint`. The last case is that during `process()` an exception is thrown, for example by `EndPoint.fill(ByteBuffer)` or, in more advanced implementations, by code that parses the bytes that have been read and finds them unacceptable; any exception thrown within `process()` will be caught by `IteratingCallback` that will exit the loop with a failure and call `onCompleteFailure(Throwable)` with the exception that has been thrown, where you can close the `EndPoint`, passing the exception that is the reason for closing prematurely the `EndPoint`. +Note that some failures may occur whilst a scheduled operation is in progress. Such failures are notified immediately via the `onFailure(Throwable)` method, but care must be taken to not release any resources that may still be in use by the scheduled operation. The `onCompleteFailure(Throwable)` method is called when both a failure has occurred and any scheduled operation has completed. An example of this issue is that a buffer used for a write operation cannot be returned to a pool in `onFailure(Throwable)` as the write may still be progressing. Either the buffer must be removed from the pool in `onFailure(Throwable)` or the release of the buffer deferred until `onCompleteFailure(Throwable)` is called. + [IMPORTANT] ==== Asynchronous programming is hard. @@ -356,9 +357,9 @@ You must initiate a second write only when the first is finished, for example: include::code:example$src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java[tags=sinkMany] ---- -When you need to perform an unknown number of writes, you must use an `IteratingCallback`, explained in <>, to avoid ``StackOverFlowError``s. +When you need to perform an unknown number of writes, you may use an `IteratingCallback`, explained in <>, to avoid ``StackOverFlowError``s. -For example, to copy from a `Content.Source` to a `Content.Sink` you should use the convenience method `Content.copy(Content.Source, Content.Sink, Callback)`. +For example, to copy from a `Content.Source` to a `Content.Sink` you could use the convenience method `Content.copy(Content.Source, Content.Sink, Callback)`. For illustrative purposes, below you can find the implementation of `copy(Content.Source, Content.Sink, Callback)` that uses an `IteratingCallback`: [,java,indent=0] diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java index 3f40f11c3902..97d0831e4def 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java @@ -145,6 +145,20 @@ public void fillInterested() getEndPoint().fillInterested(_readCallback); } + /** + *

Utility method to be called to register read interest.

+ *

After a call to this method, {@link #onFillable()} or {@link #onFillInterestedFailed(Throwable)} + * will be called back as appropriate.

+ * + * @see #onFillable() + */ + public void fillInterested(Callback callback) + { + if (LOG.isDebugEnabled()) + LOG.debug("fillInterested {} {}", callback, this); + getEndPoint().fillInterested(callback); + } + public void tryFillInterested(Callback callback) { getEndPoint().tryFillInterested(callback); From 00b87f12c1556472e007dbb9f7642a1986b5455a Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 6 Aug 2024 17:41:12 +1000 Subject: [PATCH 14/22] WIP --- .../java/org/eclipse/jetty/io/RetainableByteBuffer.java | 2 +- .../java/org/eclipse/jetty/server/handler/ErrorHandler.java | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 18e7a5ffda89..4f95754c5c5b 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -2385,7 +2385,7 @@ protected void onCompleteFailure(Throwable x) // release the last buffer written if (_buffer != null) { - _buffer.releaseForRemoval(); + _buffer.release(); _buffer = null; } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java index f24abc85701a..ef4069e2e8d7 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java @@ -254,7 +254,7 @@ else if (charsets.contains(StandardCharsets.ISO_8859_1)) } response.getHeaders().put(type.getContentTypeField(charset)); - response.write(true, buffer.getByteBuffer(), new WriteErrorCallback(callback, byteBufferPool, buffer)); + response.write(true, buffer.getByteBuffer(), new WriteErrorCallback(callback, buffer)); return true; } @@ -586,13 +586,11 @@ public String toString() private static class WriteErrorCallback implements Callback { private final AtomicReference _callback; - private final ByteBufferPool _pool; private final RetainableByteBuffer _buffer; - public WriteErrorCallback(Callback callback, ByteBufferPool pool, RetainableByteBuffer retainable) + public WriteErrorCallback(Callback callback, RetainableByteBuffer retainable) { _callback = new AtomicReference<>(callback); - _pool = pool; _buffer = retainable; } From e502db0ecbc9253356fbc779bf47a8600b0c3d9e Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 7 Aug 2024 09:33:04 +1000 Subject: [PATCH 15/22] updates from review --- .../jetty/docs/programming/ContentDocs.java | 22 ++++++------ .../docs/programming/SelectorManagerDocs.java | 34 +++++++++---------- .../docs/programming/server/ServerDocs.java | 17 ++++------ .../programming-guide/pages/arch/io.adoc | 6 +++- .../internal/HttpSenderOverHTTP.java | 18 ++++------ .../eclipse/jetty/fcgi/generator/Flusher.java | 8 +++-- .../eclipse/jetty/io/ArrayByteBufferPool.java | 4 +-- .../org/eclipse/jetty/io/ByteBufferPool.java | 8 ++--- .../java/org/eclipse/jetty/io/Content.java | 21 ++++++++++++ .../java/org/eclipse/jetty/io/Retainable.java | 26 ++++++++++++++ .../jetty/io/RetainableByteBuffer.java | 27 +++++++++++---- .../jetty/io/internal/ByteBufferChunk.java | 2 +- .../jetty/io/internal/ContentCopier.java | 25 ++++++-------- .../jetty/io/ArrayByteBufferPoolTest.java | 8 ++--- .../jetty/quic/common/QuicSession.java | 7 ++-- .../jetty/server/handler/ConnectHandler.java | 20 +++++------ .../jetty/server/handler/ErrorHandler.java | 6 ++-- 17 files changed, 157 insertions(+), 102 deletions(-) diff --git a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java index a3b7242aebf8..19321a2eb0c8 100644 --- a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java +++ b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java @@ -329,9 +329,10 @@ protected Action process() throws Throwable // Read a chunk. chunk = source.read(); - // No chunk, demand to be called back when there will be more chunks. + // If no chunk, if (chunk == null) { + // schedule a demand callback when there are more chunks. source.demand(this::succeeded); return Action.SCHEDULED; } @@ -341,7 +342,7 @@ protected Action process() throws Throwable if (Content.Chunk.isFailure(chunk)) throw chunk.getFailure(); - // Copy the chunk. + // Copy the chunk by scheduling an async write sink.write(chunk.isLast(), chunk.getByteBuffer(), this); return Action.SCHEDULED; } @@ -349,11 +350,9 @@ protected Action process() throws Throwable @Override protected void onSuccess() { - // After every successful write, release the chunk. - chunk.release(); - - // Reset the chunk, preserving errors and EOF - chunk = Content.Chunk.next(chunk); + // After every successful write, release the chunk + // and reset to the next chunk + chunk = Content.Chunk.releaseNext(chunk); } @Override @@ -367,16 +366,17 @@ protected void onCompleteSuccess() protected void onFailure(Throwable cause) { // The copy is failed, fail the callback. - // This may occur before a write has completed (due to abort or close), so we cannot release the chunk here. + // This may occur before a {@code write} has completed (due to abort or close), + // so we cannot release the chunk here. callback.failed(cause); } @Override protected void onCompleteFailure(Throwable failure) { - // In case of a failure, we wait until the write has completed before releasing any chunk. - if (chunk != null) - chunk.release(); + // In case of a failure, we wait until here, when the {@code write} + // has completed before releasing any chunk. + chunk = Content.Chunk.releaseNext(chunk); } @Override diff --git a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java index 360ab386e86a..ed8c4cabf2a8 100644 --- a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java +++ b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/SelectorManagerDocs.java @@ -24,9 +24,12 @@ import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.io.AbstractConnection; +import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.ConnectionStatistics; import org.eclipse.jetty.io.EndPoint; +import org.eclipse.jetty.io.Retainable; +import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.io.SelectorManager; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; @@ -225,11 +228,13 @@ public void echoCorrect() // tag::echo-correct[] class EchoConnection extends AbstractConnection { + private final ByteBufferPool.Sized pool; private final IteratingCallback callback = new EchoIteratingCallback(); - public EchoConnection(EndPoint endp, Executor executor) + public EchoConnection(EndPoint endp, ByteBufferPool.Sized pool, Executor executor) { super(endp, executor); + this.pool = pool; } @Override @@ -250,20 +255,20 @@ public void onFillable() class EchoIteratingCallback extends IteratingCallback { - private ByteBuffer buffer; + private RetainableByteBuffer buffer; @Override protected Action process() throws Throwable { // Obtain a buffer if we don't already have one. if (buffer == null) - buffer = BufferUtil.allocate(1024); + buffer = pool.acquire(); - int filled = getEndPoint().fill(buffer); + int filled = getEndPoint().fill(buffer.getByteBuffer()); if (filled > 0) { // We have filled some bytes, echo them back. - getEndPoint().write(this, buffer); + getEndPoint().write(this, buffer.getByteBuffer()); // Signal that the iteration should resume // when the write() operation is completed. @@ -273,14 +278,15 @@ else if (filled == 0) { // We don't need the buffer anymore, so // don't keep it around while we are idle. - buffer = null; + buffer = Retainable.release(buffer); // No more bytes to read, declare // again interest for fill events. fillInterested(this); - // Signal that the iteration is now IDLE. - return Action.IDLE; + // Signal that the iteration is now SCHEDULED + // for a fillable callback. + return Action.SCHEDULED; } else { @@ -291,17 +297,11 @@ else if (filled == 0) } @Override - protected void onCompleteSuccess() - { - // The iteration completed successfully. - getEndPoint().close(); - } - - @Override - protected void onCompleteFailure(Throwable cause) + protected void onCompleted(Throwable cause) { - // The iteration completed with a failure. + // The iteration completed. getEndPoint().close(cause); + buffer = Retainable.release(buffer); } @Override diff --git a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java index de8a00eb5c7a..bac9a1ac7d13 100644 --- a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java +++ b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java @@ -176,7 +176,8 @@ public void onOpen() @Override public void onFillable() { - callback.succeeded(); + // Called from the fill interest in onOpen() to start iteration + callback.iterate(); } private class JSONHTTPIteratingCallback extends IteratingCallback @@ -206,11 +207,8 @@ protected Action process() throws Throwable // the application completed the request processing. return Action.SCHEDULED; } - else - { - // Did not receive enough JSON bytes, - // loop around to try to read more. - } + // Did not receive enough JSON bytes to complete the parser, + // loop around to try to read more. } else if (filled == 0) { @@ -218,11 +216,10 @@ else if (filled == 0) // don't keep it around while we are idle. buffer = null; - // No more bytes to read, declare - // again interest for fill events. - fillInterested(); + // No more bytes to read, declare again interest for fill events. + fillInterested(this); - // Signal that the iteration is now SCHEDULED for a callback to onFillable. + // Signal that the iteration is now SCHEDULED for fill interest callback. return Action.SCHEDULED; } else diff --git a/documentation/jetty/modules/programming-guide/pages/arch/io.adoc b/documentation/jetty/modules/programming-guide/pages/arch/io.adoc index ff746a899af7..d47c348335bb 100644 --- a/documentation/jetty/modules/programming-guide/pages/arch/io.adoc +++ b/documentation/jetty/modules/programming-guide/pages/arch/io.adoc @@ -215,7 +215,11 @@ Another possibility is that during `process()` the read returns `-1` indicating The last case is that during `process()` an exception is thrown, for example by `EndPoint.fill(ByteBuffer)` or, in more advanced implementations, by code that parses the bytes that have been read and finds them unacceptable; any exception thrown within `process()` will be caught by `IteratingCallback` that will exit the loop with a failure and call `onCompleteFailure(Throwable)` with the exception that has been thrown, where you can close the `EndPoint`, passing the exception that is the reason for closing prematurely the `EndPoint`. -Note that some failures may occur whilst a scheduled operation is in progress. Such failures are notified immediately via the `onFailure(Throwable)` method, but care must be taken to not release any resources that may still be in use by the scheduled operation. The `onCompleteFailure(Throwable)` method is called when both a failure has occurred and any scheduled operation has completed. An example of this issue is that a buffer used for a write operation cannot be returned to a pool in `onFailure(Throwable)` as the write may still be progressing. Either the buffer must be removed from the pool in `onFailure(Throwable)` or the release of the buffer deferred until `onCompleteFailure(Throwable)` is called. +Note that some failures may occur whilst a scheduled operation is in progress. +Such failures are notified immediately via the `onFailure(Throwable)` method, but care must be taken to not release any resources that may still be in use by the scheduled operation. +The `onCompleteFailure(Throwable)` method is called when both a failure has occurred and any scheduled operation has completed. +An example of this issue is that a buffer used for a write operation cannot be returned to a pool in `onFailure(Throwable)` as the write may still be progressing. +Either the buffer must be removed from the pool in `onFailure(Throwable)` or the release of the buffer deferred until `onCompleteFailure(Throwable)` is called. [IMPORTANT] ==== diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java index bfb5d9704421..39a7787ace58 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java @@ -26,6 +26,7 @@ import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.EndPoint; +import org.eclipse.jetty.io.Retainable; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; @@ -237,7 +238,9 @@ protected Action process() throws Exception @Override protected void onSuccess() { - release(); + headerBuffer = Retainable.release(headerBuffer); + chunkBuffer = Retainable.release(chunkBuffer); + contentByteBuffer = null; } @Override @@ -256,17 +259,8 @@ protected void onFailure(Throwable cause) @Override protected void onCompleteFailure(Throwable cause) { - release(); - } - - private void release() - { - if (headerBuffer != null) - headerBuffer.release(); - headerBuffer = null; - if (chunkBuffer != null) - chunkBuffer.release(); - chunkBuffer = null; + headerBuffer = Retainable.release(headerBuffer); + chunkBuffer = Retainable.release(chunkBuffer); contentByteBuffer = null; } } diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java index 04b4210194bf..5a5236fcd575 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java @@ -105,16 +105,20 @@ protected void onCompleteSuccess() protected void onSuccess() { if (active != null) + { active.succeeded(); - active = null; + active = null; + } } @Override public void onFailure(Throwable cause) { if (active != null) + { active.failed(cause); - active = null; + active = null; + } List entries; try (AutoLock ignored = lock.lock()) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java index c1f93021864a..38f168180061 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java @@ -224,7 +224,7 @@ public RetainableByteBuffer.Mutable acquire(int size, boolean direct) } @Override - public boolean releaseForRemoval(RetainableByteBuffer buffer) + public boolean releaseAndRemove(RetainableByteBuffer buffer) { RetainableByteBuffer actual = buffer; while (actual instanceof RetainableByteBuffer.Wrapper wrapper) @@ -244,7 +244,7 @@ public boolean releaseForRemoval(RetainableByteBuffer buffer) return buffer.release(); } - return ByteBufferPool.super.releaseForRemoval(buffer); + return ByteBufferPool.super.releaseAndRemove(buffer); } private void reserve(RetainedBucket bucket, ByteBuffer byteBuffer) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java index 5be18db049fe..764f539937c9 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java @@ -63,9 +63,9 @@ public interface ByteBufferPool * If the buffer is not in a pool, calling this method is equivalent to calling {@link RetainableByteBuffer#release()}. * Calling this method satisfies any contract that requires a call to {@link RetainableByteBuffer#release()}. * @return {@code true} if a call to {@link RetainableByteBuffer#release()} would have returned {@code true}. - * @see RetainableByteBuffer#releaseForRemoval() + * @see RetainableByteBuffer#releaseAndRemove() */ - default boolean releaseForRemoval(RetainableByteBuffer buffer) + default boolean releaseAndRemove(RetainableByteBuffer buffer) { return buffer != null && buffer.release(); } @@ -94,9 +94,9 @@ public ByteBufferPool getWrapped() } @Override - public boolean releaseForRemoval(RetainableByteBuffer buffer) + public boolean releaseAndRemove(RetainableByteBuffer buffer) { - return getWrapped().releaseForRemoval(buffer); + return getWrapped().releaseAndRemove(buffer); } @Override diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java index e232a39146a2..03f8bdfbed58 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java @@ -1080,6 +1080,27 @@ static Chunk next(Chunk chunk) return null; } + /** + * Convenience method to release a chunk and return {@link #next(Chunk)}. + * Equivalent to: + * 
{@code
+         *   if (chunk != null)
+         *   {
+         *     chunk.release();
+         *     chunk = Chunk.next(chunk);
+         *   }
+         * }
+ * @param chunk The chunk to release or {@code null} + * @return The {@link #next(Chunk)} chunk; + */ + static Chunk releaseNext(Chunk chunk) + { + if (chunk == null) + return null; + chunk.release(); + return next(chunk); + } + /** * @param chunk The chunk to test for an {@link Chunk#getFailure() failure}. * @return True if the chunk is non-null and {@link Chunk#getFailure() chunk.getError()} returns non-null. diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java index d92bb6edd866..d20f1d2f19d2 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java @@ -247,4 +247,30 @@ public String toString() return String.format("%s@%x[r=%d]", getClass().getSimpleName(), hashCode(), get()); } } + + /** + * Convenience method that replaces code like: + * 
{@code
+     *   if (buffer != null)
+     *   {
+     *       buffer.release();
+     *       buffer = null;
+     *   }
+     * }
+     *
+ * with: + * 
{@code
+     *   buffer = Retainable.release(buffer);
+     * }
+     *
+ * @param retainable The retainable to release, if not {@code null}. + * @param The type of the retainable + * @return always returns {@code null} + */ + static R release(R retainable) + { + if (retainable != null) + retainable.release(); + return null; + } } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 4f95754c5c5b..94b124fbc681 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -163,11 +163,24 @@ default Mutable asMutable() throws ReadOnlyBufferException throw new ReadOnlyBufferException(); } - default boolean releaseForRemoval() + /** + * {@link #release() Release} the buffer in a way that ensures it will not be recycled in a buffer pool. + * This method should be used in cases where it is unclear if operations on the buffer have completed (e.g. when + * a write operation has aborted or timed out). + * @return True if the buffer was released. + * @see ByteBufferPool#releaseAndRemove(RetainableByteBuffer) + */ + default boolean releaseAndRemove() { return release(); } + @Override + default boolean release() + { + return Retainable.super.release(); + } + /** * Appends and consumes the contents of this buffer to the passed buffer, limited by the capacity of the target buffer. * @param buffer The buffer to append bytes to, whose limit will be updated. @@ -662,9 +675,9 @@ public RetainableByteBuffer getWrapped() } @Override - public boolean releaseForRemoval() + public boolean releaseAndRemove() { - return getWrapped().releaseForRemoval(); + return getWrapped().releaseAndRemove(); } @Override @@ -1312,9 +1325,9 @@ protected Pooled(ByteBufferPool pool, ByteBuffer byteBuffer, Retainable retainab } @Override - public boolean releaseForRemoval() + public boolean releaseAndRemove() { - return _pool.releaseForRemoval(this); + return _pool.releaseAndRemove(this); } @Override @@ -1939,14 +1952,14 @@ public boolean release() } @Override - public boolean releaseForRemoval() + public boolean releaseAndRemove() { if (LOG.isDebugEnabled()) LOG.debug("release {}", this); if (super.release()) { for (RetainableByteBuffer buffer : _buffers) - buffer.releaseForRemoval(); + buffer.releaseAndRemove(); _buffers.clear(); _aggregate = null; return true; diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ByteBufferChunk.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ByteBufferChunk.java index a4b824fd7758..334217967df5 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ByteBufferChunk.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ByteBufferChunk.java @@ -145,7 +145,7 @@ public static class WithRetainable extends ByteBufferChunk public WithRetainable(ByteBuffer byteBuffer, boolean last, Retainable retainable) { super(byteBuffer, last); - this.retainable = retainable; + this.retainable = Objects.requireNonNull(retainable); } @Override diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java index 0ffba7557894..82b5c130b248 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java @@ -27,7 +27,7 @@ public class ContentCopier extends IteratingNestedCallback private final Content.Source source; private final Content.Sink sink; private final Content.Chunk.Processor chunkProcessor; - private Content.Chunk current; + private Content.Chunk chunk; private boolean terminated; public ContentCopier(Content.Source source, Content.Sink sink, Content.Chunk.Processor chunkProcessor, Callback callback) @@ -50,34 +50,33 @@ protected Action process() throws Throwable if (terminated) return Action.SUCCEEDED; - current = source.read(); + chunk = source.read(); - if (current == null) + if (chunk == null) { source.demand(this::succeeded); return Action.SCHEDULED; } - if (chunkProcessor != null && chunkProcessor.process(current, this)) + if (chunkProcessor != null && chunkProcessor.process(chunk, this)) return Action.SCHEDULED; - terminated = current.isLast(); + terminated = chunk.isLast(); - if (Content.Chunk.isFailure(current)) + if (Content.Chunk.isFailure(chunk)) { - failed(current.getFailure()); + failed(chunk.getFailure()); return Action.SCHEDULED; } - sink.write(current.isLast(), current.getByteBuffer(), this); + sink.write(chunk.isLast(), chunk.getByteBuffer(), this); return Action.SCHEDULED; } @Override protected void onSuccess() { - super.onSuccess(); - current.release(); + chunk = Content.Chunk.releaseNext(chunk); } @Override @@ -89,10 +88,6 @@ protected void onFailure(Throwable cause) @Override protected void onCompleteFailure(Throwable x) { - ExceptionUtil.callAndThen(x, ignored -> - { - if (current != null) - current.release(); - }, super::onCompleteFailure); + chunk = Content.Chunk.releaseNext(chunk); } } diff --git a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java index 26c8e2c9f5fb..fe6f190c392b 100644 --- a/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java +++ b/jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java @@ -448,7 +448,7 @@ public void testReleaseExcessMemory() } @Test - public void testReleaseForRemoval() + public void testReleaseAndRemove() { ArrayByteBufferPool pool = new ArrayByteBufferPool(); @@ -471,9 +471,9 @@ public void testReleaseForRemoval() retained1 = pool.acquire(1024, false); retained1.retain(); - assertTrue(reserved1.releaseForRemoval()); - assertTrue(acquired1.releaseForRemoval()); - assertFalse(retained1.releaseForRemoval()); + assertTrue(reserved1.releaseAndRemove()); + assertTrue(acquired1.releaseAndRemove()); + assertFalse(retained1.releaseAndRemove()); assertTrue(retained1.release()); assertThat(pool.getHeapByteBufferCount(), is(2L)); diff --git a/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java b/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java index 2d65de86c3ab..c46bc8e00ce3 100644 --- a/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java +++ b/jetty-core/jetty-quic/jetty-quic-common/src/main/java/org/eclipse/jetty/quic/common/QuicSession.java @@ -38,6 +38,7 @@ import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.CyclicTimeout; import org.eclipse.jetty.io.EndPoint; +import org.eclipse.jetty.io.Retainable; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.quic.quiche.QuicheConnection; import org.eclipse.jetty.quic.quiche.QuicheConnectionId; @@ -533,7 +534,7 @@ protected void onSuccess() { if (LOG.isDebugEnabled()) LOG.debug("written cipher bytes on {}", QuicSession.this); - cipherBuffer.release(); + cipherBuffer = Retainable.release(cipherBuffer); } @Override @@ -547,7 +548,7 @@ protected void onCompleteSuccess() { if (LOG.isDebugEnabled()) LOG.debug("connection closed {}", QuicSession.this); - cipherBuffer.release(); + cipherBuffer = Retainable.release(cipherBuffer); finishOutwardClose(new ClosedChannelException()); timeout.destroy(); } @@ -564,7 +565,7 @@ protected void onFailure(Throwable failure) @Override protected void onCompleteFailure(Throwable cause) { - cipherBuffer.release(); + cipherBuffer = Retainable.release(cipherBuffer); } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java index 1d22e479a720..898c629fa2e1 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java @@ -37,6 +37,7 @@ import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.io.ManagedSelector; +import org.eclipse.jetty.io.Retainable; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.io.SelectorManager; import org.eclipse.jetty.io.SocketChannelEndPoint; @@ -736,18 +737,17 @@ protected Action process() write(connection.getEndPoint(), byteBuffer, this); return Action.SCHEDULED; } - else if (filled == 0) + + buffer = Retainable.release(buffer); + + if (filled == 0) { - buffer.release(); fillInterested(); return Action.IDLE; } - else - { - buffer.release(); - connection.getEndPoint().shutdownOutput(); - return Action.SUCCEEDED; - } + + connection.getEndPoint().shutdownOutput(); + return Action.SUCCEEDED; } catch (IOException x) { @@ -764,7 +764,7 @@ protected void onSuccess() { if (LOG.isDebugEnabled()) LOG.debug("Wrote {} bytes {}", filled, TunnelConnection.this); - buffer.release(); + buffer = Retainable.release(buffer); } @Override @@ -778,7 +778,7 @@ protected void onFailure(Throwable x) @Override protected void onCompleteFailure(Throwable cause) { - buffer.release(); + buffer = Retainable.release(buffer); } private void disconnect(Throwable x) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java index ef4069e2e8d7..c6621c90fc79 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java @@ -261,7 +261,7 @@ else if (charsets.contains(StandardCharsets.ISO_8859_1)) catch (Throwable x) { if (buffer != null) - buffer.releaseForRemoval(); + buffer.releaseAndRemove(); throw x; } } @@ -609,9 +609,9 @@ public void failed(Throwable x) { Callback callback = _callback.getAndSet(null); if (callback == null) - _buffer.releaseForRemoval(); + _buffer.releaseAndRemove(); else - ExceptionUtil.callAndThen(x, t -> _buffer.releaseForRemoval(), callback::failed); + ExceptionUtil.callAndThen(x, t -> _buffer.releaseAndRemove(), callback::failed); } } } From 1530cd6c5c24c837f7857b937b8bf03f4b9e4687 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 7 Aug 2024 11:34:35 +1000 Subject: [PATCH 16/22] Fixed FCGI Flusher --- .../org/eclipse/jetty/fcgi/generator/Flusher.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java index 5a5236fcd575..891788aa5c8d 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-client/src/main/java/org/eclipse/jetty/fcgi/generator/Flusher.java @@ -106,6 +106,7 @@ protected void onSuccess() { if (active != null) { + active.release(); active.succeeded(); active = null; } @@ -115,11 +116,7 @@ protected void onSuccess() public void onFailure(Throwable cause) { if (active != null) - { active.failed(cause); - active = null; - } - List entries; try (AutoLock ignored = lock.lock()) { @@ -131,6 +128,11 @@ public void onFailure(Throwable cause) @Override protected void onCompleteFailure(Throwable cause) { + if (active != null) + { + active.release(); + active = null; + } List entries; try (AutoLock ignored = lock.lock()) { @@ -145,7 +147,6 @@ private record Entry(ByteBufferPool.Accumulator accumulator, Callback callback) { public void succeeded() { - release(); callback.succeeded(); } From 7c5c003341418941878937db8c2a047079239ebf Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 8 Aug 2024 07:11:10 +1000 Subject: [PATCH 17/22] updates from review --- .../jetty/docs/programming/ContentDocs.java | 4 ++-- .../java/org/eclipse/jetty/io/Content.java | 2 +- .../eclipse/jetty/io/RetainableByteBuffer.java | 18 ++---------------- .../jetty/io/internal/ContentCopier.java | 4 ++-- .../jetty/server/handler/ConnectHandler.java | 4 ++-- 5 files changed, 9 insertions(+), 23 deletions(-) diff --git a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java index 19321a2eb0c8..915f1ddb6bdb 100644 --- a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java +++ b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java @@ -352,7 +352,7 @@ protected void onSuccess() { // After every successful write, release the chunk // and reset to the next chunk - chunk = Content.Chunk.releaseNext(chunk); + chunk = Content.Chunk.releaseAndNext(chunk); } @Override @@ -376,7 +376,7 @@ protected void onCompleteFailure(Throwable failure) { // In case of a failure, we wait until here, when the {@code write} // has completed before releasing any chunk. - chunk = Content.Chunk.releaseNext(chunk); + chunk = Content.Chunk.releaseAndNext(chunk); } @Override diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java index 68c76a9288be..56ad510f3b40 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java @@ -1093,7 +1093,7 @@ static Chunk next(Chunk chunk) * @param chunk The chunk to release or {@code null} * @return The {@link #next(Chunk)} chunk; */ - static Chunk releaseNext(Chunk chunk) + static Chunk releaseAndNext(Chunk chunk) { if (chunk == null) return null; diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index dd0f82befcca..84bc9769a080 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -176,12 +176,6 @@ default boolean releaseAndRemove() return release(); } - @Override - default boolean release() - { - return Retainable.super.release(); - } - /** * Appends and consumes the contents of this buffer to the passed buffer, limited by the capacity of the target buffer. * @param buffer The buffer to append bytes to, whose limit will be updated. @@ -2404,22 +2398,14 @@ protected Action process() protected void onSuccess() { // release the last buffer written - if (_buffer != null) - { - _buffer.release(); - _buffer = null; - } + _buffer = Retainable.release(_buffer); } @Override protected void onCompleteFailure(Throwable x) { // release the last buffer written - if (_buffer != null) - { - _buffer.release(); - _buffer = null; - } + _buffer = Retainable.release(_buffer); } }.iterate(); } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java index 82b5c130b248..abc14bb5673f 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java @@ -76,7 +76,7 @@ protected Action process() throws Throwable @Override protected void onSuccess() { - chunk = Content.Chunk.releaseNext(chunk); + chunk = Content.Chunk.releaseAndNext(chunk); } @Override @@ -88,6 +88,6 @@ protected void onFailure(Throwable cause) @Override protected void onCompleteFailure(Throwable x) { - chunk = Content.Chunk.releaseNext(chunk); + chunk = Content.Chunk.releaseAndNext(chunk); } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java index 898c629fa2e1..985cd618caf4 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ConnectHandler.java @@ -742,8 +742,8 @@ protected Action process() if (filled == 0) { - fillInterested(); - return Action.IDLE; + fillInterested(this); + return Action.SCHEDULED; } connection.getEndPoint().shutdownOutput(); From 7f81e62a3f6c2f0e9fcef1be8a58c6cadc979d75 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 12 Aug 2024 14:36:21 +0200 Subject: [PATCH 18/22] fix reported leak error messages Signed-off-by: Ludovic Orban --- .../jetty/test/client/transport/AbstractTest.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 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 b51f938ea72f..24b8289d2709 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.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.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.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) @@ -129,9 +130,9 @@ public void dispose(TestInfo testInfo) throws Exception try { if (serverBufferPool != null && !isLeakTrackingDisabled(testInfo, "server")) - assertNoLeaks(serverBufferPool, testInfo, "server-", "\n---\nServer Leaks: " + serverBufferPool.dumpLeaks() + "---\n"); + assertNoLeaks(serverBufferPool, testInfo, "server-", "Server Leaks: "); if (clientBufferPool != null && !isLeakTrackingDisabled(testInfo, "client")) - assertNoLeaks(clientBufferPool, testInfo, "client-", "\n---\nClient Leaks: " + clientBufferPool.dumpLeaks() + "---\n"); + assertNoLeaks(clientBufferPool, testInfo, "client-", "Client Leaks: "); } finally { @@ -149,7 +150,7 @@ private void assertNoLeaks(ArrayByteBufferPool.Tracking bufferPool, TestInfo tes { try { - Awaitility.await().atMost(3, TimeUnit.SECONDS).until(() -> bufferPool.getLeaks().size(), Matchers.is(0)); + await().atMost(3, TimeUnit.SECONDS).untilAsserted(() -> assertThat("\n---\n" + msg + bufferPool.dumpLeaks(), bufferPool.getLeaks().size(), is(0))); } catch (Exception e) { From 9bce4fcc8c868aa0645a0fad81146e49c15ef0b8 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 12 Aug 2024 14:36:36 +0200 Subject: [PATCH 19/22] fix leak Signed-off-by: Ludovic Orban --- .../jetty/fcgi/server/internal/ServerFCGIConnection.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/ServerFCGIConnection.java b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/ServerFCGIConnection.java index fdfbcf0d6781..ba51999d4ff1 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/ServerFCGIConnection.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/ServerFCGIConnection.java @@ -259,8 +259,7 @@ private void releaseInputBuffer() boolean released = inputBuffer.release(); if (LOG.isDebugEnabled()) LOG.debug("releaseInputBuffer {} {}", released, this); - if (released) - inputBuffer = null; + inputBuffer = null; } private int fillInputBuffer() From be283c45d247997f410a36501ca1c6887ebda441 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 13 Aug 2024 11:58:07 +0200 Subject: [PATCH 20/22] fix missing calls to super Signed-off-by: Ludovic Orban --- .../main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java index 9ef1e57b556b..c6629ba20321 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpOutput.java @@ -1855,6 +1855,7 @@ protected Action process() throws Exception @Override protected void onCompleteSuccess() { + super.onCompleteSuccess(); _buffer.release(); IO.close(_in); } @@ -1862,12 +1863,14 @@ protected void onCompleteSuccess() @Override protected void onFailure(Throwable cause) { + super.onFailure(cause); IO.close(_in); } @Override public void onCompleteFailure(Throwable x) { + super.onCompleteFailure(x); _buffer.release(); } } @@ -1929,6 +1932,7 @@ protected Action process() throws Exception @Override protected void onCompleteSuccess() { + super.onCompleteSuccess(); _buffer.release(); IO.close(_in); } @@ -1936,12 +1940,14 @@ protected void onCompleteSuccess() @Override protected void onFailure(Throwable cause) { + super.onFailure(cause); IO.close(_in); } @Override public void onCompleteFailure(Throwable x) { + super.onCompleteFailure(x); _buffer.release(); } } From 6028353e5b8ac0509a26ccbec4684dd68120091b Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 16 Aug 2024 16:11:29 +0200 Subject: [PATCH 21/22] Small tweaks to documentation and javadocs. Signed-off-by: Simone Bordet --- .../eclipse/jetty/docs/programming/ContentDocs.java | 13 ++++++------- .../jetty/docs/programming/WebSocketDocs.java | 8 ++++---- .../jetty/docs/programming/server/ServerDocs.java | 6 +++--- .../src/main/java/org/eclipse/jetty/io/Content.java | 6 +++--- .../org/eclipse/jetty/io/RetainableByteBuffer.java | 9 +++++---- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java index 915f1ddb6bdb..df5d8d6b6692 100644 --- a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java +++ b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/ContentDocs.java @@ -329,10 +329,9 @@ protected Action process() throws Throwable // Read a chunk. chunk = source.read(); - // If no chunk, + // If no chunk, schedule a demand callback when there are more chunks. if (chunk == null) { - // schedule a demand callback when there are more chunks. source.demand(this::succeeded); return Action.SCHEDULED; } @@ -342,7 +341,7 @@ protected Action process() throws Throwable if (Content.Chunk.isFailure(chunk)) throw chunk.getFailure(); - // Copy the chunk by scheduling an async write + // Copy the chunk by scheduling an asynchronous write. sink.write(chunk.isLast(), chunk.getByteBuffer(), this); return Action.SCHEDULED; } @@ -366,16 +365,16 @@ protected void onCompleteSuccess() protected void onFailure(Throwable cause) { // The copy is failed, fail the callback. - // This may occur before a {@code write} has completed (due to abort or close), - // so we cannot release the chunk here. + // This method is invoked before a write() has completed, so + // the chunk is not released here, but in onCompleteFailure(). callback.failed(cause); } @Override protected void onCompleteFailure(Throwable failure) { - // In case of a failure, we wait until here, when the {@code write} - // has completed before releasing any chunk. + // In case of a failure, this method is invoked when the write() + // is completed, and it is now possible to release the chunk. chunk = Content.Chunk.releaseAndNext(chunk); } diff --git a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java index d89fcf6a487e..eee7352a63fa 100644 --- a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java +++ b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/WebSocketDocs.java @@ -523,16 +523,16 @@ protected Action process() throws Throwable // <2> @Override public void succeed() { - // Map the o.e.j.websocket.api.Callback to o.e.jetty.util.Callback API - // When the send succeeds, succeed this IteratingCallback. + // Map the o.e.j.websocket.api.Callback to o.e.j.util.Callback. + // When the send() succeeds, succeed this IteratingCallback. succeeded(); } @Override public void fail(Throwable x) { - // Map the o.e.j.websocket.api.Callback to o.e.jetty.util.Callback API - // When the send fails, fail this IteratingCallback. + // Map the o.e.j.websocket.api.Callback to o.e.j.util.Callback. + // When the send() fails, fail this IteratingCallback. failed(x); } diff --git a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java index bac9a1ac7d13..b147d5eff245 100644 --- a/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java +++ b/documentation/jetty/modules/code/examples/src/main/java/org/eclipse/jetty/docs/programming/server/ServerDocs.java @@ -176,7 +176,7 @@ public void onOpen() @Override public void onFillable() { - // Called from the fill interest in onOpen() to start iteration + // Called from fillInterested() in onOpen() to start iteration. callback.iterate(); } @@ -207,8 +207,8 @@ protected Action process() throws Throwable // the application completed the request processing. return Action.SCHEDULED; } - // Did not receive enough JSON bytes to complete the parser, - // loop around to try to read more. + // Did not receive enough JSON bytes to complete the + // JSON parsing, loop around to try to read more bytes. } else if (filled == 0) { diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java index 56ad510f3b40..be0aeb452b0f 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java @@ -1084,11 +1084,11 @@ static Chunk next(Chunk chunk) * Convenience method to release a chunk and return {@link #next(Chunk)}. * Equivalent to: * 
{@code
-         *   if (chunk != null)
-         *   {
+         * if (chunk != null)
+         * {
          *     chunk.release();
          *     chunk = Chunk.next(chunk);
-         *   }
+         * }
          * }
* @param chunk The chunk to release or {@code null} * @return The {@link #next(Chunk)} chunk; diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java index 84bc9769a080..8c6cc2fd0c13 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java @@ -165,10 +165,11 @@ default Mutable asMutable() throws ReadOnlyBufferException } /** - * {@link #release() Release} the buffer in a way that ensures it will not be recycled in a buffer pool. - * This method should be used in cases where it is unclear if operations on the buffer have completed (e.g. when - * a write operation has aborted or timed out). - * @return True if the buffer was released. + * {@link #release() Releases} the buffer in a way that ensures it will not be recycled in a buffer pool. + * This method should be used in cases where it is unclear if operations on the buffer have completed + * (for example, when a write operation has been aborted asynchronously or timed out, but the write + * operation may still be pending). + * @return whether if the buffer was released. * @see ByteBufferPool#releaseAndRemove(RetainableByteBuffer) */ default boolean releaseAndRemove() From 86b14421deffadede6816a6f577df1c73bd077b5 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 22 Aug 2024 19:32:30 +1000 Subject: [PATCH 22/22] deflake test --- .../org/eclipse/jetty/ee9/servlet/ContextScopeListenerTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ContextScopeListenerTest.java b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ContextScopeListenerTest.java index ba36b7666061..569a5f9c7f5a 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ContextScopeListenerTest.java +++ b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ContextScopeListenerTest.java @@ -103,6 +103,7 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) { throw new RuntimeException(e); } + Awaitility.waitAtMost(5, TimeUnit.SECONDS).until(() -> _history.size() == 5); } }), "/");