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 721a00ccf6cd..321f51d90288 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 @@ -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) diff --git a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/ServerTimeoutsTest.java b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/ServerTimeoutsTest.java index 4c7778934ce5..fa1904e36cb8 100644 --- a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/ServerTimeoutsTest.java +++ b/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; } }); diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java index 719bd9d17d0f..358ba0611192 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java +++ b/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. diff --git a/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-client-transports/src/test/java/org/eclipse/jetty/ee10/test/client/transport/ServerTimeoutsTest.java b/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-client-transports/src/test/java/org/eclipse/jetty/ee10/test/client/transport/ServerTimeoutsTest.java index 49a2f377d0f4..a1afc70186cb 100644 --- a/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-client-transports/src/test/java/org/eclipse/jetty/ee10/test/client/transport/ServerTimeoutsTest.java +++ b/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(); });