Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
Received-SPF: neutral (nike.apache.org: local policy)
Message-ID: <4C46A48A.9090109@apache.org>
Date: Wed, 21 Jul 2010 08:40:58 +0100
From: Mark Thomas <markt@apache.org>
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-GB;
 rv:1.9.2.4) Gecko/20100608 Thunderbird/3.1
MIME-Version: 1.0
To: Tomcat Developers List <dev@tomcat.apache.org>
Subject: Re: svn commit: r965150 - in /tomcat/trunk:
 java/org/apache/catalina/connector/
 java/org/apache/tomcat/util/net/		 webapps/docs/ webapps/docs/config/
References: <20100717235724.4785E2388980@eris.apache.org>
 <4C424493.4020705@apache.org> <4C438548.4030304@kippdata.de>
 <4C448885.3000209@apache.org> <4C4543A8.8050206@gmail.com>
 <4C457711.9050609@kippdata.de>
In-Reply-To: <4C457711.9050609@kippdata.de>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit

On 20/07/2010 11:14, Rainer Jung wrote:
> On 20.07.2010 08:35, jean-frederic clere wrote:
>> On 07/19/2010 07:16 PM, Mark Thomas wrote:
>>> On 18/07/2010 23:50, Rainer Jung wrote:
>>>> On 18.07.2010 02:02, Mark Thomas wrote:
>>>>> On 18/07/2010 00:57, markt@apache.org wrote:
>>>>>> Author: markt
>>>>>> Date: Sat Jul 17 23:57:23 2010
>>>>>> New Revision: 965150
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=965150&view=rev
>>>>>> Log:
>>>>>> Restore pero's timeout fix for the BIO connector. Add configuration
>>>>>> of the timeout.
>>>>>
>>>>> Servlet TCK (with BIO) and unit tests pass as of this commit.
>>>>>
>>>>> It is looking like timeouts are going to be required to fix
>>>>> https://issues.apache.org/bugzilla/show_bug.cgi?id=49567. The complete
>>>>> fix is going to require some refactoring and I'm not quite there. It
>>>>> does look like most of the fix for
>>>>> https://issues.apache.org/bugzilla/show_bug.cgi?id=49528 is going
>>>>> to end
>>>>> up being reverted.
>>>>>
>>>>> Also, need to check the NIO and APR timeout async requests.
>>>>
>>>> I'm not totally sure, but after JFC's heads up that the TCK fails
>>>
>>> What heads up where?
>>
>> That was more than 2 weeks ago. I need to retest with the actual code.
>>
>>> This stuff should be on the dev list. A simple "The
> 
> As JFC said: it was an old message (on the dev list):
> 
> http://apache.markmail.org/message/7t7l2oijwtsn6gp6

That wasn't clear from your original message. Further, I don't think
those failures are relevant. Peter made a bunch of changes back then.
This is one change to the BIO connector and I have tested that the
Servlet TCK still passes with this change. I'm pretty sure it was his
other changes that caused the TCK to fail.

>>> Servlet 3.0 TCK fails with the XXX connector." is fine on the dev list.
>>>
>>> Which test with which connector?
>>
>> It was with APR.

Peter's previous changes caused failures with BIO as well.


>>
>>> I did only check with BIO.
>>>
>>>> I had
>>>> a short look at the async timeout runnable. My impression was, that the
>>>> default value for the timeout is "-1" and this would let the timeout
>>>> condition in the runnable always trigger.
>>>
>>> > From the top of page 14 of the servlet 3.0 spec:
>>>
>>> AsyncContext.setTimeout(long)... A value of 0 or less indicates that the
>>> asynchronous operation will never time out. ...
> 
>>> I couldn't find an explicit default.
>>
>> Can't find it too.. Why not using 0 (for ever) or 1 minutes (to prevent
>> bad applications).

A default timeout of forever causes problems when Runnables are used.
The original implementation didn't take into account that the Runnable
could be used to call AsyncContext.complete(). This was bug 49528. My
first attempt to fix this handled this one special case - it did not
handle the general case (bug 49528). I am currently working on a fix for
the general case. The general case fix needs timeouts since that is the
only way to detect errors in the applications where complete() is not
called.

> I was slightly confused by two possibly different notions of timeout,
> the one in AsyncContext and the one in the AsyncTimeout runnable. I was
> looking at the AsyncTimeout runnable in JIoEndpoint which contains:
> 
> if ((now-access)>socket.getTimeout()) {
>     processSocket(socket,SocketStatus.TIMEOUT);
> }
> 
> So I thought since now>=access if the timeout is <=0 it will run
> processSocket(socket,SocketStatus.TIMEOUT). This seems to end up in
> CoyoteAdapter.asyncDispatch(*, *, SocketStatus.TIMEOUT), which calls
> asyncContextImpl.setTimeoutState(). That in turn sets an internal state
> that shortcuts doInternalDispatch() to fire a timeout event and return
> an error.
> 
> The async context timeout seems to be set from the connector
> asyncTimeout attribute (default 10000) and never gets used directly.
> 
> But it is all tied together via the listener which is called when the
> async context timeout is set. The listener is implemented in the
> processor, which then sets the new timeout as a timeout to the socket,
> so the two timeouts are actually the same at the end.
> 
> So: the default we use is 10000, but if the timeout were <=0 then I
> think the condition in the AsyncTimeout runnable would fail.

Agreed. That matches with what I was observing.

I have no idea if a default timeout of 10s is appropriate for Async
requests or not. It seems like a good place to start but I'd be fine
with 60s too. The advantage of a shorter timeout is that when tests
fail, they fail relatively quickly.

Mark


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