From 657222f48d45b7258967d1abd7ec3674e259dbf7 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 10 Jun 2022 17:43:41 +1000 Subject: [PATCH 1/5] Use static exceptions for closing websocket flushers. Signed-off-by: Lachlan Roberts --- .../SentinelWebSocketCloseException.java | 32 +++++++++++++++++++ .../core/internal/DemandingFlusher.java | 15 +++++++++ .../websocket/core/internal/FrameFlusher.java | 12 ++----- .../internal/PerMessageDeflateExtension.java | 14 ++------ .../core/internal/TransformingFlusher.java | 11 ++++++- 5 files changed, 62 insertions(+), 22 deletions(-) create mode 100644 jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/exception/SentinelWebSocketCloseException.java diff --git a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/exception/SentinelWebSocketCloseException.java b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/exception/SentinelWebSocketCloseException.java new file mode 100644 index 000000000000..7f9b683d0ba1 --- /dev/null +++ b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/exception/SentinelWebSocketCloseException.java @@ -0,0 +1,32 @@ +// +// ======================================================================== +// Copyright (c) 1995-2022 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.websocket.core.exception; + +/** + * Suppressed exceptions are disabled for this implementation, + * meaning calling {@link #addSuppressed(Throwable)} has no effect. + * This means instances of {@link SentinelWebSocketCloseException} are suitable to be kept as static fields. + */ +public class SentinelWebSocketCloseException extends Exception +{ + public SentinelWebSocketCloseException() + { + this(null); + } + + public SentinelWebSocketCloseException(String message) + { + super(message, null, false, true); + } +} diff --git a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/DemandingFlusher.java b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/DemandingFlusher.java index 3c14c25d915b..49b0eba16106 100644 --- a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/DemandingFlusher.java +++ b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/DemandingFlusher.java @@ -23,6 +23,7 @@ import org.eclipse.jetty.websocket.core.Extension; import org.eclipse.jetty.websocket.core.Frame; import org.eclipse.jetty.websocket.core.IncomingFrames; +import org.eclipse.jetty.websocket.core.exception.SentinelWebSocketCloseException; /** *

