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 class ByteArrayEndPoint extends AbstractEndPoint
         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 class HttpChannelState implements HttpChannel, Components
             if (idleTO >= 0 && _oldIdleTimeout != idleTO)
                 _stream.setIdleTimeout(idleTO);
 
-
             // This is deliberately not serialized to allow a handler to block.
             return _handlerInvoker;
         }
@@ -353,15 +352,52 @@ public class HttpChannelState implements HttpChannel, Components
         {
             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();
@@ -369,7 +405,9 @@ public class HttpChannelState implements HttpChannel, Components
             };
             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 class HttpChannelState implements HttpChannel, Components
 
             // 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 @@ public class HttpChannelState implements HttpChannel, Components
 
                 // 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 @@ public class HttpChannelState implements HttpChannel, Components
                     {
                         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 @@ public class HttpChannelState implements HttpChannel, Components
                 };
 
                 // Serialize all the error actions.
-                task = _serializedInvoker.offer(invokeOnContentAvailable, invokeWriteFailure, invokeListeners);
+                task = _serializedInvoker.offer(invokeOnContentAvailable, invokeWriteFailure, invokeOnFailureListeners);
             }
         }
 
@@ -915,6 +950,7 @@ public class HttpChannelState implements HttpChannel, Components
                 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.MatcherAssert.assertThat;
 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 class ServerTimeoutsTest extends AbstractTest
 
         // 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.EventListener;
 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 @@ public class ServletContextRequest extends ContextRequest implements ServletCont
         _decodedPathInContext = decodedPathInContext;
         _response =  newServletContextResponse(response);
         _sessionManager = sessionManager;
-        addIdleTimeoutListener(this::onIdleTimeout);
+        addIdleTimeoutListener(_servletChannel.getServletRequestState()::onIdleTimeout);
     }
 
     protected ServletApiRequest newServletApiRequest()
@@ -138,11 +137,6 @@ public class ServletContextRequest extends ContextRequest implements ServletCont
         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 class ServletRequestState
         {
             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;
         }
     }
 

jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletTest.java

