This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push: new 382ae1f1dc Fix BZ 69068 - trigger read timeouts for async reads on HTTP/2 382ae1f1dc is described below commit 382ae1f1dc25f30d1991ebd1adf2cfc9b71ba570 Author: Mark Thomas <ma...@apache.org> AuthorDate: Sun Jun 2 15:22:54 2024 +0100 Fix BZ 69068 - trigger read timeouts for async reads on HTTP/2 Test case provided by hypnoce --- .../apache/coyote/http2/LocalStrings.properties | 1 + java/org/apache/coyote/http2/Stream.java | 23 ++++ java/org/apache/coyote/http2/StreamProcessor.java | 20 +++ test/org/apache/coyote/http2/Http2TestBase.java | 2 +- .../apache/coyote/http2/TestAsyncReadListener.java | 145 +++++++++++++++++++++ webapps/docs/changelog.xml | 4 + 6 files changed, 194 insertions(+), 1 deletion(-) diff --git a/java/org/apache/coyote/http2/LocalStrings.properties b/java/org/apache/coyote/http2/LocalStrings.properties index 137e3346db..891ba7ad81 100644 --- a/java/org/apache/coyote/http2/LocalStrings.properties +++ b/java/org/apache/coyote/http2/LocalStrings.properties @@ -121,6 +121,7 @@ streamProcessor.error.connection=Connection [{0}], Stream [{1}], An error occurr streamProcessor.error.stream=Connection [{0}], Stream [{1}], An error occurred during processing that was fatal to the stream streamProcessor.flushBufferedWrite.entry=Connection [{0}], Stream [{1}], Flushing buffered writes streamProcessor.service.error=Error during request processing +streamProcessor.streamReadTimeout=Stream read timeout streamStateMachine.debug.change=Connection [{0}], Stream [{1}], State changed from [{2}] to [{3}] streamStateMachine.invalidFrame=Connection [{0}], Stream [{1}], State [{2}], Frame type [{3}] diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java index 096cc52c30..062221d9a4 100644 --- a/java/org/apache/coyote/http2/Stream.java +++ b/java/org/apache/coyote/http2/Stream.java @@ -1162,6 +1162,8 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter { abstract boolean isRequestBodyFullyRead(); abstract void insertReplayedBody(ByteChunk body); + + protected abstract boolean timeoutRead(long now); } @@ -1188,6 +1190,8 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter { // 'write mode'. private volatile ByteBuffer inBuffer; private volatile boolean readInterest; + // If readInterest is true, data must be available to read no later than this time. + private volatile long readTimeoutExpiry; private volatile boolean closed; private boolean resetReceived; @@ -1287,6 +1291,12 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter { if (!isRequestBodyFullyRead()) { readInterest = true; + long readTimeout = handler.getProtocol().getStreamReadTimeout(); + if (readTimeout > 0) { + readTimeoutExpiry = System.currentTimeMillis() + handler.getProtocol().getStreamReadTimeout(); + } else { + readTimeoutExpiry = Long.MAX_VALUE; + } } return false; @@ -1438,6 +1448,12 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter { } } } + + + @Override + protected boolean timeoutRead(long now) { + return readInterest && now > readTimeoutExpiry; + } } @@ -1499,5 +1515,12 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter { void insertReplayedBody(ByteChunk body) { // NO-OP } + + + @Override + protected boolean timeoutRead(long now) { + // Reading from a saved request. Will never time out. + return false; + } } } diff --git a/java/org/apache/coyote/http2/StreamProcessor.java b/java/org/apache/coyote/http2/StreamProcessor.java index 43865c1ea4..4e5645736e 100644 --- a/java/org/apache/coyote/http2/StreamProcessor.java +++ b/java/org/apache/coyote/http2/StreamProcessor.java @@ -18,6 +18,7 @@ package org.apache.coyote.http2; import java.io.File; import java.io.IOException; +import java.net.SocketTimeoutException; import java.util.Enumeration; import java.util.HashSet; import java.util.Iterator; @@ -25,6 +26,7 @@ import java.util.Set; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; +import jakarta.servlet.RequestDispatcher; import jakarta.servlet.ServletConnection; import jakarta.servlet.http.HttpServletResponse; @@ -553,4 +555,22 @@ class StreamProcessor extends AbstractProcessor { protected final SocketState dispatchEndRequest() throws IOException { return SocketState.CLOSED; } + + + /** + * {@inheritDoc} + * <p> + * First checks for a stream read timeout and processes it if detected. If no stream read timeout is detected then + * the superclass is called to check for an asynchronous processing timeout. + */ + @Override + public void timeoutAsync(long now) { + if (stream.getInputBuffer().timeoutRead(now)) { + stream.getCoyoteRequest().setAttribute(RequestDispatcher.ERROR_EXCEPTION, + new SocketTimeoutException(sm.getString("streamProcessor.streamReadTimeout"))); + processSocketEvent(SocketEvent.ERROR, true); + } else { + super.timeoutAsync(now); + } + } } diff --git a/test/org/apache/coyote/http2/Http2TestBase.java b/test/org/apache/coyote/http2/Http2TestBase.java index 7df1b02a4f..da7dc94ab1 100644 --- a/test/org/apache/coyote/http2/Http2TestBase.java +++ b/test/org/apache/coyote/http2/Http2TestBase.java @@ -113,7 +113,7 @@ public abstract class Http2TestBase extends TomcatBaseTest { protected static final String TRAILER_HEADER_VALUE = "test"; // Client - private Socket s; + protected Socket s; protected HpackEncoder hpackEncoder; protected Input input; protected TestOutput output; diff --git a/test/org/apache/coyote/http2/TestAsyncReadListener.java b/test/org/apache/coyote/http2/TestAsyncReadListener.java new file mode 100644 index 0000000000..8b9a9bc9b8 --- /dev/null +++ b/test/org/apache/coyote/http2/TestAsyncReadListener.java @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.coyote.http2; + +import java.io.IOException; +import java.nio.ByteBuffer; + +import jakarta.servlet.AsyncContext; +import jakarta.servlet.AsyncEvent; +import jakarta.servlet.AsyncListener; +import jakarta.servlet.ReadListener; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServlet; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import org.apache.catalina.Context; +import org.apache.catalina.LifecycleException; +import org.apache.catalina.Wrapper; +import org.apache.catalina.startup.Tomcat; + +public class TestAsyncReadListener extends Http2TestBase { + + private AsyncServlet asyncServlet; + + @Before + public void before() { + asyncServlet = new AsyncServlet(); + } + + @Test + public void testEmptyWindow() throws Exception { + http2Connect(); + + byte[] headersFrameHeader = new byte[9]; + ByteBuffer headersPayload = ByteBuffer.allocate(128); + byte[] dataFrameHeader = new byte[9]; + ByteBuffer dataPayload = ByteBuffer.allocate(256); + byte[] trailerFrameHeader = new byte[9]; + ByteBuffer trailerPayload = ByteBuffer.allocate(256); + + + buildPostRequest(headersFrameHeader, headersPayload, false, null, -1, "/async", dataFrameHeader, dataPayload, + null, trailerFrameHeader, trailerPayload, 3); + + synchronized (asyncServlet) { + // Write the headers + writeFrame(headersFrameHeader, headersPayload); + asyncServlet.wait(4000); + s.close(); + asyncServlet.wait(4000); + } + Assert.assertNotNull(asyncServlet.t); + } + + @Override + protected void configureAndStartWebApplication() throws LifecycleException { + Tomcat tomcat = getTomcatInstance(); + + Context ctxt = getProgrammaticRootContext(); + Tomcat.addServlet(ctxt, "simple", new SimpleServlet()); + ctxt.addServletMappingDecoded("/simple", "simple"); + Wrapper w = Tomcat.addServlet(ctxt, "async", asyncServlet); + w.setAsyncSupported(true); + ctxt.addServletMappingDecoded("/async", "async"); + tomcat.start(); + } + + public static class AsyncServlet extends HttpServlet { + private static final long serialVersionUID = 1L; + private volatile Throwable t; + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { + final AsyncContext asyncContext = req.startAsync(); + asyncContext.getRequest().getInputStream().setReadListener(new ReadListener() { + @Override + public void onDataAvailable() throws IOException { + System.out.println("RL-onDataAvailable"); + synchronized (AsyncServlet.this) { + AsyncServlet.this.notifyAll(); + } + } + + @Override + public void onAllDataRead() throws IOException { + System.out.println("RL-onAllDataRead"); + } + + @Override + public void onError(Throwable throwable) { + System.out.println("RL-onError "); + System.out.println(throwable); + synchronized (AsyncServlet.this) { + t = throwable; + AsyncServlet.this.notifyAll(); + asyncContext.complete(); + } + } + }); + System.out.println("setReadListener"); + asyncContext.addListener(new AsyncListener() { + + @Override + public void onComplete(AsyncEvent event) throws IOException { + System.out.println("AL-onComplete"); + } + + @Override + public void onTimeout(AsyncEvent event) throws IOException { + System.out.println("AL-onTimeout"); + } + + @Override + public void onError(AsyncEvent event) throws IOException { + System.out.println("AL-onError"); + } + + @Override + public void onStartAsync(AsyncEvent event) throws IOException { + System.out.println("AL-onStartAsync"); + } + }); + System.out.println("setAsyncListener"); + } + } +} \ No newline at end of file diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 4204687f55..51c3d00807 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -130,6 +130,10 @@ Fix a crash on Windows setting CA certificate on null path. (remm) </fix> + <fix> + <bug>69068</bug>: Ensure read timouts are triggered for asynchronous, + non-blocking reads when using HTTP/2. (markt) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org