This flusher can be used to mutated and fragment {@link Frame}s and forwarded them on towards the application using the @@ -38,6 +39,8 @@ */ public abstract class DemandingFlusher extends IteratingCallback implements DemandChain { + private static final Throwable SENTINEL_CLOSE_EXCEPTION = new SentinelWebSocketCloseException(); + private final IncomingFrames _emitFrame; private final AtomicLong _demand = new AtomicLong(); private final AtomicReference _failure = new AtomicReference<>(); @@ -101,6 +104,18 @@ public void onFrame(Frame frame, Callback callback) succeeded(); } + /** + * Used to close this flusher when there is no explicit failure. + */ + public void closeFlusher() + { + if (_failure.compareAndSet(null, SENTINEL_CLOSE_EXCEPTION)) + { + failed(SENTINEL_CLOSE_EXCEPTION); + iterate(); + } + } + /** * Used to fail this flusher possibly from an external event such as a callback. * @param t the failure. diff --git a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java index e157ce65177b..1bd3fab65ae6 100644 --- a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java +++ b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java @@ -35,6 +35,7 @@ import org.eclipse.jetty.websocket.core.CloseStatus; import org.eclipse.jetty.websocket.core.Frame; import org.eclipse.jetty.websocket.core.OpCode; +import org.eclipse.jetty.websocket.core.exception.SentinelWebSocketCloseException; import org.eclipse.jetty.websocket.core.exception.WebSocketException; import org.eclipse.jetty.websocket.core.exception.WebSocketWriteTimeoutException; import org.slf4j.Logger; @@ -44,6 +45,7 @@ public class FrameFlusher extends IteratingCallback { public static final Frame FLUSH_FRAME = new Frame(OpCode.BINARY); private static final Logger LOG = LoggerFactory.getLogger(FrameFlusher.class); + private static final Throwable CLOSED_CHANNEL = new SentinelWebSocketCloseException(); private final AutoLock lock = new AutoLock(); private final LongAdder messagesOut = new LongAdder(); @@ -184,15 +186,7 @@ public void onClose(Throwable cause) { try (AutoLock l = lock.lock()) { - // TODO: find a way to not create exception if cause is null. - closedCause = cause == null ? new ClosedChannelException() - { - @Override - public Throwable fillInStackTrace() - { - return this; - } - } : cause; + closedCause = cause == null ? CLOSED_CHANNEL : cause; } iterate(); } diff --git a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java index a3aa23628ea2..beac35ad4596 100644 --- a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java +++ b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/PerMessageDeflateExtension.java @@ -14,7 +14,6 @@ package org.eclipse.jetty.websocket.core.internal; import java.nio.ByteBuffer; -import java.nio.channels.ClosedChannelException; import java.util.HashMap; import java.util.Map; import java.util.concurrent.atomic.AtomicReference; @@ -150,17 +149,8 @@ public void init(final ExtensionConfig config, WebSocketComponents components) @Override public void close() { - // TODO: use IteratingCallback.close() instead of creating exception with failFlusher methods. - ClosedChannelException exception = new ClosedChannelException() - { - @Override - public Throwable fillInStackTrace() - { - return this; - } - }; - incomingFlusher.failFlusher(exception); - outgoingFlusher.failFlusher(exception); + incomingFlusher.closeFlusher(); + outgoingFlusher.closeFlusher(); releaseInflater(); releaseDeflater(); } diff --git a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/TransformingFlusher.java b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/TransformingFlusher.java index 5bed90a3d448..1ad545984a89 100644 --- a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/TransformingFlusher.java +++ b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/TransformingFlusher.java @@ -20,6 +20,7 @@ import org.eclipse.jetty.util.IteratingCallback; import org.eclipse.jetty.util.thread.AutoLock; import org.eclipse.jetty.websocket.core.Frame; +import org.eclipse.jetty.websocket.core.exception.SentinelWebSocketCloseException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -33,6 +34,7 @@ public abstract class TransformingFlusher { private final Logger log = LoggerFactory.getLogger(this.getClass()); + private static final Throwable SENTINEL_CLOSE_EXCEPTION = new SentinelWebSocketCloseException(); private final AutoLock lock = new AutoLock(); private final Queue entries = new ArrayDeque<>(); @@ -77,13 +79,20 @@ public final void sendFrame(Frame frame, Callback callback, boolean batch) notifyCallbackFailure(callback, failure); } + /** + * Used to close this flusher when there is no explicit failure. + */ + public void closeFlusher() + { + failFlusher(SENTINEL_CLOSE_EXCEPTION); + } + /** * Used to fail this flusher possibly from an external event such as a callback. * @param t the failure. */ public void failFlusher(Throwable t) { - // TODO: find a way to close the flusher in non error case without exception. boolean failed = false; try (AutoLock l = lock.lock()) { From 805c8ee35bb9f6c5ef9bbfc54d99daf3210c98b9 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 10 Jun 2022 19:13:55 +1000 Subject: [PATCH 2/5] Use StaticException class in jetty-util for websocket flushers. Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/util/StaticException.java | 16 +++++++++------- .../core/internal/DemandingFlusher.java | 4 ++-- .../websocket/core/internal/FrameFlusher.java | 4 ++-- .../core/internal/TransformingFlusher.java | 4 ++-- 4 files changed, 15 insertions(+), 13 deletions(-) rename jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/exception/SentinelWebSocketCloseException.java => jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java (61%) diff --git a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/exception/SentinelWebSocketCloseException.java b/jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java similarity index 61% rename from jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/exception/SentinelWebSocketCloseException.java rename to jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java index 7f9b683d0ba1..699442bc742c 100644 --- a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/exception/SentinelWebSocketCloseException.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java @@ -11,22 +11,24 @@ // ======================================================================== // -package org.eclipse.jetty.websocket.core.exception; +package org.eclipse.jetty.util; /** - * Suppressed exceptions are disabled for this implementation, + * This exception can safely be stored in a static variable as suppressed exceptions are disabled, * meaning calling {@link #addSuppressed(Throwable)} has no effect. - * This means instances of {@link SentinelWebSocketCloseException} are suitable to be kept as static fields. + * This prevents potential memory leaks where a statically-stored exception would accumulate + * suppressed exceptions added to them. */ -public class SentinelWebSocketCloseException extends Exception +public class StaticException extends Exception { - public SentinelWebSocketCloseException() + public StaticException() { this(null); } - public SentinelWebSocketCloseException(String message) + public StaticException(String message) { + // Make sure to call the super constructor that disables suppressed exception. super(message, null, false, true); } -} +} \ No newline at end of file diff --git a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/DemandingFlusher.java b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/DemandingFlusher.java index 49b0eba16106..e14ec84451bd 100644 --- a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/DemandingFlusher.java +++ b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/DemandingFlusher.java @@ -20,10 +20,10 @@ import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.CountingCallback; import org.eclipse.jetty.util.IteratingCallback; +import org.eclipse.jetty.util.StaticException; import org.eclipse.jetty.websocket.core.Extension; import org.eclipse.jetty.websocket.core.Frame; import org.eclipse.jetty.websocket.core.IncomingFrames; -import org.eclipse.jetty.websocket.core.exception.SentinelWebSocketCloseException; /** *

This flusher can be used to mutated and fragment {@link Frame}s and forwarded them on towards the application using the @@ -39,7 +39,7 @@ */ public abstract class DemandingFlusher extends IteratingCallback implements DemandChain { - private static final Throwable SENTINEL_CLOSE_EXCEPTION = new SentinelWebSocketCloseException(); + private static final Throwable SENTINEL_CLOSE_EXCEPTION = new StaticException("Closed"); private final IncomingFrames _emitFrame; private final AtomicLong _demand = new AtomicLong(); diff --git a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java index 1bd3fab65ae6..0f2199375c4e 100644 --- a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java +++ b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/FrameFlusher.java @@ -29,13 +29,13 @@ import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.IteratingCallback; +import org.eclipse.jetty.util.StaticException; import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.thread.AutoLock; import org.eclipse.jetty.util.thread.Scheduler; import org.eclipse.jetty.websocket.core.CloseStatus; import org.eclipse.jetty.websocket.core.Frame; import org.eclipse.jetty.websocket.core.OpCode; -import org.eclipse.jetty.websocket.core.exception.SentinelWebSocketCloseException; import org.eclipse.jetty.websocket.core.exception.WebSocketException; import org.eclipse.jetty.websocket.core.exception.WebSocketWriteTimeoutException; import org.slf4j.Logger; @@ -45,7 +45,7 @@ public class FrameFlusher extends IteratingCallback { public static final Frame FLUSH_FRAME = new Frame(OpCode.BINARY); private static final Logger LOG = LoggerFactory.getLogger(FrameFlusher.class); - private static final Throwable CLOSED_CHANNEL = new SentinelWebSocketCloseException(); + private static final Throwable CLOSED_CHANNEL = new StaticException("Closed"); private final AutoLock lock = new AutoLock(); private final LongAdder messagesOut = new LongAdder(); diff --git a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/TransformingFlusher.java b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/TransformingFlusher.java index 1ad545984a89..4792cd434703 100644 --- a/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/TransformingFlusher.java +++ b/jetty-websocket/websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/TransformingFlusher.java @@ -18,9 +18,9 @@ import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.IteratingCallback; +import org.eclipse.jetty.util.StaticException; import org.eclipse.jetty.util.thread.AutoLock; import org.eclipse.jetty.websocket.core.Frame; -import org.eclipse.jetty.websocket.core.exception.SentinelWebSocketCloseException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -34,7 +34,7 @@ public abstract class TransformingFlusher { private final Logger log = LoggerFactory.getLogger(this.getClass()); - private static final Throwable SENTINEL_CLOSE_EXCEPTION = new SentinelWebSocketCloseException(); + private static final Throwable SENTINEL_CLOSE_EXCEPTION = new StaticException("Closed"); private final AutoLock lock = new AutoLock(); private final Queue entries = new ArrayDeque<>(); From d144c54a1f95d7570931fdb759539b4ce14f52d8 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 10 Jun 2022 11:54:12 +0200 Subject: [PATCH 3/5] Use StaticException class for ContentProducer recycle and consumeAll Signed-off-by: Ludovic Orban --- .../org/eclipse/jetty/server/AsyncContentProducer.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java index 0f854124a100..89dcea25e83d 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java @@ -19,6 +19,7 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.util.StaticException; import org.eclipse.jetty.util.component.Destroyable; import org.eclipse.jetty.util.thread.AutoLock; import org.slf4j.Logger; @@ -31,8 +32,8 @@ class AsyncContentProducer implements ContentProducer { private static final Logger LOG = LoggerFactory.getLogger(AsyncContentProducer.class); - private static final HttpInput.ErrorContent RECYCLED_ERROR_CONTENT = new HttpInput.ErrorContent(new IllegalStateException("ContentProducer has been recycled")); - private static final Throwable UNCONSUMED_CONTENT_EXCEPTION = new IOException("Unconsumed content") + private static final HttpInput.ErrorContent RECYCLED_ERROR_CONTENT = new HttpInput.ErrorContent(new StaticException("ContentProducer has been recycled")); + private static final Throwable UNCONSUMED_CONTENT_EXCEPTION = new StaticException("Unconsumed content") { @Override public Throwable fillInStackTrace() @@ -190,7 +191,7 @@ public boolean consumeAll() Throwable x = UNCONSUMED_CONTENT_EXCEPTION; if (LOG.isDebugEnabled()) { - x = new IOException("Unconsumed content"); + x = new StaticException("Unconsumed content"); LOG.debug("consumeAll {}", this, x); } failCurrentContent(x); From e5e31f21a1a2365614f906fc696c3c2bb8c58279 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 13 Jun 2022 09:19:23 +0200 Subject: [PATCH 4/5] handle review comments Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/server/AsyncContentProducer.java | 4 ++-- .../src/main/java/org/eclipse/jetty/util/StaticException.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java index 89dcea25e83d..3f562da83071 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java @@ -189,10 +189,10 @@ public boolean consumeAll() { assertLocked(); Throwable x = UNCONSUMED_CONTENT_EXCEPTION; - if (LOG.isDebugEnabled()) + if (LOG.isTraceEnabled()) { x = new StaticException("Unconsumed content"); - LOG.debug("consumeAll {}", this, x); + LOG.trace("consumeAll {}", this, x); } failCurrentContent(x); // A specific HttpChannel mechanism must be used as the following code diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java b/jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java index 699442bc742c..356426f2eb1b 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java @@ -31,4 +31,4 @@ public StaticException(String message) // Make sure to call the super constructor that disables suppressed exception. super(message, null, false, true); } -} \ No newline at end of file +} From f45970d113774a5c20a1eede47927a3529e0f20b Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 13 Jun 2022 11:16:58 +0200 Subject: [PATCH 5/5] change StaticException so that it does not collect a stack trace by default Signed-off-by: Ludovic Orban --- .../jetty/server/AsyncContentProducer.java | 11 ++------- .../eclipse/jetty/util/StaticException.java | 23 +++++++++++++++---- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java index 3f562da83071..6bce49c378c8 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java @@ -33,14 +33,7 @@ class AsyncContentProducer implements ContentProducer { private static final Logger LOG = LoggerFactory.getLogger(AsyncContentProducer.class); private static final HttpInput.ErrorContent RECYCLED_ERROR_CONTENT = new HttpInput.ErrorContent(new StaticException("ContentProducer has been recycled")); - private static final Throwable UNCONSUMED_CONTENT_EXCEPTION = new StaticException("Unconsumed content") - { - @Override - public Throwable fillInStackTrace() - { - return this; - } - }; + private static final Throwable UNCONSUMED_CONTENT_EXCEPTION = new StaticException("Unconsumed content"); private final AutoLock _lock = new AutoLock(); private final HttpChannel _httpChannel; @@ -191,7 +184,7 @@ public boolean consumeAll() Throwable x = UNCONSUMED_CONTENT_EXCEPTION; if (LOG.isTraceEnabled()) { - x = new StaticException("Unconsumed content"); + x = new StaticException("Unconsumed content", true); LOG.trace("consumeAll {}", this, x); } failCurrentContent(x); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java b/jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java index 356426f2eb1b..0af3a506cd4f 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java @@ -21,14 +21,29 @@ */ public class StaticException extends Exception { - public StaticException() + /** + * Create an instance with writable stack trace and suppression disabled. + * + * @param message – the detail message + * + * @see Throwable#Throwable(String, Throwable, boolean, boolean) + */ + public StaticException(String message) { - this(null); + this(message, false); } - public StaticException(String message) + /** + * Create an instance with suppression disabled. + * + * @param message – the detail message + * @param writableStackTrace whether or not the stack trace should be writable + * + * @see Throwable#Throwable(String, Throwable, boolean, boolean) + */ + public StaticException(String message, boolean writableStackTrace) { // Make sure to call the super constructor that disables suppressed exception. - super(message, null, false, true); + super(message, null, false, writableStackTrace); } }