Fix #10229 servlet Idle Timeout (#10233)

* Fix #10229  Idle Timeout

Added test to reproduce

Fixed NPE if no failure listener


Possible

Added test that idle works between requests

EE9 idle timeout

idle if read operation

Handle idleTimeout for IO operations differently

improve comments

fixed test to not expect timeout listener to be called if there is demand

Idle timeouts for IO operations are not last.

Disable transient idle timeouts since AsyncContentProducer cannot handle them.

revert test to persistent idle failures

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteArrayEndPoint.java

@@ -219,6 +219,11 @@ public void addInput(String s, Charset charset)
         addInput(BufferUtil.toBuffer(s, charset));
     }
 
+    public void addInputAndExecute(String s)
+    {
+        addInputAndExecute(BufferUtil.toBuffer(s, StandardCharsets.UTF_8));
+    }
+
     public void addInputAndExecute(ByteBuffer in)
     {
         boolean fillable = false;

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

@@ -287,7 +287,6 @@ public Runnable onRequest(MetaData.Request request)
             if (idleTO >= 0 && _oldIdleTimeout != idleTO)
                 _stream.setIdleTimeout(idleTO);
 
-
             // This is deliberately not serialized to allow a handler to block.
             return _handlerInvoker;
         }
@@ -353,23 +352,62 @@ public Runnable onIdleTimeout(TimeoutException t)
         {
             if (LOG.isDebugEnabled())
                 LOG.debug("onIdleTimeout {}", this, t);
-            onIdleTimeout = _onIdleTimeout;
+
+            // if not already a failure,
+            if (_failure == null)
+            {
+                // if we are currently demanding, take the onContentAvailable runnable to invoke below.
+                Runnable invokeOnContentAvailable = _onContentAvailable;
+                _onContentAvailable = null;
+
+                // If a write call is in progress, take the writeCallback to fail below
+                Runnable invokeWriteFailure = _response.lockedFailWrite(t);
+
+                // If demand was in process, then arrange for the next read to return the idle timeout, if no other error
+                // TODO to make IO timeouts transient, remove the invokeWriteFailure test below.
+                //      Probably writes cannot be made transient as it will be unclear how much of the buffer has actually
+                //      been written.  So write timeouts might always be persistent... but then we should call the listener
+                //      before calling lockedFailedWrite above.
+                if (invokeOnContentAvailable != null || invokeWriteFailure != null)
+                {
+                    // TODO The chunk here should be last==false, so that IO timeout is a transient failure.
+                    //      However AsyncContentProducer has been written on the assumption of no transient
+                    //      failures, so it needs to be updated before we can make timeouts transients.
+                    //      See ServerTimeoutTest.testAsyncReadHttpIdleTimeoutOverridesIdleTimeout
+                    _failure = Content.Chunk.from(t, true);
+                }
+
+                // If there was an IO operation, just deliver the idle timeout via them
+                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;
+            }
         }
 
+        // Ask any listener what to do
         if (onIdleTimeout != null)
         {
             Runnable onIdle = () ->
             {
                 if (onIdleTimeout.test(t))
                 {
+                    // 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();
                 }
             };
             return _serializedInvoker.offer(onIdle);
         }
-        return onFailure(t); // TODO can we avoid double lock?
+
+        // otherwise treat as a failure
+        return onFailure(t);
     }
 
     @Override
@@ -398,13 +436,9 @@ public Runnable onFailure(Throwable x)
 
             // Set the error to arrange for any subsequent reads, demands or writes to fail.
             if (_failure == null)
-            {
-                _failure = Content.Chunk.from(x);
-            }
+                _failure = Content.Chunk.from(x, true);
             else if (ExceptionUtil.areNotAssociated(_failure.getFailure(), x) && _failure.getFailure().getClass() != x.getClass())
-            {
                 _failure.getFailure().addSuppressed(x);
-            }
 
             // If not handled, then we just fail the request callback
             if (!_handled && _handling == null)
