2012/11/13 Mark Thomas <ma...@apache.org>:
> On 13/11/2012 00:57, Konstantin Kolinko wrote:
>> 2012/11/12 <ma...@apache.org>:
>>> Author: markt
>>> Date: Sun Nov 11 23:35:41 2012
>>> New Revision: 1408152
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1408152&view=rev
>>> Log:
>>> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54123
>>> There are two things going on here:
>>> 1. The reported bug. If there is a async timeout and no async listeners,
>>> trigger a 500 response.
>>> 2. Implement "error dispatch". This is used a couple of times in the spec
>>> without any definition. The implication from the part of the spec quoted in
>>> the bug report is:
>>> - The standard error page mechanism should be used to identify the page
>>> - An async request that has been started should be left in async mode
>>> when forwarding to the error page
>>> - The error page may call complete() or dispatch()
>>> This commit hooks into the StandardHostValve to access the error page
>>> mechanism. I could have copied and pasted but I preferred the dependency on
>>> StandardHostValve
>>> Because the error page may do a dispatch(), need to ensure that when
>>> running the dispatch(), the error page mechanism is not triggered a second
>>> time.
>>> Depending on what emerges running the full unit tests and the TCK, I mat
>>> still decide to copy the error page code to AsyncContextImpl
>>>
>>> Modified:
>>> tomcat/tc7.0.x/trunk/ (props changed)
>>> tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/AsyncContextImpl.java
>>>
>>> tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardHostValve.java
>>> tomcat/tc7.0.x/trunk/java/org/apache/coyote/AsyncStateMachine.java
>>>
>>> tomcat/tc7.0.x/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java
>>> tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
>>>
>>> Propchange: tomcat/tc7.0.x/trunk/
>>> ------------------------------------------------------------------------------
>>> Merged /tomcat/trunk:r1408148
>>>
>>> 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=1408152&r1=1408151&r2=1408152&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
>>> Sun Nov 11 23:35:41 2012
>>> @@ -41,6 +41,8 @@ import javax.servlet.http.HttpServletRes
>>> import org.apache.catalina.AsyncDispatcher;
>>> import org.apache.catalina.Context;
>>> import org.apache.catalina.Globals;
>>> +import org.apache.catalina.Host;
>>> +import org.apache.catalina.Valve;
>>> import org.apache.catalina.connector.Request;
>>> import org.apache.coyote.ActionCode;
>>> import org.apache.coyote.AsyncContextCallback;
>>> @@ -128,8 +130,8 @@ public class AsyncContextImpl implements
>>> ActionCode.ASYNC_IS_TIMINGOUT, result);
>>> return !result.get();
>>> } else {
>>> - // No listeners, container calls complete
>>> - complete();
>>> + // No listeners, trigger error handling
>>> + return false;
>>> }
>>>
>>> } finally {
>>> @@ -372,6 +374,23 @@ public class AsyncContextImpl implements
>>> listener.getClass().getName() + "]", ioe);
>>> }
>>> }
>>
>>
>> The spec in 2.3.3.3 says "If none of the listeners called
>> AsyncContext.complete or any of the AsyncContext.dispatch methods,
>> then perform an error dispatch...."
>>
>> As far as I see, the code below is executed unconditionally, but the
>> spec says that it has to be executed only if the listeners did not do
>> the calls....
>
> The code below is part of AsyncContextImpl.setErrorState() and that is
> only called from timeout() if there is no configured listener that calls
> complete() or dispatch().
>
I am not convinced.
I do not see other places where AsyncListenerWrapper.fireOnError(..)
were called. So it is the only place that calls onError() on those
listeners.
Where is it checking that "none of the listeners called
AsyncContext.complete or any of the AsyncContext.dispatch methods" in
their onError() methods? Why does it go on to call
StandardHostValve.throwable(,,) in case the error has already been
handled by the listener?
SRV.2.3.3.3
Points "i" and "ii" on page 38 of 230. (numbered page "16" at the
bottom of the page).
servlet-3_0-mrel-spec.pdf
>
>>
>>> +
>>> + // 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();
>>> + }
>>> }
>>>
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
