You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by sebb <se...@gmail.com> on 2010/03/24 16:54:16 UTC
Re: svn commit: r927037 - in /tomcat/trunk/java/org/apache/catalina:
ha/session/DeltaManager.java session/ManagerBase.java
On 24/03/2010, markt@apache.org <ma...@apache.org> wrote:
> Author: markt
> Date: Wed Mar 24 12:38:23 2010
> New Revision: 927037
>
> URL: http://svn.apache.org/viewvc?rev=927037&view=rev
> Log:
> Simpler fix for https://issues.apache.org/bugzilla/show_bug.cgi?id=48790 based on a patch by kkolinko
> Make maxActive thread safe. Probably unnecessary but technically a bug.
>
> Modified:
> tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java
> tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java
>
> Modified: tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java?rev=927037&r1=927036&r2=927037&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java Wed Mar 24 12:38:23 2010
> @@ -1160,7 +1160,7 @@ public class DeltaManager extends Cluste
> rejectedSessions = 0 ;
> sessionReplaceCounter = 0 ;
> counterNoStateTransfered = 0 ;
> - maxActive = getActiveSessions() ;
> + setMaxActive(getActiveSessions());
> sessionCounter = getActiveSessions() ;
> counterReceive_EVT_ALL_SESSION_DATA = 0;
> counterReceive_EVT_GET_ALL_SESSIONS = 0;
>
> Modified: tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java?rev=927037&r1=927036&r2=927037&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java Wed Mar 24 12:38:23 2010
> @@ -183,7 +183,9 @@ public abstract class ManagerBase extend
> // Number of sessions created by this manager
> protected int sessionCounter=0;
>
> - protected int maxActive=0;
> + protected volatile int maxActive=0;
> +
> + private final Object maxActiveUpdateLock = new Object();
>
> // number of duplicated session ids - anything >0 means we have problems
> protected int duplicates=0;
> @@ -765,7 +767,11 @@ public abstract class ManagerBase extend
> sessions.put(session.getIdInternal(), session);
> int size = sessions.size();
> if( size > maxActive ) {
> - maxActive = size;
> + synchronized(maxActiveUpdateLock) {
> + if( size > maxActive ) {
> + maxActive = size;
> + }
> + }
In the code above, maxActive is read twice, and may change in between reads.
It's probably not a problem, but IMO should be documented that the
possibility has been considered.
> }
> }
>
> @@ -1081,7 +1087,9 @@ public abstract class ManagerBase extend
>
>
> public void setMaxActive(int maxActive) {
> - this.maxActive = maxActive;
> + synchronized (maxActiveUpdateLock) {
> + this.maxActive = maxActive;
> + }
> }
Looks a bit odd to use both volatile and synchronized.
There seems to be no point to the synch. in setMaxActive, given that
the field is volatile.
Also, the field is protected, which allows subclasses to bypass the synch.
It would be safer to make the field private, remove the volatile
qualifier, and add a synchronised getter.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r927037 - in /tomcat/trunk/java/org/apache/catalina:
ha/session/DeltaManager.java session/ManagerBase.java
Posted by sebb <se...@gmail.com>.
On 25/03/2010, Konstantin Kolinko <kn...@gmail.com> wrote:
> 2010/3/24 sebb <se...@gmail.com>:
>
> > On 24/03/2010, markt@apache.org <ma...@apache.org> wrote:
> >> Author: markt
> >> Date: Wed Mar 24 12:38:23 2010
> >> New Revision: 927037
> >>
> >> URL: http://svn.apache.org/viewvc?rev=927037&view=rev
> >> Log:
> >> Simpler fix for https://issues.apache.org/bugzilla/show_bug.cgi?id=48790 based on a patch by kkolinko
> >> Make maxActive thread safe. Probably unnecessary but technically a bug.
> >>
> >> Modified:
> >> tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java
> >> tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java
> >>
>
>
> >> @@ -765,7 +767,11 @@ public abstract class ManagerBase extend
> >> sessions.put(session.getIdInternal(), session);
> >> int size = sessions.size();
> >> if( size > maxActive ) {
> >> - maxActive = size;
> >> + synchronized(maxActiveUpdateLock) {
> >> + if( size > maxActive ) {
> >> + maxActive = size;
> >> + }
> >> + }
> >
> > In the code above, maxActive is read twice, and may change in between reads.
> >
> > It's probably not a problem, but IMO should be documented that the
> > possibility has been considered.
> >
>
>
> That is what "double-checked locking " is about: reading something
> twice, once before the lock and once the lock is acquired. I do not
> see a need to explicitly document that.
>
Double-checked locking (DCL) normally applies to one-time
initialisation, and normally only applies where a single method does
the modifications.
Here the behaviour is more complicated, because the setMaxActive()
method can also change the value between the two checks, potentially
causing a missed update in the DCL code.
Maybe setMaxActive() is never called in such a way as to cause a
problem with this, but it's not obvious if that's the case.
>
> >> @@ -1081,7 +1087,9 @@ public abstract class ManagerBase extend
> >>
> >>
> >> public void setMaxActive(int maxActive) {
> >> - this.maxActive = maxActive;
> >> + synchronized (maxActiveUpdateLock) {
> >> + this.maxActive = maxActive;
> >> + }
> >> }
> >
> > Looks a bit odd to use both volatile and synchronized.
> > There seems to be no point to the synch. in setMaxActive, given that
> > the field is volatile.
> >
>
>
> That is similar to my idea in the original patch
> (2010-03-16_tc6_bug48790.patch),
> when I skipped adding sync to this method. My though was that
> setMaxActive/DeltaManager and if( size > maxActive ) { maxActive =
> size; } block both assign effectively the same value -- the current
> count of sessions, so I see no conflict between them.
>
> I will go with Mark's patch, though, because I expect explicit calls
> to setMaxActive() to be rare, and so whether there is a sync is not a
> big matter.
True, but it looks odd.
>
> > Also, the field is protected, which allows subclasses to bypass the synch.
> >
> > It would be safer to make the field private, remove the volatile
> > qualifier, and add a synchronised getter.
I forgot - one would need to remove DCL.
> >
>
>
> A synchronized getter is what we surely do not need here. We can go
> back to the separate read and write locks though.
It's still a bad idea to expose the field, albeit protected.
There are public getter/setter methods.
>
> We need volatile so that the current value of 'maxActive' were
> consistent between threads.
Volatile is needed for the DCL code as well.
> Even the write lock may look a bit excessive. But - why not?
>
> 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
Re: svn commit: r927037 - in /tomcat/trunk/java/org/apache/catalina:
ha/session/DeltaManager.java session/ManagerBase.java
Posted by Konstantin Kolinko <kn...@gmail.com>.
2010/3/24 sebb <se...@gmail.com>:
> On 24/03/2010, markt@apache.org <ma...@apache.org> wrote:
>> Author: markt
>> Date: Wed Mar 24 12:38:23 2010
>> New Revision: 927037
>>
>> URL: http://svn.apache.org/viewvc?rev=927037&view=rev
>> Log:
>> Simpler fix for https://issues.apache.org/bugzilla/show_bug.cgi?id=48790 based on a patch by kkolinko
>> Make maxActive thread safe. Probably unnecessary but technically a bug.
>>
>> Modified:
>> tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java
>> tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java
>>
>> @@ -765,7 +767,11 @@ public abstract class ManagerBase extend
>> sessions.put(session.getIdInternal(), session);
>> int size = sessions.size();
>> if( size > maxActive ) {
>> - maxActive = size;
>> + synchronized(maxActiveUpdateLock) {
>> + if( size > maxActive ) {
>> + maxActive = size;
>> + }
>> + }
>
> In the code above, maxActive is read twice, and may change in between reads.
>
> It's probably not a problem, but IMO should be documented that the
> possibility has been considered.
>
That is what "double-checked locking " is about: reading something
twice, once before the lock and once the lock is acquired. I do not
see a need to explicitly document that.
>> @@ -1081,7 +1087,9 @@ public abstract class ManagerBase extend
>>
>>
>> public void setMaxActive(int maxActive) {
>> - this.maxActive = maxActive;
>> + synchronized (maxActiveUpdateLock) {
>> + this.maxActive = maxActive;
>> + }
>> }
>
> Looks a bit odd to use both volatile and synchronized.
> There seems to be no point to the synch. in setMaxActive, given that
> the field is volatile.
>
That is similar to my idea in the original patch
(2010-03-16_tc6_bug48790.patch),
when I skipped adding sync to this method. My though was that
setMaxActive/DeltaManager and if( size > maxActive ) { maxActive =
size; } block both assign effectively the same value -- the current
count of sessions, so I see no conflict between them.
I will go with Mark's patch, though, because I expect explicit calls
to setMaxActive() to be rare, and so whether there is a sync is not a
big matter.
> Also, the field is protected, which allows subclasses to bypass the synch.
>
> It would be safer to make the field private, remove the volatile
> qualifier, and add a synchronised getter.
>
A synchronized getter is what we surely do not need here. We can go
back to the separate read and write locks though.
We need volatile so that the current value of 'maxActive' were
consistent between threads. Even the write lock may look a bit
excessive. But - why not?
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org