@@ -0,0 +1,193 @@
+//
+// ========================================================================
+// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
+//
+// This program and the accompanying materials are made available under the
+// terms of the Eclipse Public License v. 2.0 which is available at
+// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
+// which is available at https://www.apache.org/licenses/LICENSE-2.0.
+//
+// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
+// ========================================================================
+//
+
+package org.eclipse.jetty.ee10.servlet;
+
+import java.io.IOException;
+import java.util.concurrent.TimeUnit;
+
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import org.eclipse.jetty.server.LocalConnector;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.util.IO;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+public class ServletTest
+{
+    private Server _server;
+    ServletContextHandler _context;
+    ServletHandler _handler;
+    private LocalConnector _connector;
+
+    @BeforeEach
+    public void beforeEach() throws Exception
+    {
+        _server = new Server();
+        _connector = new LocalConnector(_server);
+        _server.addConnector(_connector);
+
+        _context = new ServletContextHandler();
+        _context.setContextPath("/ctx");
+
+        _server.setHandler(_context);
+        _handler = _context.getServletHandler();
+    }
+
+    @AfterEach
+    public void afterEach() throws Exception
+    {
+        _server.stop();
+    }
+
+    @Test
+    public void testGET() throws Exception
+    {
+        _context.addServlet(new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
+            {
+                resp.getWriter().println("Hello!");
+            }
+        }, "/get");
+
+        _server.start();
+
+        String response = _connector.getResponse("""
+            GET /ctx/get HTTP/1.0
+            
+            """);
+        assertThat(response, containsString(" 200 OK"));
+        assertThat(response, containsString("Hello!"));
+    }
+
+    @Test
+    public void testSimpleIdleIgnored() throws Exception
+    {
+        _context.addServlet(new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
+            {
+                try
+                {
+                    Thread.sleep(1000);
+                }
+                catch (InterruptedException e)
+                {
+                    Thread.currentThread().interrupt();
+                }
+                resp.getWriter().println("Hello!");
+            }
+        }, "/get");
+
+        _connector.setIdleTimeout(250);
+        _server.start();
+
+        String response = _connector.getResponse("""
+            GET /ctx/get HTTP/1.0
+            
+            """, 5, TimeUnit.SECONDS);
+        assertThat(response, containsString(" 200 OK"));
+        assertThat(response, containsString("Hello!"));
+    }
+
+    @Test
+    public void testSimpleIdleRead() throws Exception
+    {
+        _context.addServlet(new HttpServlet()
+        {
+            @Override
+            protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException
+            {
+                String input = IO.toString(req.getInputStream());
+                resp.getWriter().println("Hello " + input);
+            }
+        }, "/post");
+
+        _connector.setIdleTimeout(250);
+        _server.start();
+
+        try (LocalConnector.LocalEndPoint endPoint = _connector.connect())
+        {
+            String request = """
+                POST /ctx/post HTTP/1.1
+                Host: local
+                Content-Length: 10
+                            
+                """;
+            endPoint.addInput(request);
+            endPoint.addInput("1234567890\n");
+            String response = endPoint.getResponse(false, 5, TimeUnit.SECONDS);
+            assertThat(response, containsString(" 200 OK"));
+            assertThat(response, containsString("Hello 1234567890"));
+
+            endPoint.addInputAndExecute(request);
+            endPoint.addInput("1234567890\n");
+            response = endPoint.getResponse(false, 5, TimeUnit.SECONDS);
+            assertThat(response, containsString(" 200 OK"));
+            assertThat(response, containsString("Hello 1234567890"));
+
+            endPoint.addInputAndExecute(request);
+            response = endPoint.getResponse(false, 5, TimeUnit.SECONDS);
+            assertThat(response, containsString(" 500 "));
+            assertThat(response, containsString("Connection: close"));
+        }
+    }
+
+    @Test
+    public void testIdle() throws Exception
+    {
+        _context.addServlet(new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
+            {
+                resp.getWriter().println("Hello!");
+            }
+        }, "/get");
+
+        _connector.setIdleTimeout(250);
+        _server.start();
+
+        try (LocalConnector.LocalEndPoint endPoint = _connector.connect())
+        {
+            String request = """
+                GET /ctx/get HTTP/1.1
+                Host: local
+                            
+                """;
+            endPoint.addInput(request);
+            String response = endPoint.getResponse();
+            assertThat(response, containsString(" 200 OK"));
+            assertThat(response, containsString("Hello!"));
+            endPoint.addInput(request);
+            response = endPoint.getResponse();
+            assertThat(response, containsString(" 200 OK"));
+            assertThat(response, containsString("Hello!"));
+
+            Thread.sleep(500);
+
+            assertFalse(endPoint.isOpen());
+        }
+    }
+}

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

@@ -55,11 +55,6 @@ import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
-// TODO: the server-side timeout mechanism should be completely reworked.
-//  Currently, HttpChannel.onFailure() is called, but timeouts are different
-//  since they may be ignored, so we don't want to remember errors if they are ignored.
-//  However, this behavior is historically so because of Servlets, and we
-//  may decide differently for Handlers.
 public class ServerTimeoutsTest extends AbstractTest
 {
     @ParameterizedTest

jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java

@@ -924,6 +924,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor
     public void onRequest(ContextHandler.CoreContextRequest coreRequest)
     {
         _coreRequest = coreRequest;
+        _coreRequest.addIdleTimeoutListener(_state::onIdleTimeout);
         _requests.incrementAndGet();
         _request.onRequest(coreRequest);
         _expects100Continue = coreRequest.getHeaders().contains(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString());

jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannelState.java

@@ -16,6 +16,7 @@ package org.eclipse.jetty.ee9.nested;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 
 import jakarta.servlet.AsyncListener;
 import jakarta.servlet.ServletContext;
@@ -164,6 +165,14 @@ public class HttpChannelState
         }
     }
 