@@ -422,7 +456,7 @@ else if (ExceptionUtil.areNotAssociated(_failure.getFailure(), x) && _failure.ge
 
                 // Create runnable to invoke any onError listeners
                 ChannelRequest request = _request;
-                Runnable invokeListeners = () ->
+                Runnable invokeOnFailureListeners = () ->
                 {
                     Consumer<Throwable> onFailure;
                     try (AutoLock ignore = _lock.lock())
@@ -434,7 +468,8 @@ else if (ExceptionUtil.areNotAssociated(_failure.getFailure(), x) && _failure.ge
                     {
                         if (LOG.isDebugEnabled())
                             LOG.debug("invokeListeners {} {}", HttpChannelState.this, onFailure, x);
-                        onFailure.accept(x);
+                        if (onFailure != null)
+                            onFailure.accept(x);
                     }
                     catch (Throwable throwable)
                     {
@@ -452,7 +487,7 @@ else if (ExceptionUtil.areNotAssociated(_failure.getFailure(), x) && _failure.ge
                 };
 
                 // Serialize all the error actions.
-                task = _serializedInvoker.offer(invokeOnContentAvailable, invokeWriteFailure, invokeListeners);
+                task = _serializedInvoker.offer(invokeOnContentAvailable, invokeWriteFailure, invokeOnFailureListeners);
             }
         }
 
@@ -915,6 +950,7 @@ public Content.Chunk read()
                 HttpChannelState httpChannel = lockedGetHttpChannelState();
 
                 Content.Chunk error = httpChannel._failure;
+                httpChannel._failure = Content.Chunk.next(error);
                 if (error != null)
                     return error;
 

jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/ServerTimeoutsTest.java

@@ -40,6 +40,7 @@
 import static org.hamcrest.Matchers.containsStringIgnoringCase;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -130,18 +131,25 @@ public boolean handle(Request request, Response response, Callback callback)
 
         // Reads should yield the idle timeout.
         Content.Chunk chunk = requestRef.get().read();
+        // TODO change last to false in the next line if timeouts are transients
         assertTrue(Content.Chunk.isFailure(chunk, true));
         Throwable cause = chunk.getFailure();
         assertThat(cause, instanceOf(TimeoutException.class));
 
+        /* TODO if transient timeout failures are supported then add this check
+        // Can read again
+        assertNull(requestRef.get().read());
+        */
+
         // Complete the callback as the error listener promised.
         callbackRef.get().failed(cause);
 
         ContentResponse response = futureResponse.get(IDLE_TIMEOUT / 2, TimeUnit.MILLISECONDS);
         assertThat(response.getStatus(), is(HttpStatus.INTERNAL_SERVER_ERROR_500));
         assertThat(response.getContentAsString(), containsStringIgnoringCase("HTTP ERROR 500 java.util.concurrent.TimeoutException: Idle timeout"));
-        if (listener)
-            assertTrue(listenerCalled.get());
+
+        // listener is never called as timeout always delivered via demand
+        assertFalse(listenerCalled.get());
     }
 
     @ParameterizedTest

jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java

@@ -20,7 +20,6 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
-import java.util.concurrent.TimeoutException;
 
 import jakarta.servlet.AsyncListener;
 import jakarta.servlet.ServletRequest;
@@ -114,7 +113,7 @@ protected ServletContextRequest(
         _decodedPathInContext = decodedPathInContext;
         _response =  newServletContextResponse(response);
         _sessionManager = sessionManager;
-        addIdleTimeoutListener(this::onIdleTimeout);
+        addIdleTimeoutListener(_servletChannel.getServletRequestState()::onIdleTimeout);
     }
 
     protected ServletApiRequest newServletApiRequest()
@@ -138,11 +137,6 @@ protected ServletContextResponse newServletContextResponse(Response response)
         return new ServletContextResponse(_servletChannel, this, response);
     }
 
-    private boolean onIdleTimeout(TimeoutException timeout)
-    {
-        return _servletChannel.getServletRequestState().onIdleTimeout(timeout);
-    }
-
     @Override
     public ServletContextHandler getServletContextHandler()
     {

jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java

@@ -735,9 +735,7 @@ public boolean onIdleTimeout(TimeoutException timeout)
         {
             if (LOG.isDebugEnabled())
                 LOG.debug("onIdleTimeout {}", getStatusStringLocked(), timeout);
-            // TODO this is almost always returning false?!? what about read/write timeouts???
-            //      return _state == State.IDLE;
-            return true;
+            return _state == State.IDLE;
         }
     }