tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Kolinko <knst.koli...@gmail.com>
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 14:06:38 GMT
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

>
>>
>>> +
>>> +        // 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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message