You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2019/10/15 20:40:28 UTC
[tomcat] 01/05: Expand test for
https://bz.apache.org/bugzilla/show_bug.cgi?id=63816
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
commit 7c0a689e78512686978a8ce71ca7d4cd2216fa8e
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 55a350d..98c79ce 100644
--- a/java/org/apache/coyote/AbstractProcessor.java
+++ b/java/org/apache/coyote/AbstractProcessor.java
@@ -101,7 +101,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
@@ -113,7 +115,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 81a883b..5d0e4e5 100644
--- a/java/org/apache/coyote/AsyncStateMachine.java
+++ b/java/org/apache/coyote/AsyncStateMachine.java
@@ -154,6 +154,7 @@ public 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 @@ public 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 @@ public 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 @@ public class AsyncStateMachine {
public 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 d7afe2d..ccf3228 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 dbcaef7..f3b6a80 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -63,9 +63,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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org