You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Konstantin Kolinko <kn...@gmail.com> on 2012/12/02 23:00:05 UTC
Re: svn commit: r1415184 - in /tomcat/tc7.0.x/trunk: ./
test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java webapps/docs/changelog.xml
2012/11/29 <ma...@apache.org>:
> Author: markt
> Date: Thu Nov 29 14:35:02 2012
> New Revision: 1415184
>
> URL: http://svn.apache.org/viewvc?rev=1415184&view=rev
> Log:
> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
> Improve unit tests.
> Patch by Brian Burch.
>
> Modified:
> tomcat/tc7.0.x/trunk/ (props changed)
> tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java
> tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
>
> Propchange: tomcat/tc7.0.x/trunk/
> ------------------------------------------------------------------------------
> Merged /tomcat/trunk:r1415177-1415179
>
> + private static final int SHORT_TIMEOUT_MINS = 1;
> + private static final int LONG_TIMEOUT_MINS = 2;
> + private static final int MANAGER_SCAN_DELAY_SECS = 60;
> + private static final int EXTRA_DELAY_SECS = 5;
> + private static final long TIMEOUT_DELAY_MSECS =
> + (((SHORT_TIMEOUT_MINS * 60)
> + + MANAGER_SCAN_DELAY_SECS + EXTRA_DELAY_SECS) * 1000);
...
> + // allow the session to time out and lose authentication
> + Thread.sleep(TIMEOUT_DELAY_MSECS);
According to Buildbot logs, testBasicLoginSessionTimeout() runs for 2
minutes (125 seconds), mainly due to a sleep() call.
I wish there were a way to speed up this test.
My thoughts
a) Maybe use reflection to reduce StandardSession.lastAccessedTime and
thisAccessedTime by some fixed amount (60000) instead of waiting for
that time to pass.
b) Maybe call ManagerBase.setProcessExpiresFrequency(1) instead of the
default value (6) and reduce MANAGER_SCAN_DELAY_SECS from 60 to 10.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1415184 - in /tomcat/tc7.0.x/trunk: ./
test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java webapps/docs/changelog.xml
Posted by Konstantin Kolinko <kn...@gmail.com>.
2012/12/3 Brian Burch <br...@pingtoo.com>:
> On 03/12/12 16:22, Brian Burch wrote:
>>
>> On 03/12/12 11:44, Brian Burch wrote:
>>>
>>> On 02/12/12 22:00, Konstantin Kolinko wrote:
>>
>> <snip/>
>>>>
>>>> According to Buildbot logs, testBasicLoginSessionTimeout() runs for 2
>>>> minutes (125 seconds), mainly due to a sleep() call.
>>>>
>>>> I wish there were a way to speed up this test.
>>>>
>>>> My thoughts
>>>> a) Maybe use reflection to reduce StandardSession.lastAccessedTime and
>>>> thisAccessedTime by some fixed amount (60000) instead of waiting for
>>>> that time to pass.
>>>
>>>
>>> That would speed up the test... but it sounds like adding a time machine
>>> to the test class! My feeling is this would add inappropriate logical
>>> complexity to a test that has always created and destroyed a tomcat
>>> instance for each test case (there are now 15).
>>>
>>> However, I intend to replicate most of the improvements from this test
>>> class into the other authenticator tests, so I am already apprehensive
>>> about adding too many 60+ second delays to the entire suite.
>>>
>>> Mark and I briefly discussed adding a new protected method for use by
>>> these timeout tests:
>>>
>>> org.apache.catalina.session.StandardSession.setSessionTimeoutSecs(int
>>> secs)
>>
>>
>> Silly me! Sorry if that left you wondering what I meant. That particular
>> method already exists, is public, and is called setSessionTimeout.
>>
>> What I meant to suggest was this:
>>
>> org.apache.catalina.core.StandardContext.setSessionTimeoutSecs(int
>> timeout)
>>
>> because it would be much simpler to modify the Context in the test
>> setUp, rather than each individual Session.
>>
>>> What do you think?
>
>
> The change seemed almost trivial to me, but I couldn't work out how to
> implement it without changing the public api, i.e.
>
> I have only found one place where it is used:
>
> org.apache.catalina.session.ManagerBase.setContext(Context context)
>
> has this logic:
>
> // Register with the new Context (if any)
> if (this.context != null) {
> setMaxInactiveInterval(this.context.getSessionTimeout() * 60);
> this.context.addPropertyChangeListener(this);
> }
>
> I wanted to move the "60" back into StandardContext, but that doesn't work
> because it means changing the org.apache.catalina.Context api to work in
> seconds instead of minutes. ManagerBase and StandardContext are in different
> packages, so a protected accessor for the timeout in seconds wouldn't work
> either.
>
> On the other hand, I wondered about defining an alternative timeout in
> seconds within the ManagerBase, which would only be used by unit tests.
> ManagerBase would use its own timeout if non-zero, otherwise use the Context
> value in minutes.
>
> I would really appreciate some guidance on the best way to proceed. (Perhaps
> it is better to keep things simple by living with an 80 seconds test time?)
>
I see two other ways besides modifying the context class:
a) call ManagerBase.setMaxInactiveInterval(), as it is public.
b) сall HttpSession.setMaxInactiveInterval() on that specific session,
e.g. from a HttpSessionListener
BTW, using an HttpSessionListener you can note whether the session has
actually expired (whether sessionDestroyed() was called for it).
The listener could be used to further speed up the test. As far as the
listener knows that all created sessions have expired, the test can
proceed. (So if the background thread runs sooner that in 15 seconds,
the test can continue sooner).
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1415184 - in /tomcat/tc7.0.x/trunk: ./ test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java
webapps/docs/changelog.xml
Posted by Brian Burch <br...@pingtoo.com>.
On 03/12/12 16:22, Brian Burch wrote:
> On 03/12/12 11:44, Brian Burch wrote:
>> On 02/12/12 22:00, Konstantin Kolinko wrote:
> <snip/>
>>> According to Buildbot logs, testBasicLoginSessionTimeout() runs for 2
>>> minutes (125 seconds), mainly due to a sleep() call.
>>>
>>> I wish there were a way to speed up this test.
>>>
>>> My thoughts
>>> a) Maybe use reflection to reduce StandardSession.lastAccessedTime and
>>> thisAccessedTime by some fixed amount (60000) instead of waiting for
>>> that time to pass.
>>
>> That would speed up the test... but it sounds like adding a time machine
>> to the test class! My feeling is this would add inappropriate logical
>> complexity to a test that has always created and destroyed a tomcat
>> instance for each test case (there are now 15).
>>
>> However, I intend to replicate most of the improvements from this test
>> class into the other authenticator tests, so I am already apprehensive
>> about adding too many 60+ second delays to the entire suite.
>>
>> Mark and I briefly discussed adding a new protected method for use by
>> these timeout tests:
>>
>> org.apache.catalina.session.StandardSession.setSessionTimeoutSecs(int
>> secs)
>
> Silly me! Sorry if that left you wondering what I meant. That particular
> method already exists, is public, and is called setSessionTimeout.
>
> What I meant to suggest was this:
>
> org.apache.catalina.core.StandardContext.setSessionTimeoutSecs(int timeout)
>
> because it would be much simpler to modify the Context in the test
> setUp, rather than each individual Session.
>
>> What do you think?
The change seemed almost trivial to me, but I couldn't work out how to
implement it without changing the public api, i.e.
I have only found one place where it is used:
org.apache.catalina.session.ManagerBase.setContext(Context context)
has this logic:
// Register with the new Context (if any)
if (this.context != null) {
setMaxInactiveInterval(this.context.getSessionTimeout() * 60);
this.context.addPropertyChangeListener(this);
}
I wanted to move the "60" back into StandardContext, but that doesn't
work because it means changing the org.apache.catalina.Context api to
work in seconds instead of minutes. ManagerBase and StandardContext are
in different packages, so a protected accessor for the timeout in
seconds wouldn't work either.
On the other hand, I wondered about defining an alternative timeout in
seconds within the ManagerBase, which would only be used by unit tests.
ManagerBase would use its own timeout if non-zero, otherwise use the
Context value in minutes.
I would really appreciate some guidance on the best way to proceed.
(Perhaps it is better to keep things simple by living with an 80 seconds
test time?)
> <snip/>
>>
>>> Best regards,
>>> Konstantin Kolinko
>>>
>>> ---------------------------------------------------------------------
>>> 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
Re: svn commit: r1415184 - in /tomcat/tc7.0.x/trunk: ./ test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java
webapps/docs/changelog.xml
Posted by Brian Burch <br...@pingtoo.com>.
On 03/12/12 11:44, Brian Burch wrote:
> On 02/12/12 22:00, Konstantin Kolinko wrote:
<snip/>
>> According to Buildbot logs, testBasicLoginSessionTimeout() runs for 2
>> minutes (125 seconds), mainly due to a sleep() call.
>>
>> I wish there were a way to speed up this test.
>>
>> My thoughts
>> a) Maybe use reflection to reduce StandardSession.lastAccessedTime and
>> thisAccessedTime by some fixed amount (60000) instead of waiting for
>> that time to pass.
>
> That would speed up the test... but it sounds like adding a time machine
> to the test class! My feeling is this would add inappropriate logical
> complexity to a test that has always created and destroyed a tomcat
> instance for each test case (there are now 15).
>
> However, I intend to replicate most of the improvements from this test
> class into the other authenticator tests, so I am already apprehensive
> about adding too many 60+ second delays to the entire suite.
>
> Mark and I briefly discussed adding a new protected method for use by
> these timeout tests:
>
> org.apache.catalina.session.StandardSession.setSessionTimeoutSecs(int secs)
Silly me! Sorry if that left you wondering what I meant. That particular
method already exists, is public, and is called setSessionTimeout.
What I meant to suggest was this:
org.apache.catalina.core.StandardContext.setSessionTimeoutSecs(int timeout)
because it would be much simpler to modify the Context in the test
setUp, rather than each individual Session.
> What do you think?
>
<snip/>
>
>> Best regards,
>> Konstantin Kolinko
>>
>> ---------------------------------------------------------------------
>> 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
Re: svn commit: r1415184 - in /tomcat/tc7.0.x/trunk: ./ test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java
webapps/docs/changelog.xml
Posted by Brian Burch <br...@pingtoo.com>.
On 02/12/12 22:00, Konstantin Kolinko wrote:
> 2012/11/29 <ma...@apache.org>:
>> Author: markt
>> Date: Thu Nov 29 14:35:02 2012
>> New Revision: 1415184
>>
>> URL: http://svn.apache.org/viewvc?rev=1415184&view=rev
>> Log:
>> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>> Improve unit tests.
>> Patch by Brian Burch.
>>
>> Modified:
>> tomcat/tc7.0.x/trunk/ (props changed)
>> tomcat/tc7.0.x/trunk/test/org/apache/catalina/authenticator/TestNonLoginAndBasicAuthenticator.java
>> tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
>>
>> Propchange: tomcat/tc7.0.x/trunk/
>> ------------------------------------------------------------------------------
>> Merged /tomcat/trunk:r1415177-1415179
>>
>
>> + private static final int SHORT_TIMEOUT_MINS = 1;
>> + private static final int LONG_TIMEOUT_MINS = 2;
>> + private static final int MANAGER_SCAN_DELAY_SECS = 60;
>> + private static final int EXTRA_DELAY_SECS = 5;
>> + private static final long TIMEOUT_DELAY_MSECS =
>> + (((SHORT_TIMEOUT_MINS * 60)
>> + + MANAGER_SCAN_DELAY_SECS + EXTRA_DELAY_SECS) * 1000);
>
> ...
>
>> + // allow the session to time out and lose authentication
>> + Thread.sleep(TIMEOUT_DELAY_MSECS);
>
>
> According to Buildbot logs, testBasicLoginSessionTimeout() runs for 2
> minutes (125 seconds), mainly due to a sleep() call.
>
> I wish there were a way to speed up this test.
>
> My thoughts
> a) Maybe use reflection to reduce StandardSession.lastAccessedTime and
> thisAccessedTime by some fixed amount (60000) instead of waiting for
> that time to pass.
That would speed up the test... but it sounds like adding a time machine
to the test class! My feeling is this would add inappropriate logical
complexity to a test that has always created and destroyed a tomcat
instance for each test case (there are now 15).
However, I intend to replicate most of the improvements from this test
class into the other authenticator tests, so I am already apprehensive
about adding too many 60+ second delays to the entire suite.
Mark and I briefly discussed adding a new protected method for use by
these timeout tests:
org.apache.catalina.session.StandardSession.setSessionTimeoutSecs(int secs)
What do you think?
> b) Maybe call ManagerBase.setProcessExpiresFrequency(1) instead of the
> default value (6) and reduce MANAGER_SCAN_DELAY_SECS from 60 to 10.
Mark and I discussed this idea earlier, but I was reluctant to increase
complexity in an already radical change to the original.
Your suggestion showed me how to achieve faster detection in a fairly
simple manner, so I have reopened the bug and provided a new patch that
shaves 50 seconds off the run time of the timeout test. Thanks!
> Best regards,
> Konstantin Kolinko
>
> ---------------------------------------------------------------------
> 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