tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: Proposal: Some todo's at AsyncContext detected...
Date Wed, 30 Jun 2010 19:44:10 GMT
On 30/06/2010 21:23, Peter Roßbach wrote:
> Hi Marc,
>
>
> Am 30.06.2010 um 20:03 schrieb Mark Thomas:
>
>> On 30/06/2010 17:01, Marc Guillemot wrote:
>>> Hi,
>>>
>>> What you write means that the freshly release Tomcat 7 doesn't fully
>>> implement Servlet API 3.0, isn't it?
>>>
>>> Interesting to see that nobody reacts :-(
>>
>> If you were following the dev list you would have seen that
>> Jean-Frederic has already vetoed the change Peter made on the basis
>> that it breaks the TCK. That suggests that Peter's analysis of the
>> current situation is not 100% correct.
>>
> Which of my findings aren't correct?

I don't know - I haven't run the TCK with your patch applied. Just the 
fact the the findings led to the patch which in turn caused TCKs 
failures suggests at least something isn't quite right. It could also be 
the analysis is OK but the patch has issues or that there is a problem 
with the TCK.

I wouldn't be at all surprised to find part of the spec not covered by 
the TCK. I would be surprised to find a bug in the TCK. My own 
experiences with potential TCK bugs is that the TCK has always been correct.

Regardless, until the TCK is proved wrong Jean-Frederic's veto is valid 
and your patch needs to be reverted.

Mark

>
> Peter
>
>> My recollection is that the last thing that Filip implemented for
>> Servlet 3.0 was the necessary timeout features. I'd be surprised if
>> Filip, as a member of the Servlet Expert Group that wrote the spec,
>> missed something fundamental. I wouldn't be surprised if there were
>> some edge cases missed. I do know that Filip tended to leave a lot of
>> the Servlet 3.0 TODOs in the code, even when he had implemented the
>> appropriate feature. Those certainly need cleaning up.
>>
>
>
>> Looking at Peter's test case - along with the other issues folks have
>> raised in Bugzilla for the 7.0.0 release - is on my todo list but like
>> the other committers I have a day job. Whilst many of the committers
>> are fortunate enough (me included) to be able to allocate some work
>> time to Tomcat development, it doesn't follow one of the other
>> committers will immediately have the time necessary to look at Peter's
>> e-mail and comment in detail. I can't speak for the others, but my
>> free time is currently occupied with another Tomcat 7 issue - namely
>> http://tomcat.markmail.org/thread/25g353eqvqfqniiz
>>
>> Like any area of Tomcat, help is always appreciated. If you are
>> interested in this issue and would like to help out I would encourage
>> you to look at Peter's test case, compare the expected behaviour to
>> what the spec defines and offer your view on whether there is a bug
>> that needs fixing or not. If there is an issue, patches would be even
>> better.
>>
>> Mark
>>
>>
>>>
>>> Cheers,
>>> Marc.
>>>
>>> Peter Roßbach wrote:
>>>> Hi!
>>>>
>>>> I detected some Todo's at AsyncContext
>>>>
>>>> - AsyncContext.createListener
>>>> We don't make a resource injection or make it configurable!
>>>>
>>>> Servlet 3.0 API comment:
>>>> "This method supports resource injection if the given clazz represents
>>>> a Managed Bean. See the Java EE platform and JSR 299 specifications
>>>> for additional details about Managed Beans and resource injection."
>>>>
>>>> - I miss the is[Started,Completed] methods at AsyncContext interface
>>>> The only way to detect a completed state at AsyncContext inside an
>>>> AsyncListener is:
>>>> if (ac.getResponse() != null)
>>>> AsyncListener got an event, with empty state... -- Very strange!
>>>>
>>>> Can't we clear all AsyncListener after send complete and before we
>>>> refresh the AsyncContext state?
>>>> How can a user handle a completed situation?
>>>> see o.a.c.core.TestAsyncListener
>>>>
>>>> - AsyncContext.start() dosen't really start a thread!
>>>> We create wrapper of a Runnable, but we don't start a thread...
>>>> Error handling seam a little bit strange.
>>>> We only throw a RuntimeException(ex) and log an error!
>>>>
>>>> Servlet API comment:
>>>> "Causes the container to dispatch a thread, possibly from a managed
>>>> thread pool, to run the specified Runnable. The container may
>>>> propagate appropriate contextual information to the Runnable."
>>>>
>>>> How can we implement that?
>>>> Start everytime a new thread without pooling?
>>>> Use the connector executor pool from request?
>>>> Then we must extend the ProtocolHandler interface with getExecutor()
>>>> signature.
>>>> Use a separate Executor pool per Engine/Host or Context
>>>> How do we have to implemented the error handling?
>>>>
>>>> - JioEndpoint has a timeout detection thread, but we don't start it!
>>>> I easy fix it with revision 958362,
>>>> but the polling strategy is slow and ineffectiv.
>>>> I didn't make a test at NIO/APR-HTTP or AJP-Connectors.
>>>>
>>>> - After the timeout event is emitted, the completed method is
>>>> automatically called!
>>>> Servlet API 3.0 Spec says:
>>>> ====
>>>> ■In the event that an asynchronous operation times out, the container
>>>> must run
>>>> through the following steps:
>>>> ■ Invoke the AsyncListener.onTimeout method on all the AsyncListener
>>>> instances registered with the ServletRequest on which the asynchronous
>>>> operation was initiated.
>>>> ■ If none of the listeners called AsyncContext.complete() or any of the
>>>> AsyncContext.dispatch methods, perform an error dispatch with a status
>>>> code equal to HttpServletResponse.SC_INTERNAL_SERVER_ERROR.
>>>> ■ If no matching error page was found, or the error page did not call
>>>> AsyncContext.complete() or any of the AsyncContext.dispatch
>>>> methods, the container MUST call AsyncContext.complete().
>>>> ===
>>>> I miss the implementation that we call the error page!
>>>>
>>>> - Currently a AsyncListener can't reinit a async cycle!
>>>> see o.a.c.connector.Request.startAsync
>>>> ... L1555ff
>>>> if (asyncContext==null) {
>>>> asyncContext = new AsyncContextImpl(this);
>>>> } else if (asyncContext.isStarted()) {
>>>> throw new IllegalStateException("Already started.");
>>>> }
>>>> ...
>>>>
>>>> What does this spec definition really mean?
>>>> === Ch 2 Page 18
>>>> ■public void onStartAsync(AsyncEvent event) - Is used to notify the
>>>> listener that a new asynchronous cycle is being initiated via a call
>>>> to one of the
>>>> ServletRequest.startAsync methods. The AsyncContext corresponding
>>>> to the asynchronous operation that is being reinitialized may be
>>>> obtained by
>>>> calling AsyncEvent.getAsyncContext on the given event.
>>>> ====
>>>>
>>>> - AsyncListener doesn't receive a onStartAsync event.
>>>> We don't implement it.
>>>>
>>>> At AsyncListenerWrapper fireOnStartAsync is missing
>>>> As AsyncContext.startAsync method doesn't emit an event
>>>>
>>>> - ContextClassLoader at AsyncContext.dispatch is not set!
>>>> I can't see that we correcly set the application ContextClassLoader
>>>> before
>>>> we dispatch the Request. What must we do at a CrossContext dispatch?
>>>>
>>>> - After first complete async cycle, an AsyncListner receive
>>>> onStartAsync as code start next async cycle?
>>>>
>>>> Why doesn't the Servlet 3.0 TCK check basic AsyncContext
>>>> functionalities?
>>>> It seams that we better start a open test suite implementation at the
>>>> new Servlet 3.0 API.
>>>>
>>>> Regards,
>>>> Peter
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>


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


Mime
View raw message