From 4debff75e2d0e0dd97a434746dbcccb3505c8d4a Mon Sep 17 00:00:00 2001 From: sid-evolphin <102239119+sid-evolphin@users.noreply.github.com> Date: Tue, 20 Jun 2023 11:33:27 +0530 Subject: [PATCH 1/5] Fix 9.4.51 NullPointerException #9476 Added a null check for the Callback parameter in o.e.j.iAbstractConnection.failedCallback(Callback, Throwable) so that the subsequent code-flow is not aborted; while the root cause may be ascertained in case debug logging is enabled. Further, in the case of a RejectedExecutionException when submitting the failCallback, the Runnable is run instead of directly invoking the Callback, so that exceptions in the Callback do not abort the synchronous code-flow. --- .../java/org/eclipse/jetty/io/AbstractConnection.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java index fa8de8dcbac1..aee056a72dbd 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java @@ -85,6 +85,12 @@ protected Executor getExecutor() protected void failedCallback(final Callback callback, final Throwable x) { + if (callback == null) { + if (LOG.isDebugEnabled()) { + LOG.warn("Callback null", x); + } + return; + } Runnable failCallback = () -> { try @@ -93,7 +99,7 @@ protected void failedCallback(final Callback callback, final Throwable x) } catch (Exception e) { - LOG.warn(e); + LOG.warn("Failed callback", e); } }; @@ -107,7 +113,7 @@ protected void failedCallback(final Callback callback, final Throwable x) catch (RejectedExecutionException e) { LOG.debug(e); - callback.failed(x); + failCallback.run(); } break; From 452a418ad8bb0a995e76397544f3974e4f41b080 Mon Sep 17 00:00:00 2001 From: sid-evolphin <102239119+sid-evolphin@users.noreply.github.com> Date: Tue, 20 Jun 2023 11:54:23 +0530 Subject: [PATCH 2/5] Fixed checkstyle errors --- .../main/java/org/eclipse/jetty/io/AbstractConnection.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java index aee056a72dbd..18c7bc3a42d7 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java @@ -85,8 +85,10 @@ protected Executor getExecutor() protected void failedCallback(final Callback callback, final Throwable x) { - if (callback == null) { - if (LOG.isDebugEnabled()) { + if (callback == null) + { + if (LOG.isDebugEnabled()) + { LOG.warn("Callback null", x); } return; From 9aa8eef59884cd8c75d6460a711566c10f2aef5b Mon Sep 17 00:00:00 2001 From: sid-evolphin <102239119+sid-evolphin@users.noreply.github.com> Date: Tue, 20 Jun 2023 15:11:33 +0530 Subject: [PATCH 3/5] Issue #9476 Fixed null Callback usages in HttpConnection.SendCallback 1. Fixed the usages of `Callback` returned by `o.e.j.s.HttpConnection.SendCallback.release()` to check for `null` before invoking the `failedCallback(Callback, Throwable)` or `succeeded()` methods. 2. Also, added a try-finally block around the Callback invocations in order to ensure that shutdownOutput will get performed irrespective of whether they end up throwing any exception. --- .../eclipse/jetty/server/HttpConnection.java | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index b61b201d6d61..64ec7ada695f 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -897,17 +897,27 @@ private void releaseChunk() @Override protected void onCompleteSuccess() { - release().succeeded(); - if (_shutdownOut) - getEndPoint().shutdownOutput(); + Callback callback = release(); + try { + if (callback != null) + callback.succeeded(); + } finally { + if (_shutdownOut) + getEndPoint().shutdownOutput(); + } } @Override public void onCompleteFailure(final Throwable x) { - failedCallback(release(), x); - if (_shutdownOut) - getEndPoint().shutdownOutput(); + Callback callback = release(); + try { + if (callback != null) + failedCallback(callback, x); + } finally { + if (_shutdownOut) + getEndPoint().shutdownOutput(); + } } @Override From a4db9353f6c46b50f302643351de9ef2246e1280 Mon Sep 17 00:00:00 2001 From: sid-evolphin <102239119+sid-evolphin@users.noreply.github.com> Date: Tue, 20 Jun 2023 15:14:07 +0530 Subject: [PATCH 4/5] checkstyle fixes --- .../org/eclipse/jetty/server/HttpConnection.java | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index 64ec7ada695f..1c7cc9b328dd 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -898,10 +898,13 @@ private void releaseChunk() protected void onCompleteSuccess() { Callback callback = release(); - try { + try + { if (callback != null) callback.succeeded(); - } finally { + } + finally + { if (_shutdownOut) getEndPoint().shutdownOutput(); } @@ -911,10 +914,13 @@ protected void onCompleteSuccess() public void onCompleteFailure(final Throwable x) { Callback callback = release(); - try { + try + { if (callback != null) failedCallback(callback, x); - } finally { + } + finally + { if (_shutdownOut) getEndPoint().shutdownOutput(); } From 3df3568b277b5efe75e374097eac63493e5ba9bd Mon Sep 17 00:00:00 2001 From: sid-evolphin <102239119+sid-evolphin@users.noreply.github.com> Date: Tue, 20 Jun 2023 15:30:50 +0530 Subject: [PATCH 5/5] Issue #9476 Partially reverted AbstractConnection based on review feedback Partially reverted `o.e.j.i.AbstractConnection.failedCallback(Callback, Throwable)` to not have a null check for the `Callback` parameter, based on review feedback from @gregw. This avoids changing the contract of the method parameter (from `@NonNull` to `@Nullable`). --- .../java/org/eclipse/jetty/io/AbstractConnection.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java index 18c7bc3a42d7..fb3b720da76b 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java @@ -85,14 +85,6 @@ protected Executor getExecutor() protected void failedCallback(final Callback callback, final Throwable x) { - if (callback == null) - { - if (LOG.isDebugEnabled()) - { - LOG.warn("Callback null", x); - } - return; - } Runnable failCallback = () -> { try