tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: svn commit: r1408152 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/core/ java/org/apache/coyote/ test/org/apache/catalina/core/ webapps/docs/
Date Tue, 13 Nov 2012 15:52:17 GMT
On 13/11/2012 14:06, Konstantin Kolinko wrote:
> 2012/11/13 Mark Thomas <markt@apache.org>:
>> On 13/11/2012 00:57, Konstantin Kolinko wrote:
>>> 2012/11/12  <markt@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

OK. I see where you are coming from. This needs some more work.

Mark


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message