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.
jetty · Aug 29, 2023 · d6a0226 · d6a0226
1 parent 5808a62
commit d6a0226
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 22 deletions.
diff --git a/...y-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/...y-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
@@ -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
@@ -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;