From 846726aeb8d64f57b3d59f6ce76479033fa7a7dd Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 22 Feb 2024 09:53:10 +0100 Subject: [PATCH 1/4] Add suppressed failures in failed If an exception is thrown during failure handling, then record the original failure as a suppressed Throwable on the thrown exception --- .../server/internal/HttpChannelState.java | 89 +++++++++------ .../java/org/eclipse/jetty/util/Callback.java | 106 +++++++++++++++--- .../jetty/ee10/servlet/ServletChannel.java | 21 +++- 3 files changed, 155 insertions(+), 61 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index f12990c68aa1..9e71dd430083 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -1114,7 +1114,7 @@ private Runnable lockedFailWrite(Throwable x, boolean fatal) } if (writeCallback == null) return null; - return () -> writeCallback.failed(x); + return () -> Callback.failed(writeCallback, x); } public long getContentBytesWritten() @@ -1229,7 +1229,7 @@ else if (last && !(totalWritten == 0 && HttpMethod.HEAD.is(_request.getMethod()) if (writeFailure != null) { Throwable failure = writeFailure; - httpChannelState._writeInvoker.run(() -> callback.failed(failure)); + httpChannelState._writeInvoker.run(() -> Callback.failed(callback, failure)); return; } @@ -1297,7 +1297,7 @@ public void failed(Throwable x) httpChannel.lockedStreamSendCompleted(false); } if (callback != null) - httpChannel._writeInvoker.run(() -> callback.failed(x)); + httpChannel._writeInvoker.run(() -> Callback.failed(callback, x)); } @Override @@ -1513,41 +1513,49 @@ else if (LOG.isDebugEnabled()) @Override public void failed(Throwable failure) { - // Called when the request/response cycle is completing with a failure. - HttpStream stream; - ChannelRequest request; - HttpChannelState httpChannelState; - ErrorResponse errorResponse = null; - try (AutoLock ignored = _request._lock.lock()) + try { - if (lockedCompleteCallback()) - return; - httpChannelState = _request._httpChannelState; - stream = httpChannelState._stream; - request = _request; + // Called when the request/response cycle is completing with a failure. + HttpStream stream; + ChannelRequest request; + HttpChannelState httpChannelState; + ErrorResponse errorResponse = null; + try (AutoLock ignored = _request._lock.lock()) + { + if (lockedCompleteCallback()) + return; + httpChannelState = _request._httpChannelState; + stream = httpChannelState._stream; + request = _request; - assert httpChannelState._callbackFailure == null; + assert httpChannelState._callbackFailure == null; - httpChannelState._callbackFailure = failure; + httpChannelState._callbackFailure = failure; - // Consume any input. - Throwable unconsumed = stream.consumeAvailable(); - ExceptionUtil.addSuppressedIfNotAssociated(failure, unconsumed); + // Consume any input. + Throwable unconsumed = stream.consumeAvailable(); + ExceptionUtil.addSuppressedIfNotAssociated(failure, unconsumed); - ChannelResponse response = httpChannelState._response; - if (LOG.isDebugEnabled()) - LOG.debug("failed stream.isCommitted={}, response.isCommitted={} {}", stream.isCommitted(), response.isCommitted(), this); + ChannelResponse response = httpChannelState._response; + if (LOG.isDebugEnabled()) + LOG.debug("failed stream.isCommitted={}, response.isCommitted={} {}", stream.isCommitted(), response.isCommitted(), this); - // There may have been an attempt to write an error response that failed. - // Do not try to write again an error response if already committed. - if (!stream.isCommitted()) - errorResponse = new ErrorResponse(request); - } + // There may have been an attempt to write an error response that failed. + // Do not try to write again an error response if already committed. + if (!stream.isCommitted()) + errorResponse = new ErrorResponse(request); + } - if (errorResponse != null) - Response.writeError(request, errorResponse, new ErrorCallback(request, errorResponse, stream, failure), failure); - else - _request.getHttpChannelState()._handlerInvoker.failed(failure); + if (errorResponse != null) + Response.writeError(request, errorResponse, new ErrorCallback(request, errorResponse, stream, failure), failure); + else + _request.getHttpChannelState()._handlerInvoker.failed(failure); + } + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(t, failure); + throw t; + } } private boolean lockedCompleteCallback() @@ -1683,7 +1691,7 @@ public void succeeded() } else { - httpChannelState._handlerInvoker.failed(failure); + Callback.failed(httpChannelState._handlerInvoker, failure); } } @@ -1705,9 +1713,8 @@ public void failed(Throwable x) httpChannelState = _request.lockedGetHttpChannelState(); httpChannelState._response._status = _errorResponse._status; } - if (ExceptionUtil.areNotAssociated(failure, x)) - failure.addSuppressed(x); - httpChannelState._handlerInvoker.failed(failure); + ExceptionUtil.addSuppressedIfNotAssociated(failure, x); + Callback.failed(httpChannelState._handlerInvoker, failure); } @Override @@ -1736,8 +1743,16 @@ public void succeeded() @Override public void failed(Throwable x) { - completing(); - super.failed(x); + try + { + completing(); + super.failed(x); + } + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(t, x); + throw t; + } } private void completing() diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java index d394ab4c0d91..cb45210f141c 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java @@ -121,7 +121,15 @@ public void succeeded() @Override public void failed(Throwable x) { - completable.completeExceptionally(x); + try + { + completable.completeExceptionally(x); + } + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(t, x); + throw t; + } } @Override @@ -165,7 +173,15 @@ public void succeeded() @Override public void failed(Throwable x) { - failure.accept(x); + try + { + failure.accept(x); + } + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(t, x); + throw t; + } } @Override @@ -276,10 +292,19 @@ public void failed(Throwable x) { callback.failed(x); } - finally + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(x, t); + } + try { completed.accept(x); } + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(t, x); + throw t; + } } }; } @@ -302,13 +327,14 @@ public void succeeded() { try { - completed.run(); callback.succeeded(); + completed.run(); } catch (Throwable t) { - callback.failed(t); + Callback.failed(callback, t); } + callback.succeeded(); } @Override @@ -320,9 +346,9 @@ public void failed(Throwable x) } catch (Throwable t) { - x.addSuppressed(t); + ExceptionUtil.addSuppressedIfNotAssociated(x, t); } - callback.failed(x); + Callback.failed(callback, x); } }; } @@ -337,6 +363,7 @@ public void failed(Throwable x) */ static Callback from(Callback callback, Throwable cause) { + callback.succeeded(); return new Callback() { @Override @@ -348,8 +375,16 @@ public void succeeded() @Override public void failed(Throwable x) { - cause.addSuppressed(x); - callback.failed(cause); + try + { + cause.addSuppressed(x); + callback.failed(cause); + } + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(t, x); + throw t; + } } }; } @@ -381,7 +416,15 @@ default void succeeded() @Override default void failed(Throwable x) { - completed(); + try + { + completed(); + } + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(t, x); + throw t; + } } } @@ -426,11 +469,19 @@ public void failed(Throwable x) { try { - callback.failed(x); + try + { + callback.failed(x); + } + finally + { + completed(); + } } - finally + catch (Throwable t) { - completed(); + ExceptionUtil.addSuppressedIfNotAssociated(t, x); + throw t; } } @@ -480,10 +531,8 @@ public void failed(Throwable x) { ExceptionUtil.addSuppressedIfNotAssociated(x, t); } - finally - { - cb2.failed(x); - } + + Callback.failed(cb2, x); } @Override @@ -533,7 +582,7 @@ public void succeeded() @Override public void failed(Throwable x) { - callback.failed(x); + Callback.failed(callback, x); super.failed(x); } }; @@ -592,4 +641,25 @@ public Completable compose(Consumer consumer) return completable; } } + + /** + * Invoke a callback failure, handling any {@link Throwable} thrown + * by adding the passed {@code failure} as a suppressed with + * {@link ExceptionUtil#addSuppressedIfNotAssociated(Throwable, Throwable)}. + * @param callback The callback to fail + * @param failure The failure + * @throws RuntimeException If thrown, will have the {@code failure} added as a suppressed. + */ + static void failed(Callback callback, Throwable failure) + { + try + { + callback.failed(failure); + } + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(t, failure); + throw t; + } + } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index acc506d3e76a..23d5decb6cf4 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -499,15 +499,24 @@ public void handle() ExceptionUtil.addSuppressedIfNotAssociated(cause, x); if (LOG.isDebugEnabled()) LOG.debug("Could not perform error handling, aborting", cause); - if (_state.isResponseCommitted()) + + try { - // Perform the same behavior as when the callback is failed. - _state.errorHandlingComplete(cause); + if (_state.isResponseCommitted()) + { + // Perform the same behavior as when the callback is failed. + _state.errorHandlingComplete(cause); + } + else + { + getServletContextResponse().resetContent(); + sendErrorResponseAndComplete(); + } } - else + catch (Throwable t) { - getServletContextResponse().resetContent(); - sendErrorResponseAndComplete(); + ExceptionUtil.addSuppressedIfNotAssociated(t, cause); + throw t; } } finally From 50da68359b2b55885a353ec13ae26adbebc30ac0 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 27 Feb 2024 09:47:10 +0100 Subject: [PATCH 2/4] Fixed typos in Callback --- .../main/java/org/eclipse/jetty/util/Callback.java | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java index cb45210f141c..c10de5de0c55 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java @@ -327,7 +327,6 @@ public void succeeded() { try { - callback.succeeded(); completed.run(); } catch (Throwable t) @@ -363,7 +362,6 @@ public void failed(Throwable x) */ static Callback from(Callback callback, Throwable cause) { - callback.succeeded(); return new Callback() { @Override @@ -375,16 +373,8 @@ public void succeeded() @Override public void failed(Throwable x) { - try - { - cause.addSuppressed(x); - callback.failed(cause); - } - catch (Throwable t) - { - ExceptionUtil.addSuppressedIfNotAssociated(t, x); - throw t; - } + ExceptionUtil.addSuppressedIfNotAssociated(cause, x); + Callback.failed(callback, cause); } }; } From bf9e3129641cbc7d327ef120676974361d39df48 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 28 Feb 2024 22:53:32 +0100 Subject: [PATCH 3/4] updates from review --- .../server/internal/HttpChannelState.java | 31 ++++++++++++++++--- .../java/org/eclipse/jetty/util/Callback.java | 4 +-- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index 9e71dd430083..3aa45a79d3fa 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -1114,7 +1114,7 @@ private Runnable lockedFailWrite(Throwable x, boolean fatal) } if (writeCallback == null) return null; - return () -> Callback.failed(writeCallback, x); + return () -> HttpChannelState.failed(writeCallback, x); } public long getContentBytesWritten() @@ -1229,7 +1229,7 @@ else if (last && !(totalWritten == 0 && HttpMethod.HEAD.is(_request.getMethod()) if (writeFailure != null) { Throwable failure = writeFailure; - httpChannelState._writeInvoker.run(() -> Callback.failed(callback, failure)); + httpChannelState._writeInvoker.run(() -> HttpChannelState.failed(callback, failure)); return; } @@ -1297,7 +1297,7 @@ public void failed(Throwable x) httpChannel.lockedStreamSendCompleted(false); } if (callback != null) - httpChannel._writeInvoker.run(() -> Callback.failed(callback, x)); + httpChannel._writeInvoker.run(() -> HttpChannelState.failed(callback, x)); } @Override @@ -1691,7 +1691,7 @@ public void succeeded() } else { - Callback.failed(httpChannelState._handlerInvoker, failure); + HttpChannelState.failed(httpChannelState._handlerInvoker, failure); } } @@ -1714,7 +1714,7 @@ public void failed(Throwable x) httpChannelState._response._status = _errorResponse._status; } ExceptionUtil.addSuppressedIfNotAssociated(failure, x); - Callback.failed(httpChannelState._handlerInvoker, failure); + HttpChannelState.failed(httpChannelState._handlerInvoker, failure); } @Override @@ -1883,4 +1883,25 @@ public void onComplianceViolation(ComplianceViolation.Event event) } } } + + /** + * Invoke a callback failure, handling any {@link Throwable} thrown + * by adding the passed {@code failure} as a suppressed with + * {@link ExceptionUtil#addSuppressedIfNotAssociated(Throwable, Throwable)}. + * @param callback The callback to fail + * @param failure The failure + * @throws RuntimeException If thrown, will have the {@code failure} added as a suppressed. + */ + private static void failed(Callback callback, Throwable failure) + { + try + { + callback.failed(failure); + } + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(t, failure); + throw t; + } + } } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java index c10de5de0c55..5eb828bcc09a 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java @@ -328,12 +328,12 @@ public void succeeded() try { completed.run(); + callback.succeeded(); } catch (Throwable t) { Callback.failed(callback, t); } - callback.succeeded(); } @Override @@ -640,7 +640,7 @@ public Completable compose(Consumer consumer) * @param failure The failure * @throws RuntimeException If thrown, will have the {@code failure} added as a suppressed. */ - static void failed(Callback callback, Throwable failure) + private static void failed(Callback callback, Throwable failure) { try { From 01a1a0918e19f65ba4df5a99a3830c32358b4068 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 29 Feb 2024 17:42:44 +0100 Subject: [PATCH 4/4] Update from review --- .../java/org/eclipse/jetty/util/Callback.java | 108 +++++++++--------- 1 file changed, 54 insertions(+), 54 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java index 5eb828bcc09a..650672d99ea4 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java @@ -288,23 +288,7 @@ public void succeeded() @Override public void failed(Throwable x) { - try - { - callback.failed(x); - } - catch (Throwable t) - { - ExceptionUtil.addSuppressedIfNotAssociated(x, t); - } - try - { - completed.accept(x); - } - catch (Throwable t) - { - ExceptionUtil.addSuppressedIfNotAssociated(t, x); - throw t; - } + Callback.failed(callback::failed, completed, x); } }; } @@ -336,18 +320,15 @@ public void succeeded() } } + private void completed(Throwable ignored) + { + completed.run(); + } + @Override public void failed(Throwable x) { - try - { - completed.run(); - } - catch (Throwable t) - { - ExceptionUtil.addSuppressedIfNotAssociated(x, t); - } - Callback.failed(callback, x); + Callback.failed(this::completed, callback::failed, x); } }; } @@ -441,6 +422,11 @@ public void completed() { } + private void completed(Throwable ignored) + { + completed(); + } + @Override public void succeeded() { @@ -457,22 +443,7 @@ public void succeeded() @Override public void failed(Throwable x) { - try - { - try - { - callback.failed(x); - } - finally - { - completed(); - } - } - catch (Throwable t) - { - ExceptionUtil.addSuppressedIfNotAssociated(t, x); - throw t; - } + Callback.failed(callback::failed, this::completed, x); } @Override @@ -513,16 +484,7 @@ public void succeeded() @Override public void failed(Throwable x) { - try - { - cb1.failed(x); - } - catch (Throwable t) - { - ExceptionUtil.addSuppressedIfNotAssociated(x, t); - } - - Callback.failed(cb2, x); + Callback.failed(cb1::failed, cb2::failed, x); } @Override @@ -572,8 +534,7 @@ public void succeeded() @Override public void failed(Throwable x) { - Callback.failed(callback, x); - super.failed(x); + Callback.failed(callback::failed, super::failed, x); } }; } @@ -652,4 +613,43 @@ private static void failed(Callback callback, Throwable failure) throw t; } } + + /** + * Invoke two consumers of a failure, handling any {@link Throwable} thrown + * by adding the passed {@code failure} as a suppressed with + * {@link ExceptionUtil#addSuppressedIfNotAssociated(Throwable, Throwable)}. + * @param first The first consumer of a failure + * @param second The first consumer of a failure + * @param failure The failure + * @throws RuntimeException If thrown, will have the {@code failure} added as a suppressed. + */ + private static void failed(Consumer first, Consumer second, Throwable failure) + { + // This is an improved version of: + // try + // { + // first.accept(failure); + // } + // finally + // { + // second.accept(failure); + // } + try + { + first.accept(failure); + } + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(failure, t); + } + try + { + second.accept(failure); + } + catch (Throwable t) + { + ExceptionUtil.addSuppressedIfNotAssociated(t, failure); + throw t; + } + } }