From f0cbbce68cff22d228cd00defea6558c83ff4bec Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 26 Aug 2024 14:18:03 +0200 Subject: [PATCH 1/6] fix npe when onCompleteFailure happens after reset Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/client/transport/HttpSender.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 55476b7f48ce..ba9ab6959aae 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 @@ -388,8 +388,8 @@ private void externalAbort(Throwable failure, Promise promise) private void internalAbort(HttpExchange exchange, Throwable failure) { - anyToFailure(failure); - abortRequest(exchange); + if (anyToFailure(failure)) + abortRequest(exchange); } private boolean updateRequestState(RequestState from, RequestState to) From 457bd7a6f1644bed459ce660f94299c7587d98f0 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 26 Aug 2024 17:38:50 +0200 Subject: [PATCH 2/6] fix npe when onCompleteFailure happens after reset Signed-off-by: Ludovic Orban --- .../jetty/client/transport/HttpSender.java | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) 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 ba9ab6959aae..d05db087bddf 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 @@ -481,17 +481,21 @@ private class ContentSender extends IteratingCallback @Override public boolean reset() { - exchange = null; - proceedAction = null; - expect100 = false; - chunk = null; - contentBuffer = null; - committed = false; - success = false; - complete = false; - abort = null; - demanded = false; - return super.reset(); + if (super.reset()) + { + exchange = null; + proceedAction = null; + expect100 = false; + chunk = null; + contentBuffer = null; + committed = false; + success = false; + complete = false; + abort = null; + demanded = false; + return true; + } + return false; } @Override From b0ccac3db5a0f6813cc192c088bd477e4a17d627 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 27 Aug 2024 10:56:39 +0200 Subject: [PATCH 3/6] throwaway ContentSender if reset fails Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/client/transport/HttpSender.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 d05db087bddf..4d6a00b8f6e0 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 @@ -49,7 +49,7 @@ public abstract class HttpSender { private static final Logger LOG = LoggerFactory.getLogger(HttpSender.class); - private final ContentSender contentSender = new ContentSender(); + private ContentSender contentSender = new ContentSender(); private final AtomicReference requestState = new AtomicReference<>(RequestState.QUEUED); private final AtomicReference failure = new AtomicReference<>(); private final HttpChannel channel; @@ -310,7 +310,8 @@ private void terminateRequest(HttpExchange exchange, Throwable failure, Result r protected void reset() { - contentSender.reset(); + if (!contentSender.reset()) + contentSender = new ContentSender(); } protected void dispose() From 60ac0ce34c61c1ac1e3ab932c02f37acf8981720 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 27 Aug 2024 17:47:36 +0200 Subject: [PATCH 4/6] revert reset changes Signed-off-by: Ludovic Orban --- .../jetty/client/transport/HttpSender.java | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) 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 4d6a00b8f6e0..ba9ab6959aae 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 @@ -49,7 +49,7 @@ public abstract class HttpSender { private static final Logger LOG = LoggerFactory.getLogger(HttpSender.class); - private ContentSender contentSender = new ContentSender(); + private final ContentSender contentSender = new ContentSender(); private final AtomicReference requestState = new AtomicReference<>(RequestState.QUEUED); private final AtomicReference failure = new AtomicReference<>(); private final HttpChannel channel; @@ -310,8 +310,7 @@ private void terminateRequest(HttpExchange exchange, Throwable failure, Result r protected void reset() { - if (!contentSender.reset()) - contentSender = new ContentSender(); + contentSender.reset(); } protected void dispose() @@ -482,21 +481,17 @@ private class ContentSender extends IteratingCallback @Override public boolean reset() { - if (super.reset()) - { - exchange = null; - proceedAction = null; - expect100 = false; - chunk = null; - contentBuffer = null; - committed = false; - success = false; - complete = false; - abort = null; - demanded = false; - return true; - } - return false; + exchange = null; + proceedAction = null; + expect100 = false; + chunk = null; + contentBuffer = null; + committed = false; + success = false; + complete = false; + abort = null; + demanded = false; + return super.reset(); } @Override From 14bded8a6bccbf80e8d271ecc2e8032e136d52b2 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Wed, 28 Aug 2024 09:51:10 +0200 Subject: [PATCH 5/6] more digging Signed-off-by: Ludovic Orban --- .../org/eclipse/jetty/client/transport/HttpSender.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) 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 ba9ab6959aae..8f27bf92f904 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 @@ -376,7 +376,7 @@ private void externalAbort(Throwable failure, Promise promise) if (abort) { contentSender.abort = promise; - contentSender.abort(this.failure.get()); + contentSender.abort(this.failure.get()); // this is going to call internalAbort() } else { @@ -388,8 +388,14 @@ private void externalAbort(Throwable failure, Promise promise) private void internalAbort(HttpExchange exchange, Throwable failure) { - if (anyToFailure(failure)) + // internalAbort() may be called from externalAbort() (which already called anyToFailure) + // so we cannot rely on its return code to figure out if abortRequest() should be called or not. + anyToFailure(failure); + // internalAbort() may be called after ContentSender.reset() so exchange might be null. + if (exchange != null) abortRequest(exchange); + else + dispose(); } private boolean updateRequestState(RequestState from, RequestState to) From 327d001766884ab7a60df0f5c2a755c24a1a4ce9 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Wed, 28 Aug 2024 17:32:03 +0200 Subject: [PATCH 6/6] don't use the exchange field from the error path Signed-off-by: Ludovic Orban --- .../jetty/client/transport/HttpSender.java | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) 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 8f27bf92f904..69bade58dd68 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 @@ -376,7 +376,7 @@ private void externalAbort(Throwable failure, Promise promise) if (abort) { contentSender.abort = promise; - contentSender.abort(this.failure.get()); // this is going to call internalAbort() + contentSender.abort(this.failure.get()); } else { @@ -386,16 +386,13 @@ private void externalAbort(Throwable failure, Promise promise) } } - private void internalAbort(HttpExchange exchange, Throwable failure) + private void internalAbort(Throwable failure) { - // internalAbort() may be called from externalAbort() (which already called anyToFailure) - // so we cannot rely on its return code to figure out if abortRequest() should be called or not. + HttpExchange exchange = getHttpExchange(); + if (exchange == null) + return; anyToFailure(failure); - // internalAbort() may be called after ContentSender.reset() so exchange might be null. - if (exchange != null) - abortRequest(exchange); - else - dispose(); + abortRequest(exchange); } private boolean updateRequestState(RequestState from, RequestState to) @@ -632,7 +629,7 @@ protected void onCompleteFailure(Throwable x) } failRequest(x); - internalAbort(exchange, x); + internalAbort(x); Promise promise = abort; if (promise != null)