From d6a022686602bb9f06a1074b9bd879f89afbb325 Mon Sep 17 00:00:00 2001
From: Greg Wilkins <gregw@webtide.com>
Date: Tue, 29 Aug 2023 20:44:40 +1000
Subject: [PATCH] Fix #9169 refine idle timeout and failure (#10418)

Only fail request callback if a failure has not been otherwise notified.
Slight optimisation for failing idle timeouts by avoiding double lock.
Always create a failure if failing the callback.
---
 .../server/internal/HttpChannelState.java     | 43 +++++++++----------
 .../eclipse/jetty/server/HttpChannelTest.java |  1 +
 .../org/eclipse/jetty/server/ServerTest.java  |  1 +
 3 files changed, 23 insertions(+), 22 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 eedcf0dfaafa..00940529d473 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
@@ -346,7 +346,6 @@ public Invocable.InvocationType getInvocationType()
     @Override
     public Runnable onIdleTimeout(TimeoutException t)
     {
-        Predicate<TimeoutException> onIdleTimeout;
         try (AutoLock ignored = _lock.lock())
         {
             if (LOG.isDebugEnabled())
@@ -380,29 +379,29 @@ public Runnable onIdleTimeout(TimeoutException t)
                 if (invokeOnContentAvailable != null || invokeWriteFailure != null)
                     return _serializedInvoker.offer(invokeOnContentAvailable, invokeWriteFailure);
 
-                // Otherwise We ask any idle timeout listeners if we should call onFailure or not
-                onIdleTimeout = _onIdleTimeout;
-            }
-            else
-            {
-                onIdleTimeout = null;
-            }
-        }
+                // otherwise, if there is an idle timeout listener, we ask if if we should call onFailure or not
+                Predicate<TimeoutException> onIdleTimeout = _onIdleTimeout;
+                if (onIdleTimeout != null)
+                {
+                    return _serializedInvoker.offer(() ->
+                    {
+                        if (onIdleTimeout.test(t))
+                        {
+                            // If the idle timeout listener(s) return true, then we call onFailure and run any task it returns.
+                            Runnable task = onFailure(t);
+                            if (task != null)
+                                task.run();
+                        }
+                    });
+                }
 
-        // Ask any listener what to do
-        if (onIdleTimeout != null)
-        {
-            Runnable onIdle = () ->
-            {
-                if (onIdleTimeout.test(t))
+                // otherwise, if there is no failure listener, then we can fail the callback directly without a double lock
+                if (_onFailure == null && _request != null)
                 {
-                    // If the idle timeout listener(s) return true, then we call onFailure and any task it returns.
-                    Runnable task = onFailure(t);
-                    if (task != null)
-                        task.run();
+                    _failure = Content.Chunk.from(t, true);
+                    return () -> _request._callback.failed(t);
                 }
-            };
-            return _serializedInvoker.offer(onIdle);
+            }
         }
 
         // otherwise treat as a failure
@@ -476,7 +475,7 @@ else if (ExceptionUtil.areNotAssociated(_failure.getFailure(), x) && _failure.ge
                     }
 
                     // If the application has not been otherwise informed of the failure
-                    if (invokeOnContentAvailable == null && invokeWriteFailure == null)
+                    if (invokeOnContentAvailable == null && invokeWriteFailure == null && onFailure == null)
                     {
                         if (LOG.isDebugEnabled())
                             LOG.debug("failing callback in {}", this, x);
diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpChannelTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpChannelTest.java
index d9a488471696..72152ff84e6a 100644
--- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpChannelTest.java
+++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpChannelTest.java
@@ -1177,6 +1177,7 @@ public boolean handle(Request request, Response response, Callback callback)
                 request.addFailureListener(t -> error.set(null));
                 request.addFailureListener(t -> error.compareAndSet(null, t));
                 request.addFailureListener(t -> error.compareAndSet(null, new Throwable("WRONG")));
+                request.addFailureListener(callback::failed);
                 return true;
             }
         };
diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java
index b2cf089d6d23..2c1a99ff5a03 100644
--- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java
+++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java
@@ -385,6 +385,7 @@ public boolean handle(Request request, Response response, Callback callback)
                 request.addFailureListener(t ->
                 {
                     assertThat(ContextHandler.getCurrentContext(), sameInstance(_context.getContext()));
+                    callback.failed(t);
                     latch.countDown();
                 });
                 return true;