On 25/03/2010, Konstantin Kolinko <[email protected]> wrote:
> 2010/3/24 sebb <[email protected]>:
>
> > On 24/03/2010, [email protected] <[email protected]> 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: [email protected]
> For additional commands, e-mail: [email protected]
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
