From 87c24e7258224c85f7ab80d8e9c37231aecd7d22 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 31 Jul 2023 15:13:50 +0200 Subject: [PATCH] =?UTF-8?q?Fixes=20#8405=20-=20onAllDataRead()=20is=20call?= =?UTF-8?q?ed=20twice=20under=20h2=20if=20the=20stream=20=E2=80=A6=20(#101?= =?UTF-8?q?74)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fixes #8405 - onAllDataRead() is called twice under h2 if the stream times out Per Servlet semantic, HTTP/2 stream timeout should be ignored. The code was trying to fail the read via `_contentDemander.onTimeout()`, but then it was still calling `onContentProducible()`, which was returning `true` because the state of the read was IDLE (all the request content was read) and the request was suspended. Now the code checks if the read was really failed; if it is not, then `onContentProducible()` is not called and so the idle timeout is ignored. Signed-off-by: Simone Bordet --- .../http2/server/HttpChannelOverHTTP2.java | 47 ++++---- .../jetty/http/client/AsyncIOServletTest.java | 100 +++++++++++++++--- 2 files changed, 106 insertions(+), 41 deletions(-) diff --git a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java index 66ad5345673..a78108c3398 100644 --- a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java +++ b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java @@ -127,7 +127,7 @@ public class HttpChannelOverHTTP2 extends HttpChannel implements Closeable, Writ boolean connect = request instanceof MetaData.ConnectRequest; _delayedUntilContent = getHttpConfiguration().isDelayDispatchUntilContent() && - !endStream && !_expect100Continue && !connect; + !endStream && !_expect100Continue && !connect; // Delay the demand of DATA frames for CONNECT with :protocol // or for normal requests expecting 100 continue. @@ -146,10 +146,10 @@ public class HttpChannelOverHTTP2 extends HttpChannel implements Closeable, Writ { Stream stream = getStream(); LOG.debug("HTTP2 Request #{}/{}, delayed={}:{}{} {} {}{}{}", - stream.getId(), Integer.toHexString(stream.getSession().hashCode()), - _delayedUntilContent, System.lineSeparator(), - request.getMethod(), request.getURI(), request.getHttpVersion(), - System.lineSeparator(), fields); + stream.getId(), Integer.toHexString(stream.getSession().hashCode()), + _delayedUntilContent, System.lineSeparator(), + request.getMethod(), request.getURI(), request.getHttpVersion(), + System.lineSeparator(), fields); } return _delayedUntilContent ? null : this; @@ -179,9 +179,9 @@ public class HttpChannelOverHTTP2 extends HttpChannel implements Closeable, Writ { Stream stream = getStream(); LOG.debug("HTTP2 PUSH Request #{}/{}:{}{} {} {}{}{}", - stream.getId(), Integer.toHexString(stream.getSession().hashCode()), System.lineSeparator(), - request.getMethod(), request.getURI(), request.getHttpVersion(), - System.lineSeparator(), request.getFields()); + stream.getId(), Integer.toHexString(stream.getSession().hashCode()), System.lineSeparator(), + request.getMethod(), request.getURI(), request.getHttpVersion(), + System.lineSeparator(), request.getFields()); } return this; @@ -222,8 +222,8 @@ public class HttpChannelOverHTTP2 extends HttpChannel implements Closeable, Writ { Stream stream = getStream(); LOG.debug("HTTP2 Commit Response #{}/{}:{}{} {} {}{}{}", - stream.getId(), Integer.toHexString(stream.getSession().hashCode()), System.lineSeparator(), info.getHttpVersion(), info.getStatus(), info.getReason(), - System.lineSeparator(), info.getFields()); + stream.getId(), Integer.toHexString(stream.getSession().hashCode()), System.lineSeparator(), info.getHttpVersion(), info.getStatus(), info.getReason(), + System.lineSeparator(), info.getFields()); } } @@ -276,13 +276,13 @@ public class HttpChannelOverHTTP2 extends HttpChannel implements Closeable, Writ { Stream stream = getStream(); LOG.debug("HTTP2 Request #{}/{}: {} bytes of {} content, woken: {}, needed: {}, handle: {}", - stream.getId(), - Integer.toHexString(stream.getSession().hashCode()), - length, - endStream ? "last" : "some", - woken, - needed, - handle); + stream.getId(), + Integer.toHexString(stream.getSession().hashCode()), + length, + endStream ? "last" : "some", + woken, + needed, + handle); } boolean wasDelayed = _delayedUntilContent; @@ -622,8 +622,8 @@ public class HttpChannelOverHTTP2 extends HttpChannel implements Closeable, Writ { Stream stream = getStream(); LOG.debug("HTTP2 Request #{}/{}, trailers:{}{}", - stream.getId(), Integer.toHexString(stream.getSession().hashCode()), - System.lineSeparator(), trailers); + stream.getId(), Integer.toHexString(stream.getSession().hashCode()), + System.lineSeparator(), trailers); } // This will generate EOF -> need to call onContentProducible. @@ -645,7 +645,7 @@ public class HttpChannelOverHTTP2 extends HttpChannel implements Closeable, Writ @Override public boolean onTimeout(Throwable failure, Consumer consumer) { - final boolean delayed = _delayedUntilContent; + boolean delayed = _delayedUntilContent; _delayedUntilContent = false; boolean reset = isIdle(); @@ -655,10 +655,9 @@ public class HttpChannelOverHTTP2 extends HttpChannel implements Closeable, Writ getHttpTransport().onStreamTimeout(failure); failure.addSuppressed(new Throwable("HttpInput idle timeout")); - _contentDemander.onTimeout(failure); - boolean needed = getRequest().getHttpInput().onContentProducible(); - - if (needed || delayed) + boolean readFailed = _contentDemander.onTimeout(failure); + boolean handle = readFailed && getRequest().getHttpInput().onContentProducible(); + if (handle || delayed) { consumer.accept(this::handleWithContext); reset = false; diff --git a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/AsyncIOServletTest.java b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/AsyncIOServletTest.java index 2ad8d1fb657..59c3ef0b93f 100644 --- a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/AsyncIOServletTest.java +++ b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/AsyncIOServletTest.java @@ -29,6 +29,7 @@ import java.util.concurrent.Executor; import java.util.concurrent.Executors; import java.util.concurrent.LinkedBlockingDeque; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; @@ -96,7 +97,9 @@ import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; 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.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -1321,12 +1324,12 @@ public class AsyncIOServletTest extends AbstractTest errorRef = new AtomicReference<>(); + scenario.start(new HttpServlet() + { + @Override + protected void service(HttpServletRequest request, HttpServletResponse resp) throws IOException + { + AsyncContext asyncContext = request.startAsync(); + asyncContext.setTimeout(0); + + ServletInputStream input = request.getInputStream(); + input.setReadListener(new ReadListener() + { + @Override + public void onDataAvailable() throws IOException + { + while (input.isReady()) + { + int read = input.read(); + if (read < 0) + break; + } + } + + @Override + public void onAllDataRead() + { + allDataReadCount.incrementAndGet(); + } + + @Override + public void onError(Throwable x) + { + // There should be no errors because request body has + // been successfully read and idle timeouts are ignored. + errorRef.set(x); + } + }); + + // Never reply to the request, let it idle timeout. + // The Servlet semantic is that the idle timeout will + // be ignored so the client will timeout the request. + } + }); + long idleTimeout = 1000; + scenario.setConnectionIdleTimeout(2 * idleTimeout); + scenario.setRequestIdleTimeout(idleTimeout); + + assertThrows(TimeoutException.class, () -> scenario.client.newRequest(scenario.newURI()) + .path(scenario.servletPath) + .timeout(2 * idleTimeout, TimeUnit.MILLISECONDS) + .send() + ); + + assertNull(errorRef.get()); + assertEquals(1, allDataReadCount.get()); + } + private static class Listener implements ReadListener, WriteListener { private final Executor executor = Executors.newFixedThreadPool(32);