Transient timeouts. Fixes #10234 and #10277

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

@@ -361,15 +361,12 @@ public Runnable onIdleTimeout(TimeoutException t)
                 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
                 if (invokeOnContentAvailable != null)
-                {
                     _failure = Content.Chunk.from(t, false);
-                    System.err.println("failure chunk " + _failure);
-                }
+
+                // If a write call is in progress, take the writeCallback to fail below
+                Runnable invokeWriteFailure = _response.lockedFailWrite(t);
 
                 // If there was an IO operation, just deliver the idle timeout via them
                 if (invokeOnContentAvailable != null || invokeWriteFailure != null)

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

@@ -42,6 +42,7 @@
 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.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class ServerTimeoutsTest extends AbstractTest
@@ -75,7 +76,10 @@ public boolean handle(Request request, Response response, Callback callback)
             {
 
                 if (listener)
+                {
                     request.addIdleTimeoutListener(t -> listenerCalled.compareAndSet(false, true));
+                    request.addFailureListener(callback::failed);
+                }
 
                 // Do not complete the callback, so it idle times out.
                 return true;
@@ -130,15 +134,13 @@ 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));
+        assertTrue(Content.Chunk.isFailure(chunk, false));
         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);
@@ -198,6 +200,7 @@ public void testIdleTimeoutErrorListenerReturnsFalseThenTrue(Transport transport
             public boolean handle(Request request, Response response, Callback callback)
             {
                 request.addIdleTimeoutListener(t -> error.getAndSet(t) != null);
+                request.addFailureListener(callback::failed);
                 return true;
             }
         });

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

@@ -200,7 +200,11 @@ public Content.Chunk nextChunk()
         if (LOG.isDebugEnabled())
             LOG.debug("nextChunk = {} {}", chunk, this);
         if (chunk != null)
+        {
             _servletChannel.getServletRequestState().onReadIdle();
+            if (Content.Chunk.isFailure(chunk, false))
+                _chunk = Content.Chunk.next(chunk);
+        }
         return chunk;
     }
 
@@ -267,7 +271,7 @@ private Content.Chunk produceChunk()
         {
             if (_chunk != null)
             {
-                if (_chunk.isLast() || _chunk.hasRemaining())
+                if (_chunk.isLast() || _chunk.hasRemaining() || Content.Chunk.isFailure(_chunk, false))
                 {
                     if (LOG.isDebugEnabled())
                         LOG.debug("chunk not yet depleted, returning it {}", this);
@@ -292,10 +296,9 @@ private Content.Chunk produceChunk()
                         LOG.debug("channel has no new chunk {}", this);
                     return null;
                 }
-                else
-                {
-                    _servletChannel.getServletRequestState().onContentAdded();
-                }
+                _servletChannel.getServletRequestState().onContentAdded();
+                if (Content.Chunk.isFailure(_chunk, false))
+                    return _chunk;
             }
 
             // Release the chunk immediately, if it is empty.

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

@@ -463,7 +463,8 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
                     @Override
                     public void onDataAvailable() throws IOException
                     {
-                        assertEquals(0, input.read());
+                        while (input.isReady())
+                            assertEquals(0, input.read());
                         assertFalse(input.isReady());
                     }
 
@@ -477,17 +478,10 @@ public void onError(Throwable failure)
                     {
                         if (failure instanceof TimeoutException)
                         {
-                            response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR_500);
+                            response.setStatus(HttpStatus.GATEWAY_TIMEOUT_504);
                             handlerLatch.countDown();
                         }
 
-                        // TODO the problem here is that timeout failures are currently persistent and affect reads
-                        //      and writes.  So after the 500 is set above, the complete below tries to commit the response,
-                        //      but the write/send for that fails with the same timeout exception.   Thus the 500 is never
-                        //      sent and the connection is just closed.
-                        //      This was not apparent until the change in HttpOutput#onWriteComplete to not always abort on
-                        //      failure (as this prevents async handling completing on its own terms).
-                        //      We can "fix" this here by doing a response.sendError(-1);
                         asyncContext.complete();
                     }
                 });
@@ -501,7 +495,7 @@ public void onError(Throwable failure)
             .body(content)
             .send(result ->
             {
-                if (result.getResponse().getStatus() == HttpStatus.INTERNAL_SERVER_ERROR_500)
+                if (result.getResponse().getStatus() == HttpStatus.GATEWAY_TIMEOUT_504)
                     resultLatch.countDown();
             });