This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 8.5.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/8.5.x by this push: new d91f848 asyncStarted should be false once complete/dispatch in onTimeout d91f848 is described below commit d91f848dbb9b20b2649db918af901c841b796ed8 Author: Mark Thomas <ma...@apache.org> AuthorDate: Thu Oct 10 16:57:55 2019 +0100 asyncStarted should be false once complete/dispatch in onTimeout --- java/org/apache/coyote/AsyncStateMachine.java | 16 +++++---- .../apache/catalina/core/TestAsyncContextImpl.java | 38 +++++++++++++++++++--- webapps/docs/changelog.xml | 10 ++++++ 3 files changed, 54 insertions(+), 10 deletions(-) diff --git a/java/org/apache/coyote/AsyncStateMachine.java b/java/org/apache/coyote/AsyncStateMachine.java index 2c2c582..0d364f4 100644 --- a/java/org/apache/coyote/AsyncStateMachine.java +++ b/java/org/apache/coyote/AsyncStateMachine.java @@ -321,8 +321,7 @@ public class AsyncStateMachine { private synchronized boolean doComplete() { clearNonBlockingListeners(); boolean triggerDispatch = false; - if (state == AsyncState.STARTING || state == AsyncState.TIMING_OUT || - state == AsyncState.ERROR) { + if (state == AsyncState.STARTING || state == AsyncState.ERROR) { // Processing is on a container thread so no need to transfer // processing to a new container thread state = AsyncState.MUST_COMPLETE; @@ -335,10 +334,13 @@ public class AsyncStateMachine { // request/response associated with the AsyncContext so need a new // container thread to process the different request/response. triggerDispatch = true; - } else if (state == AsyncState.READ_WRITE_OP) { + } else if (state == AsyncState.READ_WRITE_OP || state == AsyncState.TIMING_OUT) { // Read/write operations can happen on or off a container thread but // while in this state the call to listener that triggers the // read/write will be in progress on a container thread. + // Processing of timeouts can happen on or off a container thread + // (on is much more likely) but while in this state the call that + // triggers the timeout will be in progress on a container thread. // The socket will be added to the poller when the container thread // exits the AbstractConnectionHandler.process() method so don't do // a dispatch here which would add it to the poller a second time. @@ -383,8 +385,7 @@ public class AsyncStateMachine { private synchronized boolean doDispatch() { clearNonBlockingListeners(); boolean triggerDispatch = false; - if (state == AsyncState.STARTING || state == AsyncState.TIMING_OUT || - state == AsyncState.ERROR) { + if (state == AsyncState.STARTING || state == AsyncState.ERROR) { // Processing is on a container thread so no need to transfer // processing to a new container thread state = AsyncState.MUST_DISPATCH; @@ -397,10 +398,13 @@ public class AsyncStateMachine { // request/response associated with the AsyncContext so need a new // container thread to process the different request/response. triggerDispatch = true; - } else if (state == AsyncState.READ_WRITE_OP) { + } else if (state == AsyncState.READ_WRITE_OP || state == AsyncState.TIMING_OUT) { // Read/write operations can happen on or off a container thread but // while in this state the call to listener that triggers the // read/write will be in progress on a container thread. + // Processing of timeouts can happen on or off a container thread + // (on is much more likely) but while in this state the call that + // triggers the timeout will be in progress on a container thread. // The socket will be added to the poller when the container thread // exits the AbstractConnectionHandler.process() method so don't do // a dispatch here which would add it to the poller a second time. diff --git a/test/org/apache/catalina/core/TestAsyncContextImpl.java b/test/org/apache/catalina/core/TestAsyncContextImpl.java index 32bad39..6e01bbe 100644 --- a/test/org/apache/catalina/core/TestAsyncContextImpl.java +++ b/test/org/apache/catalina/core/TestAsyncContextImpl.java @@ -555,19 +555,25 @@ public class TestAsyncContextImpl extends TomcatBaseTest { alv.validateAccessLog(1, 200, timeoutDelay, timeoutDelay + TIMEOUT_MARGIN + REQUEST_TIME); } + + Assert.assertTrue(timeout.isAsyncStartedCorrect()); } private static class TimeoutServlet extends HttpServlet { private static final long serialVersionUID = 1L; private final Boolean completeOnTimeout; - private final String dispatchUrl; + private final TrackingListener trackingListener; public static final long ASYNC_TIMEOUT = 100; public TimeoutServlet(Boolean completeOnTimeout, String dispatchUrl) { this.completeOnTimeout = completeOnTimeout; - this.dispatchUrl = dispatchUrl; + if (completeOnTimeout == null) { + this.trackingListener = null; + } else { + this.trackingListener = new TrackingListener(false, completeOnTimeout.booleanValue(), dispatchUrl); + } } @Override @@ -579,13 +585,19 @@ public class TestAsyncContextImpl extends TomcatBaseTest { ac.setTimeout(ASYNC_TIMEOUT); if (completeOnTimeout != null) { - ac.addListener(new TrackingListener(false, - completeOnTimeout.booleanValue(), dispatchUrl)); + ac.addListener(trackingListener); } } else { resp.getWriter().print("FAIL: Async unsupported"); } } + + public boolean isAsyncStartedCorrect() { + if (trackingListener == null) { + return true; + } + return trackingListener.isAsyncStartedCorrect(); + } } @Test @@ -827,6 +839,7 @@ public class TestAsyncContextImpl extends TomcatBaseTest { private final boolean completeOnError; private final boolean completeOnTimeout; private final String dispatchUrl; + private boolean asyncStartedCorrect = true; public TrackingListener(boolean completeOnError, boolean completeOnTimeout, String dispatchUrl) { @@ -842,27 +855,44 @@ public class TestAsyncContextImpl extends TomcatBaseTest { @Override public void onTimeout(AsyncEvent event) throws IOException { + boolean expectedAsyncStarted = true; + TestAsyncContextImpl.track("onTimeout-"); if (completeOnTimeout){ event.getAsyncContext().complete(); + expectedAsyncStarted = false; } if (dispatchUrl != null) { event.getAsyncContext().dispatch(dispatchUrl); + expectedAsyncStarted = false; } + + ServletRequest req = event.getSuppliedRequest(); + asyncStartedCorrect = (expectedAsyncStarted == req.isAsyncStarted()); } @Override public void onError(AsyncEvent event) throws IOException { + boolean expectedAsyncStarted = true; + TestAsyncContextImpl.track("onError-"); if (completeOnError) { event.getAsyncContext().complete(); + expectedAsyncStarted = false; } + + ServletRequest req = event.getSuppliedRequest(); + asyncStartedCorrect = (expectedAsyncStarted == req.isAsyncStarted()); } @Override public void onStartAsync(AsyncEvent event) throws IOException { TestAsyncContextImpl.track("onStartAsync-"); } + + public boolean isAsyncStartedCorrect() { + return asyncStartedCorrect; + } } private static class StickyTrackingListener extends TrackingListener { diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index aac8e06..9af29b4 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -45,6 +45,16 @@ issues do not "pop up" wrt. others). --> <section name="Tomcat 8.5.48 (markt)" rtext="in development"> + <subsection name="Coyote"> + <changelog> + <fix> + Ensure that <code>ServletRequest.isAsyncStarted()</code> returns + <code>false</code> once <code>AsyncContext.complete()</code> or + <code>AsyncContext.dispatch()</code> has been called during + <code>AsyncListener.onTimeout()</code>. (markt) + </fix> + </changelog> + </subsection> </section> <section name="Tomcat 8.5.47 (markt)" rtext="release in progress"> <subsection name="Coyote"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org