Author: markt
Date: Tue Nov 13 23:44:15 2012
New Revision: 1409030
URL: http://svn.apache.org/viewvc?rev=1409030&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/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java
tomcat/trunk/java/org/apache/coyote/ActionCode.java
tomcat/trunk/java/org/apache/coyote/AsyncStateMachine.java
tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java
tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java
Modified: tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java?rev=1409030&r1=1409029&r2=1409030&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
(original)
+++ tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java Tue Nov
13 23:44:15 2012
@@ -300,7 +300,7 @@ public class CoyoteAdapter implements Ad
if (status==SocketStatus.TIMEOUT) {
success = true;
if (!asyncConImpl.timeout()) {
- asyncConImpl.setErrorState(null);
+ asyncConImpl.setErrorState(null, false);
}
} else if (status==SocketStatus.ASYNC_READ_ERROR) {
success = true;
@@ -308,7 +308,7 @@ public class CoyoteAdapter implements Ad
req.getAttributes().remove(RequestDispatcher.ERROR_EXCEPTION);
asyncConImpl.notifyReadError(t);
if (t != null) {
- asyncConImpl.setErrorState(t);
+ asyncConImpl.setErrorState(t, true);
}
} else if (status==SocketStatus.ASYNC_WRITE_ERROR) {
success = true;
@@ -316,7 +316,7 @@ public class CoyoteAdapter implements Ad
req.getAttributes().remove(RequestDispatcher.ERROR_EXCEPTION);
asyncConImpl.notifyWriteError(t);
if (t != null) {
- asyncConImpl.setErrorState(t);
+ asyncConImpl.setErrorState(t, true);
}
}
@@ -338,7 +338,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/trunk/java/org/apache/catalina/core/AsyncContextImpl.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java?rev=1409030&r1=1409029&r2=1409030&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java Tue Nov 13
23:44:15 2012
@@ -171,7 +171,7 @@ public class AsyncContextImpl implements
}
}
- public boolean timeout() throws IOException {
+ public boolean timeout() {
AtomicBoolean result = new AtomicBoolean();
request.getCoyoteRequest().action(ActionCode.ASYNC_TIMEOUT, result);
@@ -181,22 +181,19 @@ public class AsyncContextImpl implements
ClassLoader newCL =
request.getContext().getLoader().getClassLoader();
try {
Thread.currentThread().setContextClassLoader(newCL);
- boolean listenerInvoked = false;
List<AsyncListenerWrapper> listenersCopy = new ArrayList<>();
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);
}
@@ -420,37 +417,50 @@ 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<>();
- 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);
+
+ if (fireOnError) {
+ AsyncEvent errorEvent = new AsyncEvent(event.getAsyncContext(),
+ event.getSuppliedRequest(), event.getSuppliedResponse(),
t);
+ List<AsyncListenerWrapper> listenersCopy = new ArrayList<>();
+ 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);
- }
- 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/trunk/java/org/apache/coyote/ActionCode.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/ActionCode.java?rev=1409030&r1=1409029&r2=1409030&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/ActionCode.java (original)
+++ tomcat/trunk/java/org/apache/coyote/ActionCode.java Tue Nov 13 23:44:15 2012
@@ -203,6 +203,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/trunk/java/org/apache/coyote/AsyncStateMachine.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/AsyncStateMachine.java?rev=1409030&r1=1409029&r2=1409030&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/AsyncStateMachine.java (original)
+++ tomcat/trunk/java/org/apache/coyote/AsyncStateMachine.java Tue Nov 13
23:44:15 2012
@@ -52,41 +52,45 @@ import org.apache.tomcat.util.res.String
*
* |----------------->--------------|
* | \|/
- * |
|----------<---------------ERROR-----------------------<----------------------------------|
- * | | complete() /|\ |
|
- * | | | |postProcess()
|
- * | | error()| |
|
- * | | | | |--|timeout()
|
- * | | postProcess() | \|/ | \|/ auto
|
- * | |
|--------------->DISPATCHED<-----------------------COMPLETING<-----|
|
- * | | | /|\ | | /|\
| |
- * | | | |--->-------| | |--|
| |
- * | | ^ | |startAsync() timeout()
| |
- * | | | | |
| |
- * | \|/ | | complete() \|/ postProcess()
| |
- * | MUST_COMPLETE-<- | ----<------STARTING-->--------------------|
^ |
- * | /|\ /|\ | | |
complete() | |
- * | | | | | |
/-----------| |
- * | | | ^ |dispatch() | /
|
- * | | | | | | /
|
- * | | | | \|/ \|/ /
|
- * | | | | MUST_DISPATCH STARTED
|
- * | | | | | /| \ \
|
- * | | | | |postProcess() / | \ \
|
- * ^ | ^ | | dispatch() / | \ \
|
- * | | | | | / | \ \
postProcess() |
- * | | | | | |-------------------/ |auto \
\----<--------------| |
- * | | | | auto \|/ \|/ \|/ \
| |
- * | | | |---<----DISPATCHING<-----------------TIMING_OUT \
| |
- * | | | dispatch() | |
|asyncOperation() ^ |
- * | | | | | \|/
| ^
- * | | |-------<----------------------------------<----| |
READ_WRITE_OP->-----| |
- * | | complete() | |
| |
- * | | | |
| error() |
- * |<- | ----<-------------------<-----------------------------<--| |
|->-----------|
- * | error() |
- * | complete() |
- * |--------------------------------------------------------------------|
+ * |
|----------<---------------ERROR---------------------------<-------------------------------|
+ * | | complete() /|\ | \
|
+ * | | | | \---------------|
|
+ * | | | | |dispatch()
|
+ * | | | |postProcess() \|/
|
+ * | | error()| | |
|
+ * | | | | |--|timeout() |
|
+ * | | postProcess() | \|/ | \|/ | auto
|
+ * | | |--------------->DISPATCHED<---------- |
--------------COMPLETING<-----| |
+ * | | | /|\ | | |
/|\ | |
+ * | | | |--->-------| | | |--|
| |
+ * | | ^ | |startAsync() |
timeout() | |
+ * | | | | | |
| |
+ * | \|/ | | complete() \|/ postProcess() |
| |
+ * | MUST_COMPLETE-<- | ----<------STARTING-->--------- | ------------|
^ |
+ * | /|\ /|\ | | | |
complete() | |
+ * | | | | | | |
/-----------| |
+ * | | | ^ |dispatch() | | /
|
+ * | | | | | | | /
|
+ * | | | | \|/ / \|/ /
|
+ * | | | | MUST_DISPATCH / STARTED
|
+ * | | | | | / /| \ \
|
+ * | | | | |postProcess() / / | \ \
|
+ * ^ | ^ | | / dispatch()/ | \ \
|
+ * | | | | | / / | \ \
postProcess() |
+ * | | | | | |---------- / -----------/ |auto \
\----<-----------| |
+ * | | | | | | / | \
| |
+ * | | | | | | |-----/ | |
| |
+ * | | | | auto \|/ \|/ \|/ \|/ |
| |
+ * | | | |---<------DISPATCHING<-----------------TIMING_OUT |
| |
+ * | | | dispatch() | |
|asyncOperation() ^ |
+ * | | | | | \|/
| ^
+ * | | |-------<----------------------------------<------| |
READ_WRITE_OP->-----| |
+ * | | complete() | |
| |
+ * | | | |
| error() |
+ * |<- | ----<-------------------<-------------------------------<--| |
|->-----------|
+ * | error() |
+ * | complete() |
+ * |---------------------------------------------------------------------|
* </pre>
*/
public class AsyncStateMachine<S> {
@@ -165,6 +169,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) {
@@ -211,13 +218,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/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java?rev=1409030&r1=1409029&r2=1409030&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java Tue Nov
13 23:44:15 2012
@@ -459,6 +459,8 @@ public abstract class AbstractAjpProcess
((AtomicBoolean) param).set(asyncStateMachine.isAsyncOperation());
} 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/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java?rev=1409030&r1=1409029&r2=1409030&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
(original)
+++ tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java Tue
Nov 13 23:44:15 2012
@@ -800,6 +800,8 @@ public abstract class AbstractHttp11Proc
((AtomicBoolean) param).set(asyncStateMachine.isAsyncOperation());
} 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/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java?rev=1409030&r1=1409029&r2=1409030&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java Tue Nov 13
23:44:15 2012
@@ -375,6 +375,8 @@ public class SpdyProcessor extends Abstr
((AtomicBoolean) param).set(asyncStateMachine.isAsyncOperation());
} 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 {
// TODO:
// actionInternal(actionCode, param);
Modified: tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java?rev=1409030&r1=1409029&r2=1409030&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java
(original)
+++ tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java Tue
Nov 13 23:44:15 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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