+    public boolean onIdleTimeout(TimeoutException timeout)
+    {
+        try (AutoLock l = lock())
+        {
+            return _state == State.IDLE;
+        }
+    }
+
     public void addListener(AsyncListener listener)
     {
         try (AutoLock l = lock())

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ServletTest.java

@@ -0,0 +1,193 @@
+//
+// ========================================================================
+// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
+//
+// This program and the accompanying materials are made available under the
+// terms of the Eclipse Public License v. 2.0 which is available at
+// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
+// which is available at https://www.apache.org/licenses/LICENSE-2.0.
+//
+// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
+// ========================================================================
+//
+
+package org.eclipse.jetty.ee9.servlet;
+
+import java.io.IOException;
+import java.util.concurrent.TimeUnit;
+
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import org.eclipse.jetty.server.LocalConnector;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.util.IO;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+public class ServletTest
+{
+    private Server _server;
+    ServletContextHandler _context;
+    ServletHandler _handler;
+    private LocalConnector _connector;
+
+    @BeforeEach
+    public void beforeEach()
+    {
+        _server = new Server();
+        _connector = new LocalConnector(_server);
+        _server.addConnector(_connector);
+
+        _context = new ServletContextHandler();
+        _context.setContextPath("/ctx");
+
+        _server.setHandler(_context);
+        _handler = _context.getServletHandler();
+    }
+
+    @AfterEach
+    public void afterEach() throws Exception
+    {
+        _server.stop();
+    }
+
+    @Test
+    public void testGET() throws Exception
+    {
+        _context.addServlet(new ServletHolder(new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException
+            {
+                resp.getWriter().println("Hello!");
+            }
+        }), "/get");
+
+        _server.start();
+
+        String response = _connector.getResponse("""
+            GET /ctx/get HTTP/1.0
+            
+            """);
+        assertThat(response, containsString(" 200 OK"));
+        assertThat(response, containsString("Hello!"));
+    }
+
+    @Test
+    public void testSimpleIdleIgnored() throws Exception
+    {
+        _context.addServlet(new ServletHolder(new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException
+            {
+                try
+                {
+                    Thread.sleep(1000);
+                }
+                catch (InterruptedException e)
+                {
+                    Thread.currentThread().interrupt();
+                }
+                resp.getWriter().println("Hello!");
+            }
+        }), "/get");
+
+        _connector.setIdleTimeout(250);
+        _server.start();
+
+        String response = _connector.getResponse("""
+            GET /ctx/get HTTP/1.0
+            
+            """, 5, TimeUnit.SECONDS);
+        assertThat(response, containsString(" 200 OK"));
+        assertThat(response, containsString("Hello!"));
+    }
+
+    @Test
+    public void testSimpleIdleRead() throws Exception
+    {
+        _context.addServlet(new ServletHolder(new HttpServlet()
+        {
+            @Override
+            protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException
+            {
+                String input = IO.toString(req.getInputStream());
+                resp.getWriter().println("Hello " + input);
+            }
+        }), "/post");
+
+        _connector.setIdleTimeout(250);
+        _server.start();
+
+        try (LocalConnector.LocalEndPoint endPoint = _connector.connect())
+        {
+            String request = """
+                POST /ctx/post HTTP/1.1
+                Host: local
+                Content-Length: 10
+                            
+                """;
+            endPoint.addInput(request);
+            endPoint.addInput("1234567890\n");
+            String response = endPoint.getResponse(false, 5, TimeUnit.SECONDS);
+            assertThat(response, containsString(" 200 OK"));
+            assertThat(response, containsString("Hello 1234567890"));
+
+            endPoint.addInputAndExecute(request);
+            endPoint.addInput("1234567890\n");
+            response = endPoint.getResponse(false, 5, TimeUnit.SECONDS);
+            assertThat(response, containsString(" 200 OK"));
+            assertThat(response, containsString("Hello 1234567890"));
+
+            endPoint.addInputAndExecute(request);
+            response = endPoint.getResponse(false, 5, TimeUnit.SECONDS);
+            assertThat(response, containsString(" 500 "));
+            assertThat(response, containsString("Connection: close"));
+        }
+    }
+
+    @Test
+    public void testIdle() throws Exception
+    {
+        _context.addServlet(new ServletHolder(new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
+            {
+                resp.getWriter().println("Hello!");
+            }
+        }), "/get");
+
+        _connector.setIdleTimeout(250);
+        _server.start();
+
+        try (LocalConnector.LocalEndPoint endPoint = _connector.connect())
+        {
+            String request = """
+                GET /ctx/get HTTP/1.1
+                Host: local
+                            
+                """;
+            endPoint.addInput(request);
+            String response = endPoint.getResponse();
+            assertThat(response, containsString(" 200 OK"));
+            assertThat(response, containsString("Hello!"));
+            endPoint.addInput(request);
+            response = endPoint.getResponse();
+            assertThat(response, containsString(" 200 OK"));
+            assertThat(response, containsString("Hello!"));
+
+            Thread.sleep(500);
+
+            assertFalse(endPoint.isOpen());
+        }
+    }
+}