This is an automated email from the ASF dual-hosted git repository. markt-asf pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 059afab9a075a3dda934cbc9f05aee7ac58c8f22 Author: Mark Thomas <[email protected]> AuthorDate: Fri Nov 6 19:00:52 2026 +0000 Always call onError() for async errors reported after a dispatch/complete BZ70290. If an error was reported after the application had already requested a dispatch or a completion on the container thread (for example ErrorReportValve closing a committed response), the async state machine moved to the error state but AsyncListener.onError() was only called if an exception was present. The pending operation was then discarded and the async processing never ended. Make CoyoteAdapter.service() start the async error processing whenever the async state machine is in the error state once the container thread has finished with the request, not only when an exception is present. This calls onError() and, whether the application then calls dispatch() or complete() from it or does nothing, the existing post-processing flushes and closes the connection in the same way ErrorReportValve handles an error for a committed response. Add tests for both error types with onError() doing nothing, dispatching and completing, and for an error following a requested dispatch or completion. Co-Authored-By: Claude Sonnet 5.5 <[email protected]> --- .../apache/catalina/connector/CoyoteAdapter.java | 13 +- .../TestCoyoteAdapterAsyncErrorState.java | 147 ++++++++++++++++++++- test/org/apache/coyote/TestAsyncStateMachine.java | 62 +++++++++ webapps/docs/changelog.xml | 7 +- 4 files changed, 224 insertions(+), 5 deletions(-) diff --git a/java/org/apache/catalina/connector/CoyoteAdapter.java b/java/org/apache/catalina/connector/CoyoteAdapter.java index 7f09798129..f2b1bcdd58 100644 --- a/java/org/apache/catalina/connector/CoyoteAdapter.java +++ b/java/org/apache/catalina/connector/CoyoteAdapter.java @@ -369,9 +369,16 @@ public class CoyoteAdapter implements Adapter { // If an async request was started, is not going to end once // this container thread finishes and an error occurred, trigger - // the async error process - if (!request.isAsyncCompleting() && throwable != null) { - request.getAsyncContextInternal().setErrorState(throwable, true); + // the async error process. The error may have been reported + // without an exception (e.g. the connection being closed after + // the response had been committed) in which case the async + // state machine is already in the error state. + if (!request.isAsyncCompleting()) { + AtomicBoolean asyncError = new AtomicBoolean(false); + res.action(ActionCode.ASYNC_IS_ERROR, asyncError); + if (throwable != null || asyncError.get()) { + request.getAsyncContextInternal().setErrorState(throwable, true); + } } } else { request.finishRequest(); diff --git a/test/org/apache/catalina/connector/TestCoyoteAdapterAsyncErrorState.java b/test/org/apache/catalina/connector/TestCoyoteAdapterAsyncErrorState.java index 85ff3f1756..89de0136a8 100644 --- a/test/org/apache/catalina/connector/TestCoyoteAdapterAsyncErrorState.java +++ b/test/org/apache/catalina/connector/TestCoyoteAdapterAsyncErrorState.java @@ -218,6 +218,113 @@ public class TestCoyoteAdapterAsyncErrorState extends TomcatBaseTest { } + /* + * BZ 70290: The response has been committed (the output was flushed to the client) before the connection went into + * an error state that permits IO and dispatch() was called. The dispatch can not be performed so onComplete() has + * to be called once, the in-progress async count has to be balanced and the output already sent has to have been + * delivered. + */ + @Test + public void testBz70290DispatchAfterCloseCleanWithCommittedResponse() throws Exception { + TestServlet servlet = + new TestServlet(ActionCode.DISABLE_SWALLOW_INPUT).withOutput("FLUSHED-OUTPUT").withFlush(); + startTomcat(servlet); + + String response = sendRequest(); + + Assert.assertTrue(response, response.contains("FLUSHED-OUTPUT")); + assertCompletedOnce(servlet); + Assert.assertEquals("onError() must be called once", 1, servlet.errors.get()); + } + + + /* + * BZ 70290: As above but complete() is called rather than dispatch(). + */ + @Test + public void testBz70290CompleteAfterCloseCleanWithCommittedResponse() throws Exception { + TestServlet servlet = + new TestServlet(ActionCode.DISABLE_SWALLOW_INPUT, true).withOutput("FLUSHED-OUTPUT").withFlush(); + startTomcat(servlet); + + String response = sendRequest(); + + Assert.assertTrue(response, response.contains("FLUSHED-OUTPUT")); + assertCompletedOnce(servlet); + Assert.assertEquals("onError() must be called once", 1, servlet.errors.get()); + } + + + /* + * BZ 70290: As above but the connection is closed (CLOSE_NOW) rather than closed cleanly. + */ + @Test + public void testBz70290DispatchAfterCloseNowWithCommittedResponse() throws Exception { + TestServlet servlet = new TestServlet(ActionCode.CLOSE_NOW).withOutput("FLUSHED-OUTPUT").withFlush(); + startTomcat(servlet); + + sendRequest(); + + assertCompletedOnce(servlet); + } + + + /* + * BZ 70290: When an error occurs after the response has been committed onError() must always be called and + * whatever onError() does (nothing, dispatch() or complete()) has to be handled the way ErrorReportValve handles + * errors for a committed response: the response that was sent is left as it is and the connection is closed. In all + * cases onComplete() has to be called once and the in-progress async count has to be balanced. + */ + private void doCommittedResponseOnError(ActionCode errorAction, String onErrorOperation) throws Exception { + TestServlet servlet = new TestServlet(errorAction).withOutput("FLUSHED-OUTPUT").withFlush() + .withoutInitialOperation().withOnErrorOperation(onErrorOperation); + startTomcat(servlet); + + String response = sendRequest(); + + Assert.assertTrue(response, response.contains("FLUSHED-OUTPUT")); + Assert.assertEquals("onError() must be called once", 1, servlet.errors.get()); + Assert.assertEquals("The dispatch can not be performed", 0, servlet.dispatches.get()); + assertCompletedOnce(servlet); + } + + + @Test + public void testBz70290CommittedCloseCleanOnErrorDoesNothing() throws Exception { + doCommittedResponseOnError(ActionCode.DISABLE_SWALLOW_INPUT, null); + } + + + @Test + public void testBz70290CommittedCloseCleanOnErrorDispatches() throws Exception { + doCommittedResponseOnError(ActionCode.DISABLE_SWALLOW_INPUT, "dispatch"); + } + + + @Test + public void testBz70290CommittedCloseCleanOnErrorCompletes() throws Exception { + doCommittedResponseOnError(ActionCode.DISABLE_SWALLOW_INPUT, "complete"); + } + + + @Test + public void testBz70290CommittedCloseNowOnErrorDoesNothing() throws Exception { + doCommittedResponseOnError(ActionCode.CLOSE_NOW, null); + } + + + @Test + public void testBz70290CommittedCloseNowOnErrorDispatches() throws Exception { + doCommittedResponseOnError(ActionCode.CLOSE_NOW, "dispatch"); + } + + + @Test + public void testBz70290CommittedCloseNowOnErrorCompletes() throws Exception { + doCommittedResponseOnError(ActionCode.CLOSE_NOW, "complete"); + } + + private void startTomcat(TestServlet servlet) throws Exception { Tomcat tomcat = getTomcatInstance(); Context ctx = getProgrammaticRootContext(); @@ -288,16 +395,41 @@ public class TestCoyoteAdapterAsyncErrorState extends TomcatBaseTest { private final ActionCode errorAction; private final boolean complete; private volatile String output; + private volatile boolean flush; + private volatile boolean noInitialOperation; + private volatile String onErrorOperation; private volatile boolean completeFromTimeout; final AtomicInteger dispatches = new AtomicInteger(); final AtomicInteger completions = new AtomicInteger(); + final AtomicInteger errors = new AtomicInteger(); final CountDownLatch completed = new CountDownLatch(1); TestServlet(ActionCode errorAction) { this(errorAction, false); } + /* + * Do not call dispatch() or complete() from service(). The container is left to report the error. + */ + TestServlet withoutInitialOperation() { + this.noInitialOperation = true; + return this; + } + + /* + * Call dispatch() ("dispatch") or complete() ("complete") from AsyncListener.onError() + */ + TestServlet withOnErrorOperation(String operation) { + this.onErrorOperation = operation; + return this; + } + + TestServlet withFlush() { + this.flush = true; + return this; + } + /* * Write the given output to the response before the error occurs. It is deliberately not flushed so it is * still held in the Catalina response buffer when the error occurs. @@ -348,7 +480,12 @@ public class TestCoyoteAdapterAsyncErrorState extends TomcatBaseTest { @Override public void onError(AsyncEvent event) { - // NO-OP + errors.incrementAndGet(); + if ("dispatch".equals(onErrorOperation)) { + event.getAsyncContext().dispatch(); + } else if ("complete".equals(onErrorOperation)) { + event.getAsyncContext().complete(); + } } @Override @@ -364,6 +501,10 @@ public class TestCoyoteAdapterAsyncErrorState extends TomcatBaseTest { writeOutputAndFail(req, resp); + if (noInitialOperation) { + return; + } + // Request the dispatch (or completion) on the container thread during the initial service call if (complete) { ac.complete(); @@ -376,6 +517,10 @@ public class TestCoyoteAdapterAsyncErrorState extends TomcatBaseTest { if (output != null) { resp.setContentType("text/plain"); resp.getWriter().print(output); + if (flush) { + // Commits the response so the status and headers have been sent to the client + resp.flushBuffer(); + } } if (errorAction != null) { diff --git a/test/org/apache/coyote/TestAsyncStateMachine.java b/test/org/apache/coyote/TestAsyncStateMachine.java index 423269457e..0407613cfe 100644 --- a/test/org/apache/coyote/TestAsyncStateMachine.java +++ b/test/org/apache/coyote/TestAsyncStateMachine.java @@ -200,6 +200,68 @@ public class TestAsyncStateMachine { } + /* + * BZ 70290: An error that prevents further IO is raised (e.g. by ErrorReportValve closing a committed response) + * after dispatch() has been requested but before the container thread has left the initial service call + * (MUST_DISPATCH). The error must be recorded (so that onError() can be called) rather than ignored. A + * subsequent call to dispatch() from onError() is valid but, as the connection is going to be closed, can not be + * performed so the async processing must end. + */ + @Test + public void testBz70290ErrorAfterDispatchRequestedThenDispatchFromOnError() throws IOException { + processor.action(ActionCode.ASYNC_DISPATCH, null); + + processor.setErrorState(ErrorState.CLOSE_NOW, new IOException()); + + Assert.assertTrue("The error must be recorded so onError() is called", isAsyncError()); + + // AsyncListener.onError() calls dispatch() + processor.action(ActionCode.ASYNC_DISPATCH, null); + Assert.assertTrue(isAsyncDispatching()); + + Assert.assertEquals(SocketState.ASYNC_END, processor.asyncPostProcess()); + assertCompleted(); + } + + + /* + * BZ 70290: As above but onError() calls complete(). + */ + @Test + public void testBz70290ErrorAfterDispatchRequestedThenCompleteFromOnError() throws IOException { + processor.action(ActionCode.ASYNC_DISPATCH, null); + + processor.setErrorState(ErrorState.CLOSE_NOW, new IOException()); + + Assert.assertTrue("The error must be recorded so onError() is called", isAsyncError()); + + // AsyncListener.onError() calls complete() + processor.action(ActionCode.ASYNC_COMPLETE, null); + + Assert.assertEquals(SocketState.ASYNC_END, processor.asyncPostProcess()); + assertCompleted(); + } + + + /* + * BZ 70290: As above for an error raised after complete() has been requested (MUST_COMPLETE) and onError() then + * calls dispatch(). + */ + @Test + public void testBz70290ErrorAfterCompleteRequestedThenDispatchFromOnError() throws IOException { + processor.action(ActionCode.ASYNC_COMPLETE, null); + + processor.setErrorState(ErrorState.CLOSE_NOW, new IOException()); + + Assert.assertTrue("The error must be recorded so onError() is called", isAsyncError()); + + processor.action(ActionCode.ASYNC_DISPATCH, null); + + Assert.assertEquals(SocketState.ASYNC_END, processor.asyncPostProcess()); + assertCompleted(); + } + + /* * BZ 70290: Control. The equivalent complete() handling (MUST_COMPLETE) in an error state already balanced the * count. Confirm the dispatch and complete cases behave consistently. diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index ffacb0131c..be23a83b3c 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -353,7 +353,12 @@ <code>onComplete()</code> was not called and the in-progress async count was not decremented. In addition, when the error state still permits I/O, output buffered before the request was completed is now - written rather than being discarded. (markt) + written rather than being discarded. Also fix a case where the async + processing never ended if the response had been committed and the + connection was then closed after the application had requested a + dispatch or completion, and ensure + <code>AsyncListener.onError()</code> is called when the container + reports an error without an exception. (markt) </fix> </changelog> </subsection> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
