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/14 15:20:19 UTC
[tomcat] branch master updated (4892eda -> 327c553)
This is an automated email from the ASF dual-hosted git repository.
markt pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git.
from 4892eda Update to Tomcat 9.0.27
new 44a50ce Add test case for bug 63816
new 85289c3 Add debug log messages for the triggering of async listener events
new 327c553 Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63816 async errors
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../org/apache/catalina/core/AsyncContextImpl.java | 12 ++
.../apache/catalina/core/LocalStrings.properties | 4 +
java/org/apache/coyote/AbstractProcessor.java | 13 +-
java/org/apache/coyote/AsyncStateMachine.java | 32 +----
.../apache/catalina/core/TestAsyncContextImpl.java | 149 +++++++++++++++++++++
webapps/docs/changelog.xml | 6 +
6 files changed, 178 insertions(+), 38 deletions(-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] 03/03: Fix
https://bz.apache.org/bugzilla/show_bug.cgi?id=63816 async errors
Posted by Mark Thomas <ma...@apache.org>.
On 14/10/2019 16:20, markt@apache.org wrote:
> 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
>
> commit 327c5532f3f3123c937572541e56175851d73c48
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Mon Oct 14 16:18:17 2019 +0100
>
> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63816 async errors
>
> 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 current
> request/response.
Just a heads up that, while all the test cases pass, I think this patch
may over-simplify some edge cases. I'm planning on working on BZ 63817
next and the fix for that issue may end up re-instating some of the code
removed by this fix.
I plan to update/replace the state diagram once the code changes are
complete.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 03/03: Fix
https://bz.apache.org/bugzilla/show_bug.cgi?id=63816 async errors
Posted by ma...@apache.org.
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
commit 327c5532f3f3123c937572541e56175851d73c48
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Oct 14 16:18:17 2019 +0100
Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63816 async errors
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 current
request/response.
---
java/org/apache/coyote/AbstractProcessor.java | 13 +++--------
java/org/apache/coyote/AsyncStateMachine.java | 32 ++++-----------------------
webapps/docs/changelog.xml | 6 +++++
3 files changed, 13 insertions(+), 38 deletions(-)
diff --git a/java/org/apache/coyote/AbstractProcessor.java b/java/org/apache/coyote/AbstractProcessor.java
index fc32745..d5c3ee1 100644
--- a/java/org/apache/coyote/AbstractProcessor.java
+++ b/java/org/apache/coyote/AbstractProcessor.java
@@ -110,17 +110,10 @@ public abstract class AbstractProcessor extends AbstractProcessorLight implement
if (t != null) {
request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, t);
}
- if (blockIo && !ContainerThreadMarker.isContainerThread() && isAsync()) {
- // The error occurred on a non-container thread during async
- // processing which means not all of the necessary clean-up will
- // have been completed. Dispatch to a container thread to do the
- // clean-up. Need to do it this way to ensure that all the necessary
- // clean-up is performed.
- asyncStateMachine.asyncMustError();
- if (getLog().isDebugEnabled()) {
- getLog().debug(sm.getString("abstractProcessor.nonContainerThreadError"), t);
+ if (blockIo && isAsync()) {
+ if (asyncStateMachine.asyncError()) {
+ processSocketEvent(SocketEvent.ERROR, true);
}
- processSocketEvent(SocketEvent.ERROR, true);
}
}
diff --git a/java/org/apache/coyote/AsyncStateMachine.java b/java/org/apache/coyote/AsyncStateMachine.java
index 5fe1ecc..00191ba 100644
--- a/java/org/apache/coyote/AsyncStateMachine.java
+++ b/java/org/apache/coyote/AsyncStateMachine.java
@@ -154,7 +154,6 @@ 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;
@@ -434,36 +433,13 @@ class AsyncStateMachine {
}
- synchronized void asyncMustError() {
- if (state == AsyncState.STARTED) {
- clearNonBlockingListeners();
- state = AsyncState.MUST_ERROR;
- } else {
- throw new IllegalStateException(
- sm.getString("asyncStateMachine.invalidAsyncState",
- "asyncMustError()", state));
- }
+ synchronized boolean asyncError() {
+ clearNonBlockingListeners();
+ state = AsyncState.ERROR;
+ return !ContainerThreadMarker.isContainerThread();
}
- synchronized void asyncError() {
- if (state == AsyncState.STARTING ||
- state == AsyncState.STARTED ||
- state == AsyncState.DISPATCHED ||
- state == AsyncState.TIMING_OUT ||
- state == AsyncState.MUST_COMPLETE ||
- state == AsyncState.READ_WRITE_OP ||
- state == AsyncState.COMPLETING ||
- state == AsyncState.MUST_ERROR) {
- clearNonBlockingListeners();
- state = AsyncState.ERROR;
- } else {
- throw new IllegalStateException(
- sm.getString("asyncStateMachine.invalidAsyncState",
- "asyncError()", state));
- }
- }
-
synchronized void asyncRun(Runnable runnable) {
if (state == AsyncState.STARTING || state == AsyncState.STARTED ||
state == AsyncState.READ_WRITE_OP) {
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index a860c45..c36ce45 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -66,6 +66,12 @@
<code>AsyncListener.onTimeout()</code> or
<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
+ current request/response. (markt)
+ </fix>
</changelog>
</subsection>
</section>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 02/03: Add debug log messages for the triggering of async
listener events
Posted by ma...@apache.org.
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
commit 85289c3a7feeb10ee47662cb4f7649abee5b9230
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Oct 14 16:14:52 2019 +0100
Add debug log messages for the triggering of async listener events
---
java/org/apache/catalina/core/AsyncContextImpl.java | 12 ++++++++++++
java/org/apache/catalina/core/LocalStrings.properties | 4 ++++
2 files changed, 16 insertions(+)
diff --git a/java/org/apache/catalina/core/AsyncContextImpl.java b/java/org/apache/catalina/core/AsyncContextImpl.java
index 674236c..3f801d5 100644
--- a/java/org/apache/catalina/core/AsyncContextImpl.java
+++ b/java/org/apache/catalina/core/AsyncContextImpl.java
@@ -94,6 +94,9 @@ public class AsyncContextImpl implements AsyncContext, AsyncContextCallback {
@Override
public void fireOnComplete() {
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("asyncContextImpl.fireOnComplete"));
+ }
List<AsyncListenerWrapper> listenersCopy = new ArrayList<>();
listenersCopy.addAll(listeners);
@@ -124,6 +127,9 @@ public class AsyncContextImpl implements AsyncContext, AsyncContextCallback {
Context context = this.context;
if (result.get()) {
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("asyncContextImpl.fireOnTimeout"));
+ }
ClassLoader oldCL = context.bind(false, null);
try {
List<AsyncListenerWrapper> listenersCopy = new ArrayList<>();
@@ -327,6 +333,9 @@ public class AsyncContextImpl implements AsyncContext, AsyncContextCallback {
List<AsyncListenerWrapper> listenersCopy = new ArrayList<>();
listenersCopy.addAll(listeners);
listeners.clear();
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("asyncContextImpl.fireOnStartAsync"));
+ }
for (AsyncListenerWrapper listener : listenersCopy) {
try {
listener.fireOnStartAsync(event);
@@ -401,6 +410,9 @@ public class AsyncContextImpl implements AsyncContext, AsyncContextCallback {
request.getCoyoteRequest().action(ActionCode.ASYNC_ERROR, null);
if (fireOnError) {
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("asyncContextImpl.fireOnError"));
+ }
AsyncEvent errorEvent = new AsyncEvent(event.getAsyncContext(),
event.getSuppliedRequest(), event.getSuppliedResponse(), t);
List<AsyncListenerWrapper> listenersCopy = new ArrayList<>();
diff --git a/java/org/apache/catalina/core/LocalStrings.properties b/java/org/apache/catalina/core/LocalStrings.properties
index 12bd425..fda5ec3 100644
--- a/java/org/apache/catalina/core/LocalStrings.properties
+++ b/java/org/apache/catalina/core/LocalStrings.properties
@@ -94,6 +94,10 @@ aprListener.wrongFIPSMode=Unexpected value of FIPSMode option of AprLifecycleLis
asyncContextImpl.asyncDispachError=Error during asynchronous dispatch
asyncContextImpl.asyncRunnableError=Error during processing of asynchronous Runnable via AsyncContext.start()
asyncContextImpl.dispatchingStarted=Asynchronous dispatch operation has already been called. Additional asynchronous dispatch operation within the same asynchronous cycle is not allowed.
+asyncContextImpl.fireOnComplete=Firing onComplete() event for any AsyncListeners
+asyncContextImpl.fireOnError=Firing onError() event for any AsyncListeners
+asyncContextImpl.fireOnStartAsync=Firing onStartAsync() event for any AsyncListeners
+asyncContextImpl.fireOnTimeout=Firing onTimeout() event for any AsyncListeners
asyncContextImpl.noAsyncDispatcher=The dispatcher returned from the ServletContext does not support asynchronous dispatching
asyncContextImpl.onCompleteError=onComplete() call failed for listener of type [{0}]
asyncContextImpl.onErrorError=onError() call failed for listener of type [{0}]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[tomcat] 01/03: Add test case for bug 63816
Posted by ma...@apache.org.
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
commit 44a50ce0c7cfe40ed0aa4c422570cc2d278eb569
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Wed Oct 9 11:49:19 2019 +0100
Add test case for bug 63816
---
.../apache/catalina/core/TestAsyncContextImpl.java | 149 +++++++++++++++++++++
1 file changed, 149 insertions(+)
diff --git a/test/org/apache/catalina/core/TestAsyncContextImpl.java b/test/org/apache/catalina/core/TestAsyncContextImpl.java
index e02753d..35165b6 100644
--- a/test/org/apache/catalina/core/TestAsyncContextImpl.java
+++ b/test/org/apache/catalina/core/TestAsyncContextImpl.java
@@ -25,8 +25,10 @@ import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicBoolean;
import javax.servlet.AsyncContext;
import javax.servlet.AsyncEvent;
@@ -56,6 +58,8 @@ import org.apache.catalina.Context;
import org.apache.catalina.Wrapper;
import org.apache.catalina.connector.Request;
import org.apache.catalina.connector.Response;
+import org.apache.catalina.connector.TestCoyoteAdapter;
+import org.apache.catalina.startup.SimpleHttpClient;
import org.apache.catalina.startup.Tomcat;
import org.apache.catalina.startup.TomcatBaseTest;
import org.apache.catalina.valves.TesterAccessLogValve;
@@ -2861,4 +2865,149 @@ public class TestAsyncContextImpl extends TomcatBaseTest {
resp.getOutputStream().write("OK".getBytes(StandardCharsets.UTF_8));
}
}
+
+
+ /*
+ * Tests an error on an async thread before the container thread that called
+ * startAsync() has returned to the container.
+ *
+ * Required sequence is:
+ * - enter Servlet's service() method
+ * - startAsync()
+ * - start async thread
+ * - close client connection
+ * - write on async thread -> I/O error
+ * - exit Servlet's service() method
+ *
+ * This test makes extensive use of instance fields in the Servlet that
+ * would normally be considered very poor practice. It is only safe in this
+ * test as the Servlet only processes a single request.
+ */
+ @Test
+ public void testBug63816() throws Exception {
+ CountDownLatch doGetLatch = new CountDownLatch(1);
+ CountDownLatch clientCloseLatch = new CountDownLatch(1);
+ CountDownLatch threadCompleteLatch = new CountDownLatch(1);
+
+ AtomicBoolean ise = new AtomicBoolean(true);
+
+ // Setup Tomcat instance
+ Tomcat tomcat = getTomcatInstance();
+
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+
+ Bug63816Servlet bug63816Servlet = new Bug63816Servlet(doGetLatch, clientCloseLatch, threadCompleteLatch, ise);
+ Wrapper wrapper = Tomcat.addServlet(ctx, "bug63816Servlet", bug63816Servlet);
+ wrapper.setAsyncSupported(true);
+ ctx.addServletMappingDecoded("/*", "bug63816Servlet");
+
+ tomcat.start();
+
+ Bug63816Client client = new Bug63816Client();
+ client.setPort(getPort());
+ client.setRequest(new String[] { "GET / HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: localhost:" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF});
+ client.connect();
+ client.sendRequest();
+
+ // Wait for async to start
+ doGetLatch.await();
+
+ client.disconnect();
+
+ clientCloseLatch.countDown();
+
+ threadCompleteLatch.await();
+
+ Assert.assertFalse(ise.get());
+ }
+
+
+ private static final class Bug63816Client extends SimpleHttpClient {
+
+ @Override
+ public boolean isResponseBodyOK() {
+ return true;
+ }
+ }
+
+
+ private static final class Bug63816Servlet extends HttpServlet {
+
+ private static final long serialVersionUID = 1L;
+
+ private final CountDownLatch doGetLatch;
+ private final CountDownLatch clientCloseLatch;
+ private final CountDownLatch threadCompleteLatch;
+ private final AtomicBoolean ise;
+
+ public Bug63816Servlet(CountDownLatch doGetLatch, CountDownLatch clientCloseLatch,
+ CountDownLatch threadCompleteLatch, AtomicBoolean ise) {
+ this.doGetLatch = doGetLatch;
+ this.clientCloseLatch = clientCloseLatch;
+ this.threadCompleteLatch = threadCompleteLatch;
+ this.ise = ise;
+ }
+
+ @Override
+ protected void doGet(HttpServletRequest req, HttpServletResponse resp)
+ throws ServletException, IOException {
+
+ doGetLatch.countDown();
+
+ AsyncContext ac = req.startAsync();
+ Thread t = new Bug63816Thread(ac, clientCloseLatch, threadCompleteLatch, ise);
+ t.start();
+
+ try {
+ threadCompleteLatch.await();
+ } catch (InterruptedException e) {
+ // Ignore
+ }
+ }
+ }
+
+
+ private static final class Bug63816Thread extends Thread {
+
+ private final AsyncContext ac;
+ private final CountDownLatch clientCloseLatch;
+ private final CountDownLatch threadCompleteLatch;
+ private final AtomicBoolean ise;
+
+ public Bug63816Thread(AsyncContext ac, CountDownLatch clientCloseLatch, CountDownLatch threadCompleteLatch,
+ AtomicBoolean ise) {
+ this.ac = ac;
+ this.clientCloseLatch = clientCloseLatch;
+ this.threadCompleteLatch = threadCompleteLatch;
+ this.ise = ise;
+ }
+
+ @Override
+ public void run() {
+ try {
+ // Wait for client to close connection
+ clientCloseLatch.await();
+
+ try {
+ ServletResponse resp = ac.getResponse();
+ resp.setContentType("text/plain");
+ for (int i = 0; i < 4; i++) {
+ resp.getWriter().write(TestCoyoteAdapter.TEXT_8K);
+ resp.flushBuffer();
+ }
+ } catch (IOException e) {
+ // Ignore
+ }
+ ise.set(false);
+ } catch (InterruptedException e) {
+ // Ignore
+ } finally {
+ threadCompleteLatch.countDown();
+ }
+ }
+ }
+
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org