You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2006/11/05 02:11:12 UTC
svn commit: r471309 - in /tomcat/container/tc5.5.x:
catalina/src/share/org/apache/catalina/session/StandardSession.java
webapps/docs/changelog.xml
Author: markt
Date: Sat Nov 4 17:11:11 2006
New Revision: 471309
URL: http://svn.apache.org/viewvc?view=rev&rev=471309
Log:
Improve fix for 37356.
Modified:
tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java
tomcat/container/tc5.5.x/webapps/docs/changelog.xml
Modified: tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java
URL: http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java?view=diff&rev=471309&r1=471308&r2=471309
==============================================================================
--- tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java (original)
+++ tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java Sat Nov 4 17:11:11 2006
@@ -612,8 +612,10 @@
evaluateIfValid();
- synchronized (lock) {
- accessCount++;
+ if (Globals.STRICT_SERVLET_COMPLIANCE) {
+ synchronized (lock) {
+ accessCount++;
+ }
}
}
@@ -625,10 +627,12 @@
public void endAccess() {
isNew = false;
- synchronized (lock) {
- accessCount--;
+
+ if (Globals.STRICT_SERVLET_COMPLIANCE) {
+ synchronized (lock) {
+ accessCount--;
+ }
}
-
}
Modified: tomcat/container/tc5.5.x/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/webapps/docs/changelog.xml?view=diff&rev=471309&r1=471308&r2=471309
==============================================================================
--- tomcat/container/tc5.5.x/webapps/docs/changelog.xml (original)
+++ tomcat/container/tc5.5.x/webapps/docs/changelog.xml Sat Nov 4 17:11:11 2006
@@ -62,7 +62,15 @@
StandardWrapper. (markt)
</fix>
<fix>
- <bug>37356</bug>: Ensure sessions time out correctly. (markt)
+ <bug>37356</bug>: Ensure sessions time out correctly. This has been
+ fixed by removing the accessCount feature by default. This feature
+ prevents the session from timing out whilst requests that last
+ longer than the session time out are being processed. This feature
+ is enabled by setting the Java option
+ <code>-Dorg.apache.catalina.STRICT_SERVLET_COMPLIANCE=true</code>
+ The feature is now implemented with synchronization which addresses
+ the thread safety issues associated with the original bug report.
+ (markt)
</fix>
<fix>
<bug>40528</bug>: Add missing message localisations as provided by
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r471309 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/session/StandardSession.java
webapps/docs/changelog.xml
Posted by Remy Maucherat <re...@apache.org>.
Sandy McArthur wrote:
> Attached is what I think may be an even more improved fix for 37356.
> It does away with the accessCount which should make Remy happy.
> It should prevent sessions currently in use from expiring which should
> make those other 2 people and myself happy.
> It doesn't add any synchronization but I believe it to be thread-safe
> which should make speed people happy.
That's ok: I think long requests are an aberration (they tie up a
thread, so it really won't scale), so I'm ok with having a switch for
that, with syncs (5.5) or atomic ints (6.0).
We can look into other strategies to do the same thing, of course. Your
trick is quite smart.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r471309 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/session/StandardSession.java webapps/docs/changelog.xml
Posted by Sandy McArthur <sa...@apache.org>.
After having slept on it and reading more of the servlet spec I no
longer think this is the best solution. I think it would make Tomcat
more divergent from the spec in the common case which is not good. I
think I was trying to be too clever. If someone wants the patch, I'll
send it to you.
On 11/9/06, Filip Hanik - Dev Lists <de...@hanik.com> wrote:
> I tend to agree with Rainer, session expiration should be done with a
> predictable pattern
> the trick below is neat, the idea of it, but for practical session
> expiration I'm not so sure.
>
> Filip
>
> Rainer Jung wrote:
> > Hi,
> >
> > Sandy McArthur schrieb:
> >
> >> The way it works is the StandardSessionFacade is referenced like it
> >> was with the facade field in StandardSession and a WeakReference to
> >> the StandardSessionFacade is added the the field facadeReference.
> >> After the maxInactiveInterval the facade field is set to null. Next
> >> time the garbage collector it should break the WeakReference and the
> >> next time the StandardSession is checked to see if it is still valid,
> >> it will be expired.
> >>
> >
> > I think session expiration neeeds to be done with some quality of
> > service. Of course we don't aim at immediate expiration, but I think we
> > should prevent getting worse than the default of 1 minute granularity we
> > have at the moment (plus configurable).
> >
> > If we depend on GC for session expiration, the quality of this service
> > is out of our control. I can easily imagine the relvant objects getting
> > moved to the tenured space, which will be subject to GC very rarely
> > (like every hour or only once a day). And such behaviour is not wrong,
> > instead one should try to configure the new generation and the semi
> > spaces to keep GC on the tenured space very low.
> >
> >
> > ...
> >
> >> If a request for the session comes in after the maxInactiveInterval
> >> but before the WeakReference is broken the facade field will be
> >> updated with the value of the weak reference to prevent the session
> >> expiration.
> >>
> >
> > Is that good? It sounds like: great, if the session is not expired and
> > the user is coming back he will love to find his session is still there.
> >
> > But: if it's online banking and it's not really the user, they'll not
> > like the idea of "your session timeout expired long ago, but you can
> > still use the session".
> >
> > So in my opinion we should not make session expirration dependant on GC
> > runs. GC invocations are totally out of our control and major GC changes
> > are to be expected. Session expiration needs to be done with a defined
> > quality of service.
> >
> > Regards,
> >
> > Rainer
> >
> >
> > ---------------------------------------------------------------------
> > 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
>
>
--
Sandy McArthur
"He who dares not offend cannot be honest."
- Thomas Paine
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r471309 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/session/StandardSession.java
webapps/docs/changelog.xml
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
I tend to agree with Rainer, session expiration should be done with a
predictable pattern
the trick below is neat, the idea of it, but for practical session
expiration I'm not so sure.
Filip
Rainer Jung wrote:
> Hi,
>
> Sandy McArthur schrieb:
>
>> The way it works is the StandardSessionFacade is referenced like it
>> was with the facade field in StandardSession and a WeakReference to
>> the StandardSessionFacade is added the the field facadeReference.
>> After the maxInactiveInterval the facade field is set to null. Next
>> time the garbage collector it should break the WeakReference and the
>> next time the StandardSession is checked to see if it is still valid,
>> it will be expired.
>>
>
> I think session expiration neeeds to be done with some quality of
> service. Of course we don't aim at immediate expiration, but I think we
> should prevent getting worse than the default of 1 minute granularity we
> have at the moment (plus configurable).
>
> If we depend on GC for session expiration, the quality of this service
> is out of our control. I can easily imagine the relvant objects getting
> moved to the tenured space, which will be subject to GC very rarely
> (like every hour or only once a day). And such behaviour is not wrong,
> instead one should try to configure the new generation and the semi
> spaces to keep GC on the tenured space very low.
>
>
> ...
>
>> If a request for the session comes in after the maxInactiveInterval
>> but before the WeakReference is broken the facade field will be
>> updated with the value of the weak reference to prevent the session
>> expiration.
>>
>
> Is that good? It sounds like: great, if the session is not expired and
> the user is coming back he will love to find his session is still there.
>
> But: if it's online banking and it's not really the user, they'll not
> like the idea of "your session timeout expired long ago, but you can
> still use the session".
>
> So in my opinion we should not make session expirration dependant on GC
> runs. GC invocations are totally out of our control and major GC changes
> are to be expected. Session expiration needs to be done with a defined
> quality of service.
>
> Regards,
>
> Rainer
>
>
> ---------------------------------------------------------------------
> 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: r471309 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/session/StandardSession.java
webapps/docs/changelog.xml
Posted by Rainer Jung <ra...@kippdata.de>.
Hi,
Sandy McArthur schrieb:
> The way it works is the StandardSessionFacade is referenced like it
> was with the facade field in StandardSession and a WeakReference to
> the StandardSessionFacade is added the the field facadeReference.
> After the maxInactiveInterval the facade field is set to null. Next
> time the garbage collector it should break the WeakReference and the
> next time the StandardSession is checked to see if it is still valid,
> it will be expired.
I think session expiration neeeds to be done with some quality of
service. Of course we don't aim at immediate expiration, but I think we
should prevent getting worse than the default of 1 minute granularity we
have at the moment (plus configurable).
If we depend on GC for session expiration, the quality of this service
is out of our control. I can easily imagine the relvant objects getting
moved to the tenured space, which will be subject to GC very rarely
(like every hour or only once a day). And such behaviour is not wrong,
instead one should try to configure the new generation and the semi
spaces to keep GC on the tenured space very low.
>
...
>
> If a request for the session comes in after the maxInactiveInterval
> but before the WeakReference is broken the facade field will be
> updated with the value of the weak reference to prevent the session
> expiration.
Is that good? It sounds like: great, if the session is not expired and
the user is coming back he will love to find his session is still there.
But: if it's online banking and it's not really the user, they'll not
like the idea of "your session timeout expired long ago, but you can
still use the session".
So in my opinion we should not make session expirration dependant on GC
runs. GC invocations are totally out of our control and major GC changes
are to be expected. Session expiration needs to be done with a defined
quality of service.
Regards,
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r471309 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/session/StandardSession.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
Sandy McArthur wrote:
> Attached is what I think may be an even more improved fix for 37356.
> It does away with the accessCount which should make Remy happy.
> It should prevent sessions currently in use from expiring which should
> make those other 2 people and myself happy.
> It doesn't add any synchronization but I believe it to be thread-safe
> which should make speed people happy.
It sounds good. Attachments tend to get dropped on the mailing lists
so please re-open the bug report and attach your improved patch.
Cheers,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r471309 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/session/StandardSession.java webapps/docs/changelog.xml
Posted by Sandy McArthur <sa...@apache.org>.
Attached is what I think may be an even more improved fix for 37356.
It does away with the accessCount which should make Remy happy.
It should prevent sessions currently in use from expiring which should
make those other 2 people and myself happy.
It doesn't add any synchronization but I believe it to be thread-safe
which should make speed people happy.
The negative is session expiration is now partially dependent on the
garbage collector.
The way it works is the StandardSessionFacade is referenced like it
was with the facade field in StandardSession and a WeakReference to
the StandardSessionFacade is added the the field facadeReference.
After the maxInactiveInterval the facade field is set to null. Next
time the garbage collector it should break the WeakReference and the
next time the StandardSession is checked to see if it is still valid,
it will be expired.
If there are any long running requests, they will maintain a "strong"
reference to the session facade preventing the standard session from
being expired.
If a request for the session comes in after the maxInactiveInterval
but before the WeakReference is broken the facade field will be
updated with the value of the weak reference to prevent the session
expiration.
The patch still needs a force session expire time in case a webapp
holds on to a session object after it's done processing a request. I
was thinking that could be 5 times the maxInactiveInterval value.
Is there any reason this solution isn't going to work that I'm not
aware of? I haven't done any long running test with this code yet
either. Patch is against the lastest svn as of this morning.
On 11/4/06, markt@apache.org <ma...@apache.org> wrote:
> Author: markt
> Date: Sat Nov 4 17:11:11 2006
> New Revision: 471309
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=471309
> Log:
> Improve fix for 37356.
>
> Modified:
> tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java
> tomcat/container/tc5.5.x/webapps/docs/changelog.xml
>
> Modified: tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java
> URL: http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java?view=diff&rev=471309&r1=471308&r2=471309
> ==============================================================================
> --- tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java (original)
> +++ tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java Sat Nov 4 17:11:11 2006
> @@ -612,8 +612,10 @@
>
> evaluateIfValid();
>
> - synchronized (lock) {
> - accessCount++;
> + if (Globals.STRICT_SERVLET_COMPLIANCE) {
> + synchronized (lock) {
> + accessCount++;
> + }
> }
>
> }
> @@ -625,10 +627,12 @@
> public void endAccess() {
>
> isNew = false;
> - synchronized (lock) {
> - accessCount--;
> +
> + if (Globals.STRICT_SERVLET_COMPLIANCE) {
> + synchronized (lock) {
> + accessCount--;
> + }
> }
> -
> }
>
>
>
> Modified: tomcat/container/tc5.5.x/webapps/docs/changelog.xml
> URL: http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/webapps/docs/changelog.xml?view=diff&rev=471309&r1=471308&r2=471309
> ==============================================================================
> --- tomcat/container/tc5.5.x/webapps/docs/changelog.xml (original)
> +++ tomcat/container/tc5.5.x/webapps/docs/changelog.xml Sat Nov 4 17:11:11 2006
> @@ -62,7 +62,15 @@
> StandardWrapper. (markt)
> </fix>
> <fix>
> - <bug>37356</bug>: Ensure sessions time out correctly. (markt)
> + <bug>37356</bug>: Ensure sessions time out correctly. This has been
> + fixed by removing the accessCount feature by default. This feature
> + prevents the session from timing out whilst requests that last
> + longer than the session time out are being processed. This feature
> + is enabled by setting the Java option
> + <code>-Dorg.apache.catalina.STRICT_SERVLET_COMPLIANCE=true</code>
> + The feature is now implemented with synchronization which addresses
> + the thread safety issues associated with the original bug report.
> + (markt)
> </fix>
> <fix>
> <bug>40528</bug>: Add missing message localisations as provided by
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
--
Sandy McArthur
"He who dares not offend cannot be honest."
- Thomas Paine