This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push: new a80693f Expand test for https://bz.apache.org/bugzilla/show_bug.cgi?id=63816 a80693f is described below commit a80693f3d4f9c2628f9f1c978f65af9bef8ce04e Author: Mark Thomas <ma...@apache.org> AuthorDate: Tue Oct 15 14:18:26 2019 +0100 Expand test for https://bz.apache.org/bugzilla/show_bug.cgi?id=63816 Expanded to cover https://bz.apache.org/bugzilla/show_bug.cgi?id=63817 and additional, similar scenarios. Correct bugs in existing tests. --- java/org/apache/coyote/AbstractProcessor.java | 6 +- java/org/apache/coyote/AsyncStateMachine.java | 11 +- .../apache/catalina/core/TestAsyncContextImpl.java | 7 +- .../core/TestAsyncContextStateChanges.java | 317 +++++++++++++++++++++ webapps/docs/changelog.xml | 6 +- 5 files changed, 334 insertions(+), 13 deletions(-) diff --git a/java/org/apache/coyote/AbstractProcessor.java b/java/org/apache/coyote/AbstractProcessor.java index d5c3ee1..3c0c3b9 100644 --- a/java/org/apache/coyote/AbstractProcessor.java +++ b/java/org/apache/coyote/AbstractProcessor.java @@ -98,7 +98,9 @@ public abstract class AbstractProcessor extends AbstractProcessorLight implement * @param t The error which occurred */ protected void setErrorState(ErrorState errorState, Throwable t) { - response.setError(); + // Use the return value to avoid processing more than one async error + // in a single async cycle. + boolean setError = response.setError(); boolean blockIo = this.errorState.isIoAllowed() && !errorState.isIoAllowed(); this.errorState = this.errorState.getMostSevere(errorState); // Don't change the status code for IOException since that is almost @@ -110,7 +112,7 @@ public abstract class AbstractProcessor extends AbstractProcessorLight implement if (t != null) { request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, t); } - if (blockIo && isAsync()) { + if (blockIo && isAsync() && setError) { if (asyncStateMachine.asyncError()) { processSocketEvent(SocketEvent.ERROR, true); } diff --git a/java/org/apache/coyote/AsyncStateMachine.java b/java/org/apache/coyote/AsyncStateMachine.java index 00191ba..b4fa4a4 100644 --- a/java/org/apache/coyote/AsyncStateMachine.java +++ b/java/org/apache/coyote/AsyncStateMachine.java @@ -154,6 +154,7 @@ class AsyncStateMachine { DISPATCH_PENDING(true, true, false, false), DISPATCHING (true, false, false, true), READ_WRITE_OP (true, true, false, false), + MUST_ERROR (true, true, false, false), ERROR (true, true, false, false); private final boolean isAsync; @@ -320,7 +321,7 @@ class AsyncStateMachine { private synchronized boolean doComplete() { clearNonBlockingListeners(); boolean triggerDispatch = false; - if (state == AsyncState.STARTING) { + if (state == AsyncState.STARTING || state == AsyncState.MUST_ERROR) { // Processing is on a container thread so no need to transfer // processing to a new container thread state = AsyncState.MUST_COMPLETE; @@ -386,7 +387,7 @@ class AsyncStateMachine { private synchronized boolean doDispatch() { clearNonBlockingListeners(); boolean triggerDispatch = false; - if (state == AsyncState.STARTING) { + if (state == AsyncState.STARTING || state == AsyncState.MUST_ERROR) { // Processing is on a container thread so no need to transfer // processing to a new container thread state = AsyncState.MUST_DISPATCH; @@ -435,7 +436,11 @@ class AsyncStateMachine { synchronized boolean asyncError() { clearNonBlockingListeners(); - state = AsyncState.ERROR; + if (state == AsyncState.STARTING) { + state = AsyncState.MUST_ERROR; + } else { + state = AsyncState.ERROR; + } return !ContainerThreadMarker.isContainerThread(); } diff --git a/test/org/apache/catalina/core/TestAsyncContextImpl.java b/test/org/apache/catalina/core/TestAsyncContextImpl.java index 35165b6..f01aba5 100644 --- a/test/org/apache/catalina/core/TestAsyncContextImpl.java +++ b/test/org/apache/catalina/core/TestAsyncContextImpl.java @@ -851,6 +851,8 @@ public class TestAsyncContextImpl extends TomcatBaseTest { private final boolean completeOnError; private final boolean completeOnTimeout; private final String dispatchUrl; + // Assumes listener is fired after container thread that initiated async + // has exited. private boolean asyncStartedCorrect = true; public TrackingListener(boolean completeOnError, @@ -1901,7 +1903,6 @@ public class TestAsyncContextImpl extends TomcatBaseTest { } Assert.assertEquals(expectedTrack, getTrack()); - Assert.assertTrue(bug59219Servlet.isAsyncStartedCorrect()); } @@ -1936,10 +1937,6 @@ public class TestAsyncContextImpl extends TomcatBaseTest { } else throw new ServletException(); } - - public boolean isAsyncStartedCorrect() { - return trackingListener.isAsyncStartedCorrect(); - } } @Test diff --git a/test/org/apache/catalina/core/TestAsyncContextStateChanges.java b/test/org/apache/catalina/core/TestAsyncContextStateChanges.java new file mode 100644 index 0000000..5df1740 --- /dev/null +++ b/test/org/apache/catalina/core/TestAsyncContextStateChanges.java @@ -0,0 +1,317 @@ +/* + * 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.catalina.core; + +import java.io.IOException; +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicBoolean; + +import javax.servlet.AsyncContext; +import javax.servlet.AsyncEvent; +import javax.servlet.AsyncListener; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; + +import org.apache.catalina.Context; +import org.apache.catalina.Wrapper; +import org.apache.catalina.connector.TestCoyoteAdapter; +import org.apache.catalina.startup.SimpleHttpClient; +import org.apache.catalina.startup.Tomcat; +import org.apache.catalina.startup.TomcatBaseTest; + +/* + * Derived from a test for https://bz.apache.org/bugzilla/show_bug.cgi?id=63816 + * Expanded to cover https://bz.apache.org/bugzilla/show_bug.cgi?id=63817 and + * additional scenarios. + */ +@RunWith(Parameterized.class) +public class TestAsyncContextStateChanges extends TomcatBaseTest { + + @Parameterized.Parameters(name = "{index}: end [{0}], timing [{1}]") + public static Collection<Object[]> parameters() { + List<Object[]> parameterSets = new ArrayList<>(); + for (AsyncEnd asyncEnd : AsyncEnd.values()) { + for (EndTiming endTiming : EndTiming.values()) { + parameterSets.add(new Object[] { asyncEnd, endTiming }); + } + } + return parameterSets; + } + + @Parameter(0) + public AsyncEnd asyncEnd; + + @Parameter(1) + public EndTiming endTiming; + + private ServletRequest servletRequest = null; + private AsyncContext asyncContext = null; + private AtomicBoolean failed = new AtomicBoolean(); + private CountDownLatch threadLatch; + private CountDownLatch closeLatch; + private CountDownLatch endLatch; + private boolean dispatch; + + @Test + public void testAsync() throws Exception { + dispatch = false; + servletRequest = null; + asyncContext = null; + + // Initialise tracking fields + failed.set(true); + threadLatch = new CountDownLatch(1); + closeLatch = new CountDownLatch(1); + endLatch = new CountDownLatch(1); + + // Setup Tomcat instance + Tomcat tomcat = getTomcatInstance(); + + // No file system docBase required + Context ctx = tomcat.addContext("", null); + + AsyncServlet bug63816Servlet = new AsyncServlet(); + Wrapper wrapper = Tomcat.addServlet(ctx, "bug63816Servlet", bug63816Servlet); + wrapper.setAsyncSupported(true); + ctx.addServletMappingDecoded("/*", "bug63816Servlet"); + + tomcat.start(); + + Client client = new Client(); + client.setPort(getPort()); + client.setRequest(new String[] { "GET / HTTP/1.1" + SimpleHttpClient.CRLF + + "Host: localhost:" + SimpleHttpClient.CRLF + + SimpleHttpClient.CRLF}); + client.connect(); + client.sendRequest(); + + if (asyncEnd.isError()) { + client.disconnect(); + closeLatch.countDown(); + try { + endLatch.await(); + } catch (InterruptedException e) { + // Ignore + } + } else { + client.setUseContentLength(true); + client.readResponse(true); + } + + Assert.assertFalse(failed.get()); + } + + + private static final class Client extends SimpleHttpClient { + + @Override + public boolean isResponseBodyOK() { + return true; + } + } + + + private final class AsyncServlet extends HttpServlet { + + private static final long serialVersionUID = 1L; + + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) + throws ServletException, IOException { + if (dispatch) { + return; + } + + if (!asyncEnd.isError()) { + resp.setContentType("text/plain"); + resp.setCharacterEncoding("UTF-8"); + resp.setContentLength(2); + resp.getWriter().print("OK"); + } + + servletRequest = req; + asyncContext = req.startAsync(); + asyncContext.addListener(new Listener()); + asyncContext.setTimeout(1000); + Thread t = new NonContainerThread(); + + switch (endTiming) { + case INLINE: { + t.run(); + break; + } + case THREAD_AFTER_EXIT: { + t.start(); + threadLatch.countDown(); + break; + } + case THREAD_BEFORE_EXIT: { + t.start(); + try { + threadLatch.await(); + } catch (InterruptedException e) { + // Ignore + } + endLatch.countDown(); + break; + } + } + } + } + + + private final class NonContainerThread extends Thread { + + @Override + public void run() { + if (endTiming == EndTiming.THREAD_AFTER_EXIT) { + try { + threadLatch.await(); + } catch (InterruptedException e) { + // Ignore + } + } + + // Trigger the error if necessary + if (asyncEnd.isError()) { + try { + closeLatch.await(); + } catch (InterruptedException e) { + // Ignore + } + try { + ServletResponse resp = asyncContext.getResponse(); + resp.setContentType("text/plain"); + resp.setCharacterEncoding("UTF-8"); + OutputStream os = resp.getOutputStream(); + resp.setContentType("text/plain"); + for (int i = 0; i < 16; i++) { + os.write(TestCoyoteAdapter.TEXT_8K.getBytes(StandardCharsets.UTF_8)); + } + } catch (IOException e) { + // Expected + } + } + + try { + switch (asyncEnd) { + case COMPLETE: + case ERROR_COMPLETE: { + asyncContext.complete(); + break; + } + case DISPATCH: + case ERROR_DISPATCH: { + dispatch = true; + asyncContext.dispatch(); + break; + } + case NONE: + case ERROR_NONE: { + break; + } + } + + // The request must stay in async mode until doGet() exists + if (servletRequest.isAsyncStarted() != (endTiming == EndTiming.THREAD_AFTER_EXIT && !asyncEnd.isNone())) { + failed.set(false); + } + } finally { + if (endTiming == EndTiming.THREAD_BEFORE_EXIT) { + threadLatch.countDown(); + } else if (endTiming == EndTiming.THREAD_AFTER_EXIT) { + endLatch.countDown(); + } + } + } + } + + + private class Listener implements AsyncListener { + + @Override + public void onComplete(AsyncEvent event) throws IOException { + if (endTiming == EndTiming.INLINE) { + endLatch.countDown(); + } + } + + @Override + public void onTimeout(AsyncEvent event) throws IOException { + // NO-OP + } + + @Override + public void onError(AsyncEvent event) throws IOException { + // NO-OP + } + + @Override + public void onStartAsync(AsyncEvent event) throws IOException { + // NO-OP + } + } + + + public enum AsyncEnd { + + NONE ( true, false), + COMPLETE (false, false), + DISPATCH (false, false), + ERROR_NONE ( true, true), + ERROR_COMPLETE(false, true), + ERROR_DISPATCH(false, true); + + final boolean none; + final boolean error; + + private AsyncEnd(boolean none, boolean error) { + this.none = none; + this.error = error; + } + + public boolean isNone() { + return none; + } + + public boolean isError() { + return error; + } + } + + + public enum EndTiming { + INLINE, + THREAD_BEFORE_EXIT, + THREAD_AFTER_EXIT + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index c36ce45..6eb0435 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -67,9 +67,9 @@ <code>AsyncListener.onError()</code>. (markt) </fix> <fix> - <bug>63816</bug>: Correctly handle I/O errors on a non-container thread - after asynchronous processing has been started but before the container - thread that started asynchronous processing has completed processing the + <bug>63816</bug> and <bug>63817</bug>: Correctly handle I/O errors after + asynchronous processing has been started but before the container thread + that started asynchronous processing has completed processing the current request/response. (markt) </fix> </changelog> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org