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 2012/11/14 00:55:28 UTC
svn commit: r1409036 - in /tomcat/tc7.0.x/trunk: ./
java/org/apache/catalina/connector/ java/org/apache/catalina/core/
java/org/apache/coyote/ java/org/apache/coyote/ajp/
java/org/apache/coyote/http11/ test/org/apache/catalina/core/
Author: markt
Date: Tue Nov 13 23:55:26 2012
New Revision: 1409036
URL: http://svn.apache.org/viewvc?rev=1409036&view=rev
Log:
More updates to the async error handling triggered by kkolinko's review
- simplify check to see if listeners changed async state during onTimeout
- add option to control if onError fires during error handling (it doesn't if called from the timeout code since that has its own event)
- add check to see if listeners changed state during onError
- add check to see if state changed during "error dispatch"
- remove AsyncState.ERROR as a valid start state for asyncPostProcess()
- aligned the unit tests (one change) with the new behaviour
Not tested with the TCK.
Modified:
tomcat/tc7.0.x/trunk/ (props changed)
tomcat/tc7.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/AsyncContextImpl.java
tomcat/tc7.0.x/trunk/java/org/apache/coyote/ActionCode.java
tomcat/tc7.0.x/trunk/java/org/apache/coyote/AsyncStateMachine.java
tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
tomcat/tc7.0.x/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
tomcat/tc7.0.x/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java
Propchange: tomcat/tc7.0.x/trunk/
------------------------------------------------------------------------------
Merged /tomcat/trunk:r1409030
Modified: tomcat/tc7.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java?rev=1409036&r1=1409035&r2=1409036&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java Tue Nov 13 23:55:26 2012
@@ -290,7 +290,7 @@ public class CoyoteAdapter implements Ad
if (status==SocketStatus.TIMEOUT) {
success = true;
if (!asyncConImpl.timeout()) {
- asyncConImpl.setErrorState(null);
+ asyncConImpl.setErrorState(null, false);
}
}
if (request.isAsyncDispatching()) {
@@ -299,7 +299,7 @@ public class CoyoteAdapter implements Ad
Throwable t = (Throwable) request.getAttribute(
RequestDispatcher.ERROR_EXCEPTION);
if (t != null) {
- asyncConImpl.setErrorState(t);
+ asyncConImpl.setErrorState(t, true);
}
}
Modified: tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/AsyncContextImpl.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/AsyncContextImpl.java?rev=1409036&r1=1409035&r2=1409036&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/AsyncContextImpl.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/AsyncContextImpl.java Tue Nov 13 23:55:26 2012
@@ -116,7 +116,7 @@ public class AsyncContextImpl implements
}
}
- public boolean timeout() throws IOException {
+ public boolean timeout() {
AtomicBoolean result = new AtomicBoolean();
request.getCoyoteRequest().action(ActionCode.ASYNC_TIMEOUT, result);
@@ -126,23 +126,20 @@ public class AsyncContextImpl implements
ClassLoader newCL = request.getContext().getLoader().getClassLoader();
try {
Thread.currentThread().setContextClassLoader(newCL);
- boolean listenerInvoked = false;
List<AsyncListenerWrapper> listenersCopy =
new ArrayList<AsyncListenerWrapper>();
listenersCopy.addAll(listeners);
for (AsyncListenerWrapper listener : listenersCopy) {
- listener.fireOnTimeout(event);
- listenerInvoked = true;
+ try {
+ listener.fireOnTimeout(event);
+ } catch (IOException ioe) {
+ log.warn("onTimeout() failed for listener of type [" +
+ listener.getClass().getName() + "]", ioe);
+ }
}
- if (listenerInvoked) {
- request.getCoyoteRequest().action(
- ActionCode.ASYNC_IS_TIMINGOUT, result);
- return !result.get();
- } else {
- // No listeners, trigger error handling
- return false;
- }
-
+ request.getCoyoteRequest().action(
+ ActionCode.ASYNC_IS_TIMINGOUT, result);
+ return !result.get();
} finally {
Thread.currentThread().setContextClassLoader(oldCL);
}
@@ -246,7 +243,6 @@ public class AsyncContextImpl implements
listeners.add(wrapper);
}
- @SuppressWarnings("unchecked")
@Override
public <T extends AsyncListener> T createListener(Class<T> clazz)
throws ServletException {
@@ -368,38 +364,51 @@ public class AsyncContextImpl implements
}
- public void setErrorState(Throwable t) {
+ public void setErrorState(Throwable t, boolean fireOnError) {
if (t!=null) request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, t);
request.getCoyoteRequest().action(ActionCode.ASYNC_ERROR, null);
- AsyncEvent errorEvent = new AsyncEvent(event.getAsyncContext(),
- event.getSuppliedRequest(), event.getSuppliedResponse(), t);
- List<AsyncListenerWrapper> listenersCopy =
- new ArrayList<AsyncListenerWrapper>();
- listenersCopy.addAll(listeners);
- for (AsyncListenerWrapper listener : listenersCopy) {
- try {
- listener.fireOnError(errorEvent);
- } catch (IOException ioe) {
- log.warn("onError() failed for listener of type [" +
- listener.getClass().getName() + "]", ioe);
- }
- }
- // SRV.2.3.3.3 (search for "error dispatch")
- if (servletResponse instanceof HttpServletResponse) {
- ((HttpServletResponse) servletResponse).setStatus(
- HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ if (fireOnError) {
+ AsyncEvent errorEvent = new AsyncEvent(event.getAsyncContext(),
+ event.getSuppliedRequest(), event.getSuppliedResponse(), t);
+ List<AsyncListenerWrapper> listenersCopy =
+ new ArrayList<AsyncListenerWrapper>();
+ listenersCopy.addAll(listeners);
+ for (AsyncListenerWrapper listener : listenersCopy) {
+ try {
+ listener.fireOnError(errorEvent);
+ } catch (IOException ioe) {
+ log.warn("onError() failed for listener of type [" +
+ listener.getClass().getName() + "]", ioe);
+ }
+ }
}
- Host host = (Host) context.getParent();
- Valve stdHostValve = host.getPipeline().getBasic();
- if (stdHostValve instanceof StandardHostValve) {
- ((StandardHostValve) stdHostValve).throwable(request,
- request.getResponse(), t);
- }
- if (isStarted() && !request.isAsyncDispatching()) {
- complete();
+ AtomicBoolean result = new AtomicBoolean();
+ request.getCoyoteRequest().action(ActionCode.ASYNC_IS_ERROR, result);
+ if (result.get()) {
+ // No listener called dispatch() or complete(). This is an error.
+ // SRV.2.3.3.3 (search for "error dispatch")
+ if (servletResponse instanceof HttpServletResponse) {
+ ((HttpServletResponse) servletResponse).setStatus(
+ HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ }
+
+ Host host = (Host) context.getParent();
+ Valve stdHostValve = host.getPipeline().getBasic();
+ if (stdHostValve instanceof StandardHostValve) {
+ ((StandardHostValve) stdHostValve).throwable(request,
+ request.getResponse(), t);
+ }
+
+ request.getCoyoteRequest().action(
+ ActionCode.ASYNC_IS_ERROR, result);
+ if (result.get()) {
+ // Still in the error state. The error page did not call
+ // complete() or dispatch(). Complete the async processing.
+ complete();
+ }
}
}
Modified: tomcat/tc7.0.x/trunk/java/org/apache/coyote/ActionCode.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ActionCode.java?rev=1409036&r1=1409035&r2=1409036&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/coyote/ActionCode.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/ActionCode.java Tue Nov 13 23:55:26 2012
@@ -192,6 +192,11 @@ public enum ActionCode {
ASYNC_IS_TIMINGOUT,
/**
+ * Callback to determine if async is in error
+ */
+ ASYNC_IS_ERROR,
+
+ /**
* Callback to trigger the HTTP upgrade process.
*/
UPGRADE
Modified: tomcat/tc7.0.x/trunk/java/org/apache/coyote/AsyncStateMachine.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/AsyncStateMachine.java?rev=1409036&r1=1409035&r2=1409036&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/coyote/AsyncStateMachine.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/AsyncStateMachine.java Tue Nov 13 23:55:26 2012
@@ -49,40 +49,46 @@ import org.apache.tomcat.util.res.String
* DISPATCHING - The dispatch is being processed.
* ERROR - Something went wrong.
*
- * |----------------->--------------|
- * | \|/
- * | |----------<---------------ERROR
- * | | complete() /|\ |postProcess()
- * | | error()| |
- * | | | | |--|timeout()
- * | | postProcess() | \|/ | \|/ auto
- * | | |--------------->DISPATCHED<------------------COMPLETING<----|
- * | | | /|\ | | /|\ |
- * | | | |--->-------| | |--| |
- * | | ^ | |startAsync() timeout() |
- * | | | | | |
- * | \|/ | | complete() \|/ postProcess() |
- * | MUST_COMPLETE-<- | ----<------STARTING-->----------------| ^
- * | /|\ | | | |
- * | | | | | |
- * | | ^ |dispatch() | |
- * | | | | | |
- * | | | \|/ \|/ complete() |
- * | | | MUST_DISPATCH STARTED---->-------|
- * | | | | | |
- * | | | |postProcess() | |
- * ^ ^ | | dispatch() | |auto
- * | | | | |--------------------| |
- * | | | auto \|/ \|/ \|/
- * | | |---<----DISPATCHING<-----------------TIMING_OUT
- * | | dispatch() | |
- * | | | |
- * | |-------<-------------------------------------<----| |
- * | complete() |
- * | |
- * |----<------------------------<-----------------------------<--|
- * error()
- * </pre>
+ * |----------------->--------------|
+ * | \|/
+ * | |----------<---------------ERROR
+ * | | complete() /|\ | \
+ * | | | | \---------------|
+ * | | | | |dispatch()
+ * | | | |postProcess() \|/
+ * | | error()| | |
+ * | | | | |--|timeout() |
+ * | | postProcess() | \|/ | \|/ | auto
+ * | | |--------------->DISPATCHED<---------- | --------------COMPLETING<-----|
+ * | | | /|\ | | | /|\ |
+ * | | | |--->-------| | | |--| |
+ * | | ^ | |startAsync() | timeout() |
+ * | | | | | | |
+ * | \|/ | | complete() \|/ postProcess() | |
+ * | MUST_COMPLETE-<- | ----<------STARTING-->--------- | ------------| ^
+ * | /|\ | | | | complete() |
+ * | | | | | | /-----------|
+ * | | ^ |dispatch() | | /
+ * | | | | | | /
+ * | | | \|/ / \|/ /
+ * | | | MUST_DISPATCH / STARTED
+ * | | | | / /|
+ * | | | |postProcess() / / |
+ * ^ ^ | | / dispatch()/ |
+ * | | | | / / |
+ * | | | | |---------- / -----------/ |auto
+ * | | | | | / |
+ * | | | | | |-----/ |
+ * | | | auto \|/ \|/ \|/ \|/
+ * | | |---<------DISPATCHING<-----------------TIMING_OUT
+ * | | dispatch() | |
+ * | | | |
+ * | |-------<----------------------------------<------| |
+ * | complete() |
+ * | |
+ * |<--------<-------------------<-------------------------------<--|
+ * error()
+ * </pre>
*/
public class AsyncStateMachine<S> {
@@ -155,6 +161,9 @@ public class AsyncStateMachine<S> {
return state == AsyncState.TIMING_OUT;
}
+ public boolean isAsyncError() {
+ return state == AsyncState.ERROR;
+ }
public synchronized void asyncStart(AsyncContextCallback asyncCtxt) {
if (state == AsyncState.DISPATCHED) {
@@ -191,13 +200,6 @@ public class AsyncStateMachine<S> {
} else if (state == AsyncState.DISPATCHING) {
state = AsyncState.DISPATCHED;
return SocketState.ASYNC_END;
- } else if (state == AsyncState.ERROR) {
- asyncCtxt.fireOnComplete();
- state = AsyncState.DISPATCHED;
- return SocketState.ASYNC_END;
- //} else if (state == AsyncState.DISPATCHED) {
- // // No state change
- // return SocketState.OPEN;
} else {
throw new IllegalStateException(
sm.getString("asyncStateMachine.invalidAsyncState",
Modified: tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java?rev=1409036&r1=1409035&r2=1409036&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java Tue Nov 13 23:55:26 2012
@@ -457,6 +457,8 @@ public abstract class AbstractAjpProcess
((AtomicBoolean) param).set(asyncStateMachine.isAsync());
} else if (actionCode == ActionCode.ASYNC_IS_TIMINGOUT) {
((AtomicBoolean) param).set(asyncStateMachine.isAsyncTimingOut());
+ } else if (actionCode == ActionCode.ASYNC_IS_ERROR) {
+ ((AtomicBoolean) param).set(asyncStateMachine.isAsyncError());
} else if (actionCode == ActionCode.UPGRADE) {
// HTTP connections only. Unsupported for AJP.
// NOOP
Modified: tomcat/tc7.0.x/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java?rev=1409036&r1=1409035&r2=1409036&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java Tue Nov 13 23:55:26 2012
@@ -839,6 +839,8 @@ public abstract class AbstractHttp11Proc
((AtomicBoolean) param).set(asyncStateMachine.isAsync());
} else if (actionCode == ActionCode.ASYNC_IS_TIMINGOUT) {
((AtomicBoolean) param).set(asyncStateMachine.isAsyncTimingOut());
+ } else if (actionCode == ActionCode.ASYNC_IS_ERROR) {
+ ((AtomicBoolean) param).set(asyncStateMachine.isAsyncError());
} else if (actionCode == ActionCode.UPGRADE) {
upgradeInbound = (UpgradeInbound) param;
// Stop further HTTP output
Modified: tomcat/tc7.0.x/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java?rev=1409036&r1=1409035&r2=1409036&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java (original)
+++ tomcat/tc7.0.x/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java Tue Nov 13 23:55:26 2012
@@ -472,9 +472,7 @@ public class TestAsyncContextImpl extend
}
} else {
expected.append("onTimeout-");
- if (dispatchUrl == null) {
- expected.append("onError-");
- } else {
+ if (dispatchUrl != null) {
expected.append("NonAsyncServletGet-");
}
expected.append("onComplete-");
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org