This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit e50fb5e92407071ec830acf4502de5fb470944ce Author: opencode <[email protected]> AuthorDate: Mon Oct 12 22:18:43 2026 +0200 Keep asynchronous timeouts in place until they are processed The async timeout task previously cleared the timeout as soon as it dispatched a timeout event, to avoid multiple timeouts. Dispatching is not the same as delivery, though. The event is queued for processing on a container thread and can be discarded before it is ever processed: the dispatch is rejected when the executor is saturated, or the event is skipped as stale once another event has already taken the processor for the socket. In those cases the timeout was never re-armed and the asynchronous request ran on without ever receiving an onTimeout callback. Instead, keep the timeout armed and clear it only when the event reaches the processor in the ASYNC_TIMEOUT action. At that point the generation is also marked as handled, so any duplicate event that is already queued is treated as stale rather than being delivered a second time. Until delivery, the timeout task simply retries once per second, matching the existing behaviour for stopped web applications and the HTTP/2 connection level timeout task. --- java/org/apache/coyote/AbstractProcessor.java | 21 +++- .../coyote/TestAbstractProcessorAsyncTimeout.java | 128 +++++++++++++++++++++ webapps/docs/changelog.xml | 8 ++ 3 files changed, 155 insertions(+), 2 deletions(-) diff --git a/java/org/apache/coyote/AbstractProcessor.java b/java/org/apache/coyote/AbstractProcessor.java index 3b8b9cb73a..2fdfee6b2e 100644 --- a/java/org/apache/coyote/AbstractProcessor.java +++ b/java/org/apache/coyote/AbstractProcessor.java @@ -57,6 +57,11 @@ public abstract class AbstractProcessor extends AbstractProcessorLight implement /** State machine for async processing. */ protected final AsyncStateMachine asyncStateMachine; private volatile long asyncTimeout = -1; + /** + * Value for asyncTimeoutGeneration that marks a timeout as already processed, so that any event that is still + * queued for the generation it was dispatched for is treated as stale. + */ + private static final long NO_TIMEOUT_GENERATION = -1; /* * Tracks the current async generation when a timeout is dispatched. In the time it takes for a container thread to * be allocated and the timeout processing to start, it is possible that the application completes this generation @@ -624,6 +629,13 @@ public abstract class AbstractProcessor extends AbstractProcessorLight implement break; } case ASYNC_TIMEOUT: { + /* + * The event has reached the processor, so the timeout no longer needs to be dispatched by the timeout + * task. Mark the generation as handled so that any event that is already queued for this timeout is + * treated as stale rather than delivered a second time. + */ + setAsyncTimeout(-1); + asyncTimeoutGeneration = NO_TIMEOUT_GENERATION; AtomicBoolean result = (AtomicBoolean) param; result.set(asyncStateMachine.asyncTimeout()); break; @@ -753,8 +765,13 @@ public abstract class AbstractProcessor extends AbstractProcessorLight implement private void doTimeoutAsync() { - // Avoid multiple timeouts - setAsyncTimeout(-1); + /* + * The timeout is deliberately not cleared here. It is cleared when the event is processed, when the timeout + * task will stop dispatching it (see the ASYNC_TIMEOUT handling in action()). Clearing it here would mean that + * an event that is not processed - for example, because another thread was processing the socket when the + * event was dispatched, so the dispatch was skipped as stale - is never re-dispatched and the asynchronous + * request is left running without a timeout. + */ asyncTimeoutGeneration = asyncStateMachine.getCurrentGeneration(); processSocketEvent(SocketEvent.TIMEOUT, true); } diff --git a/test/org/apache/coyote/TestAbstractProcessorAsyncTimeout.java b/test/org/apache/coyote/TestAbstractProcessorAsyncTimeout.java new file mode 100644 index 0000000000..a6ff23163a --- /dev/null +++ b/test/org/apache/coyote/TestAbstractProcessorAsyncTimeout.java @@ -0,0 +1,128 @@ +/* + * 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; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import org.apache.coyote.http11.Http11NioProtocol; +import org.apache.coyote.http11.Http11Processor; +import org.apache.tomcat.util.net.SocketEvent; + +/** + * Tests the dispatching of the asynchronous timeout by the timeout task of the protocol, which runs + * {@link AbstractProcessor#timeoutAsync(long)} once per second for every processor that is waiting. + */ +public class TestAbstractProcessorAsyncTimeout { + + private TestProcessor processor; + private TestAsyncContextCallback asyncCtxt; + + @Before + public void setupTest() { + processor = new TestProcessor(); + asyncCtxt = new TestAsyncContextCallback(); + + processor.action(ActionCode.ASYNC_START, asyncCtxt); + processor.action(ActionCode.ASYNC_POST_PROCESS, null); + processor.setAsyncTimeout(1000); + } + + @Test + public void testTimeoutIsDispatchedUntilItIsProcessed() { + // A dispatched event is not necessarily processed. It may be discarded, for example if another thread is + // processing the socket when the event is dispatched, in which case the timeout remains in place so that the + // next run of the timeout task dispatches it again. + long asyncStart = processor.asyncStateMachine.getLastAsyncStart(); + + processor.timeoutAsync(asyncStart + 2000); + + Assert.assertEquals("The timeout should have been dispatched", List.of(SocketEvent.TIMEOUT), + processor.getDispatchedEvents()); + Assert.assertTrue("The timeout must remain in place until it has been processed", + processor.getAsyncTimeout() > 0); + + // The event was not processed, so this run of the timeout task has to dispatch it again. + processor.timeoutAsync(asyncStart + 4000); + + Assert.assertEquals("The timeout should have been dispatched again", + List.of(SocketEvent.TIMEOUT, SocketEvent.TIMEOUT), processor.getDispatchedEvents()); + + // Processing the event removes the timeout and marks the generation as handled, so any event that is already + // queued for it is ignored. + AtomicBoolean result = new AtomicBoolean(); + processor.action(ActionCode.ASYNC_TIMEOUT, result); + Assert.assertTrue("The timeout should have been processed", result.get()); + Assert.assertEquals(-1, processor.getAsyncTimeout()); + Assert.assertFalse("A second event for the same timeout should be stale", + processor.checkAsyncTimeoutGeneration()); + + // Once processed, the timeout task must not dispatch it again. + processor.timeoutAsync(asyncStart + 6000); + + Assert.assertEquals("The timeout must not be dispatched once it has been processed", + List.of(SocketEvent.TIMEOUT, SocketEvent.TIMEOUT), processor.getDispatchedEvents()); + } + + private static class TestProcessor extends Http11Processor { + + private final List<SocketEvent> dispatchedEvents = new ArrayList<>(); + + TestProcessor() { + super(new Http11NioProtocol(), null); + } + + @Override + protected void processSocketEvent(SocketEvent event, boolean dispatch) { + // Log the event rather than passing it on to the socket. Tests can then determine which events would have + // been dispatched without requiring a socket, an endpoint or an executor. + dispatchedEvents.add(event); + } + + List<SocketEvent> getDispatchedEvents() { + return dispatchedEvents; + } + } + + private static class TestAsyncContextCallback implements AsyncContextCallback { + + @Override + public void fireOnComplete() { + // NO-OP + } + + @Override + public boolean isAvailable() { + return true; + } + + @Override + public void incrementInProgressAsyncCount() { + // NO-OP + } + + @Override + public void decrementInProgressAsyncCount() { + // NO-OP + } + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 763d9ac374..0e35123b00 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -395,6 +395,14 @@ </subsection> <subsection name="Coyote"> <changelog> + <fix> + Keep the asynchronous timeout in place until the timeout event has + been processed, rather than clearing it when the event is dispatched. + A dispatched event may be discarded without being processed, for + example when no container thread is available to process it, which + previously left the asynchronous request running without any timeout. + (remm) + </fix> <update> Change the default value of the <code>cookiesWithoutEquals</code> attribute of the <code>Rfc6265CookieProcessor</code> from --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
